[PATCH] problems in eval_options()

[Martin Sebor <se...@roguewave.com>]

I've discovered a number of problems with the eval_options() function
(see below). The attached patch implements fixes for them all. Andrew,
let me know if you see any problems with it. If not, I'll commit it
later today.

Btw., as an FYI, C99 strtol() is declared to take a couple of restrict
pointers:

     long int strtol(const char * restrict nptr,
                     char ** restrict endptr,
                     int base);

which means that the behavior of the function is undefined when
(nptr == &nptr) is true. I fixed this as well.

Martin

1. Undefined behavior for "-t"
$ ./exec -t
Segmentation Fault (core dumped)

2. Negative argument to -t not diagnosed:
$ ./exec -t -1; echo $?
0

3. Poor diagnostic of invalid argument to -t:
$ ./exec -t 1x
./exec: Unknown value for -t: x

4. Abort on missing option argument:
$ ./exec --exit
Assertion failed: 0 != opt, file /build/sebor/stdcxx/util/cmdopt.cpp, 
line 141
Abort (core dumped)

5. Missing text for invalid argument:
$ ./exec --exit=9999999999
./exec: Unknown value for --exit:

6. Invalid argument to --signal and --ignorte not diagnosed:
$ ./exec --signal 12345; echo $?
0
$ ./exec --ignore 12345; echo $?
0

With the patch applied:

$ ./exec -t
./exec: Missing argument for -t
$ ./exec -t -1
./exec: Bad argument for -t: -1
$ ./exec -t 1x
./exec: Bad argument for -t: 1x
$ ./exec --exit
./exec: Missing argument for --exit
$ ./exec --exit=9999999999
./exec: Bad argument for --exit: 9999999999
$ ./exec --signal 12345
./exec: kill(20842, SIG#12345) failed: Invalid argument
$ ./exec --ignore 12345 
      ./exec: sigaction(SIG#12345, ...) failed: Invalid argument

Re: [PATCH] problems in eval_options()

[Andrew Black <ab...@roguewave.com>]

Martin Sebor wrote:
> Andrew Black wrote:
>> Greetings Martin
>>
>> I've glanced over the patch and have attached a revised version that I 
>> feel is a tad more consistent.
> 
> What? The consistency of my patch being challenged?! ;-)
> 
>> * Invalid/missing options error messages are now routed through the 
>> (new) bad_value and missing_value functions.  I prefer these to 
>> calling terminate(), as they also display the usage instructions when 
>> the option is incorrectly used.
> 
> I don't think we should be displaying the entire help screen for
> each typo. I have reservations about doing that for invalid option,
> let alone for their arguments. The help text is long and might drown
> out the error message itself. It's also not how most implementations
> of utilities behave. So unless you have a really good argument for
> doing so I'll remove this bit.

*shrug*  I'm not too attached to displaying the full usage instructions. 
  An advantage of creating the additional help functions is you can 
alter the behavior in one or two places as whims change, rather than 
having to hunt down all the places changes are required.  I suppose my 
reason for dumping the full help text is so the user can know what is 
expected, but the text might not be as helpful on that score as it could be.

--Andrew Black

Re: [PATCH] problems in eval_options()

[Martin Sebor <se...@roguewave.com>]

Andrew Black wrote:
> Greetings Martin
> 
> I've glanced over the patch and have attached a revised version that I 
> feel is a tad more consistent.

What? The consistency of my patch being challenged?! ;-)

> * Invalid/missing options error messages are now routed through the 
> (new) bad_value and missing_value functions.  I prefer these to calling 
> terminate(), as they also display the usage instructions when the option 
> is incorrectly used.

I don't think we should be displaying the entire help screen for
each typo. I have reservations about doing that for invalid option,
let alone for their arguments. The help text is long and might drown
out the error message itself. It's also not how most implementations
of utilities behave. So unless you have a really good argument for
doing so I'll remove this bit.

> * the -d and -x switches now check for a missing value for consistency 
> with the -t switch.

Okay.

> * use 'sizeof option - 1' rather than a magic constant for parameter 3 
> when calling get_long_val

Ah, I missed those. Good catch.

> * Removed superfluous assignment to optname for the compat and nocompat 
> switches (This one's debatable).

I tried to be (but wasn't) consistent about the assignment even
where it wasn't needed in case we made changes in the future
that assumed the variables were non-null. I suppose since we
don't set the variables for options like -q or -h we might as
well avoid setting them for these two.

> * Moved declaration of act struct in one conditional

Martin

Re: [PATCH] problems in eval_options()

[Andrew Black <ab...@roguewave.com>]

Greetings Martin

I've glanced over the patch and have attached a revised version that I 
feel is a tad more consistent.
* Invalid/missing options error messages are now routed through the 
(new) bad_value and missing_value functions.  I prefer these to calling 
terminate(), as they also display the usage instructions when the option 
is incorrectly used.
* the -d and -x switches now check for a missing value for consistency 
with the -t switch.
* use 'sizeof option - 1' rather than a magic constant for parameter 3 
when calling get_long_val
* Removed superfluous assignment to optname for the compat and nocompat 
switches (This one's debatable).
* Moved declaration of act struct in one conditional

--Andrew Black

Martin Sebor wrote:
> I've discovered a number of problems with the eval_options() function
> (see below). The attached patch implements fixes for them all. Andrew,
> let me know if you see any problems with it. If not, I'll commit it
> later today.
> 
> Btw., as an FYI, C99 strtol() is declared to take a couple of restrict
> pointers:
> 
>     long int strtol(const char * restrict nptr,
>                     char ** restrict endptr,
>                     int base);
> 
> which means that the behavior of the function is undefined when
> (nptr == &nptr) is true. I fixed this as well.
> 
> Martin
> 
> 1. Undefined behavior for "-t"
> $ ./exec -t
> Segmentation Fault (core dumped)
> 
> 2. Negative argument to -t not diagnosed:
> $ ./exec -t -1; echo $?
> 0
> 
> 3. Poor diagnostic of invalid argument to -t:
> $ ./exec -t 1x
> ./exec: Unknown value for -t: x
> 
> 4. Abort on missing option argument:
> $ ./exec --exit
> Assertion failed: 0 != opt, file /build/sebor/stdcxx/util/cmdopt.cpp, 
> line 141
> Abort (core dumped)
> 
> 5. Missing text for invalid argument:
> $ ./exec --exit=9999999999
> ./exec: Unknown value for --exit:
> 
> 6. Invalid argument to --signal and --ignorte not diagnosed:
> $ ./exec --signal 12345; echo $?
> 0
> $ ./exec --ignore 12345; echo $?
> 0
> 
> With the patch applied:
> 
> $ ./exec -t
> ./exec: Missing argument for -t
> $ ./exec -t -1
> ./exec: Bad argument for -t: -1
> $ ./exec -t 1x
> ./exec: Bad argument for -t: 1x
> $ ./exec --exit
> ./exec: Missing argument for --exit
> $ ./exec --exit=9999999999
> ./exec: Bad argument for --exit: 9999999999
> $ ./exec --signal 12345
> ./exec: kill(20842, SIG#12345) failed: Invalid argument
> $ ./exec --ignore 12345      ./exec: sigaction(SIG#12345, ...) failed: 
> Invalid argument