Re: Possibly incomplete bounds check after strtol(3)

[G. Branden Robinson]

Hi Alex,

I forgot about this; fortunately, Dave reminded me.

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.

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.

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.

Let me know if I overlooked something (again).

Regards,
Branden

signature.asc
Description: PGP signature