Re: Possibly incomplete bounds check after strtol(3)

[Alejandro Colomar]

On Tue, Mar 12, 2024 at 11:22:32PM +0100, Alejandro Colomar wrote:
> 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.

Whoops, no.  The liwer bound is 2, and you still need the >2 test.


-- 
<https://www.alejandro-colomar.es/>
Looking for a remote C programming job at the moment.

signature.asc
Description: PGP signature