Re: Possibly incomplete bounds check after strtol(3)

[Alejandro Colomar]

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