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 */
