On Tue, Mar 12, 2024 at 05:05:31PM -0500, G. Branden Robinson wrote: > Hi Alex, > > I forgot about this; fortunately, Dave reminded me.
Hi Branden! (Hi Dave!)
> At 2024-01-14T20:19:18+0100, Alejandro Colomar wrote:
> > I see some code calling strtol(3) that I suspect won't behave well in
> > some systems:
> >
> > $ grepc -tfd check_integer_arg .
> > ./src/utils/indxbib/indxbib.cpp:static void check_integer_arg(char opt,
> > const char *arg, int min, int *res)
> > {
> > char *ptr;
> > long n = strtol(arg, &ptr, 10);
> > if (n == 0 && ptr == arg)
> > error("argument to -%1 not an integer", opt);
> > else if (n < min)
> > error("argument to -%1 must not be less than %2", opt, min);
> > else {
> > if (n > INT_MAX)
> > error("argument to -%1 greater than maximum integer", opt);
> > else if (*ptr != '\0')
> > error("junk after integer argument to -%1", opt);
> > *res = int(n);
> > }
> > }
> >
> >
> > I think these tests miss some corner cases:
> >
> > - If INT_MAX==LONG_MAX, then n>INT_MAX is impossible, but strtol(3)
> > will return LONG_MAX and errno ERANGE for values greater than that.
> > groff is silently accepting input >LONG_MAX in those systems, and
> > silently saturating it to LONG_MAX (INT_MAX).
>
> Yes--I forgot about systems where sizeof (int) == sizeof (long).
>
> So I reckon I'll throw the `long long` type and `strtoll()` at it. We
> claim to require a C99 compiler already.
That's still a problem on ILP64, ain't it? :)
Not that I like such systems, but Paul Eggert reminded me of their
existence when I suggested a similar fix for a similar problem some time
ago.
You'll need to just use a better API. strtoi(3), provided by the BSDs,
and by libbsd on non-BSD systems, is a better one. It had a bug until
earlier this year, when I fixed it, so you may want to avoid it. Then
you may become the first user of liba2i[1], or roll your own wrapper
(hopefully compatible with liba2i).
[1] <https://git.kernel.org/pub/scm/libs/liba2i/liba2i.git/>
If you want to use it, please let me know; I'm still working on
the build system. The source code, however, is tested, and I'd
say it's good.
> The call sites (and some context) are as follows.
>
> 79 int hash_table_size = DEFAULT_HASH_TABLE_SIZE;
>
> 147 case 'h':
> 148 {
> 149 int requested_hash_table_size;
> 150 check_integer_arg('h', optarg, 1, &requested_hash_table_size);
> 151 hash_table_size = requested_hash_table_size;
> 152 if ((hash_table_size > 2) && (hash_table_size % 2) == 0)
> 153 hash_table_size++;
> 154 while (!is_prime(hash_table_size))
> 155 hash_table_size += 2;
> 156 if (hash_table_size != requested_hash_table_size)
> 157 warning("requested hash table size %1 is not prime: using %2"
> 158 " instead", optarg, hash_table_size);
> 159 }
> 160 break;
>
> You may see another problem here. We accept '1' as an argument, but
> then pass it to a function called `is_prime()`...which fails an
> assertion on that input. Whoops.
Hmmm, yeah. So you could raise it to 3, and then also drop the >2 test.
> 164 case 'k':
> 165 check_integer_arg('k', optarg, 1, &max_keys_per_item);
> 166 break;
>
> 167 case 'l':
> 168 check_integer_arg('l', optarg, 0, &shortest_len);
> 169 break;
>
> 170 case 'n':
> 171 check_integer_arg('n', optarg, 0, &n_ignore_words);
> 172 break;
>
> 176 case 't':
> 177 check_integer_arg('t', optarg, 1, &truncate_len);
> 178 break;
>
> > - If min==INT_MIN==LONG_MIN, then a similar thing happens for
> > underflow.
>
> On the other hand, this will never happen. All of the call sites use a
> static literal minimum value, and that is always 0 or 1...or, soon, 2.
Hmm, ok. Let's hope nobody adds a call to this function with a
different 'min'.
Have a lovely day!
Alex
> Let me know if I overlooked something (again).
>
> Regards,
> Branden
--
<https://www.alejandro-colomar.es/>
Looking for a remote C programming job at the moment.
signature.asc
Description: PGP signature
