Re: [PATCH RFC] sscanf: Fix integer overflow with sscanf field width

From: Jan Beulich
Date: Fri May 24 2019 - 03:27:40 EST

>>> On 23.05.19 at 19:27, <noring@xxxxxxxxxx> wrote:
> This fixes 53809751ac23 ("sscanf: don't ignore field widths for numeric
> conversions"). sscanf overflows integers with simple strings such as dates.
> As an example, consider
> r = sscanf("20190523123456", "%4d%2d%2d%2d%2d%2d",
> &year, &month, &day,
> &hour, &minute, &second);
> pr_info("%d %04d-%02d-%2d %02d:%02d:%02d\n",
> r,
> year, month, day,
> hour, minute, second);
> On a 32-bit machine this prints
> 6 0000-05-23 12:34:56
> where the year is zero, and not 2019 as expected. The reason is that sscanf
> attempts to read 20190523123456 as a whole integer and then divide it with
> 10^10 to obtain 2019, which obviously fails. Of course, 64-bit machines fail
> similarly on longer numerical strings.

Right, and that's also what that commit's description says remains as
non-conforming behavior.

> I'm offering a simple patch to correct this below. The idea is to have a
> variant of _parse_integer() called _parse_integer_end(), with the ability
> to stop consuming digits. The functions
> simple_{strtol,strtoll,strtoul,strtoull}()
> now have the corresponding
> sscanf_{strtol,strtoll,strtoul,strtoull}()
> taking a field width into account. There are some code duplication issues
> etc. so one might consider making more extensive changes than these.

I'm not the maintainer here, but to me it looks mostly okay.

> +static long sscanf_strtol(const char *cp, int field_width,
> + char **endp, unsigned int base)
> +{
> + if (*cp == '-')
> + return -sscanf_strtoul(cp + 1, field_width - 1, endp, base);

I'm afraid you may neither convert a field width of zero to -1 here,
nor convert a field width of 1 to zero (unlimited).

I'd also like to note that the 'u' and 'x' format characters also accept
a sign as per the standard, but that's an orthogonal issue which you
may or may not want to address at the same time.