Hi Tobias,

Tobias Stoeckmann wrote on Tue, Mar 27, 2018 at 01:30:03PM +0200:

> This patch fixes an out of boundary write in cut:
> 
> $ cut -c 2147483648 -
> Segmentation fault (core dumped)
> 
> The supplied number can be parsed by strtol, but the result is casted
> into a signed int, therefore turning negative. Afterwards, it is used
> as an offset into a char array to write data at the given position.
> This leads to the shown segmentation fault.
> 
> I have changed the strtol call to strtonum, making sure that range
> specifications still work. Our cut regress tests pass. Also it has a
> nicer error message now, thanks to strtonum.
> 
> While at it, I replaced a manual for-loop with memset.

See inline for one optional suggestion.

OK schwarze@ either way.
  Ingo


> Index: usr.bin/cut/cut.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/cut/cut.c,v
> retrieving revision 1.23
> diff -u -p -u -p -r1.23 cut.c
> --- usr.bin/cut/cut.c 2 Dec 2015 00:56:46 -0000       1.23
> +++ usr.bin/cut/cut.c 27 Mar 2018 11:24:31 -0000
> @@ -154,11 +154,32 @@ int autostart, autostop, maxval;
>  
>  char positions[_POSIX2_LINE_MAX + 1];
>  
> +int
> +read_number(char **p)
> +{
> +     size_t pos;
> +     int dash, n;
> +     const char *errstr;
> +     char *q;
> +
> +     q = *p + strcspn(*p, "-");
> +     dash = *q == '-';
> +     *q = '\0';
> +     n = strtonum(*p, 1, _POSIX2_LINE_MAX, &errstr);
> +     if (errstr != NULL)
> +             errx(1, "[-bcf] list: %s %s (allowed 1-%d)", *p, errstr,
> +                 _POSIX2_LINE_MAX);
> +     if (dash)
> +             *q = '-';
> +     *p = q;
> +
> +     return n;
> +}
> +
>  void
>  get_list(char *list)
>  {
>       int setautostart, start, stop;
> -     char *pos;
>       char *p;
>  
>       /*
> @@ -176,13 +197,15 @@ get_list(char *list)
>                       setautostart = 1;
>               }
>               if (isdigit((unsigned char)*p)) {
> -                     start = stop = strtol(p, &p, 10);
> +                     start = stop = read_number(&p);
>                       if (setautostart && start > autostart)
>                               autostart = start;
>               }
>               if (*p == '-') {
> -                     if (isdigit((unsigned char)p[1]))
> -                             stop = strtol(p + 1, &p, 10);
> +                     if (isdigit((unsigned char)p[1])) {
> +                             ++p;
> +                             stop = read_number(&p);
> +                     }
>                       if (*p == '-') {
>                               ++p;
>                               if (!autostop || autostop > stop)
> @@ -193,13 +216,10 @@ get_list(char *list)
>                       errx(1, "[-bcf] list: illegal list value");
>               if (!stop || !start)
>                       errx(1, "[-bcf] list: values may not include zero");

Consider deleting these two lines, too.

You new function read_number() already makes sure that neither stop
nor start can be 0 at this point.

> -             if (stop > _POSIX2_LINE_MAX)
> -                     errx(1, "[-bcf] list: %d too large (max %d)",
> -                         stop, _POSIX2_LINE_MAX);
>               if (maxval < stop)
>                       maxval = stop;
> -             for (pos = positions + start; start++ <= stop; *pos++ = 1)
> -                     ;
> +             if (start <= stop)
> +                     memset(positions + start, 1, stop - start + 1);
>       }
>  
>       /* overlapping ranges */

Reply via email to