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