Re: [PATCH v2] string: Fix OOB read on generic strncmp

[Wilco Dijkstra <Wilco.Dijkstra@arm.com>]

Hi,

> It's common to use strncmp as a starts-with predicate, like this:
>
>  strncmp (s, "prefix", 5)
>
> This requires that reading stops at the first null byte.  C11 wording
> makes this kind of usage inadvisable because it treats the inputs as
> arrays, and this means that mean that implementations could read past
> the null byte.  But that doesn't match current programming practice.

C11 says you can't read past the end of the array, but it doesn't say that
you *must* read past a NUL-byte. My interpretation is that this is a valid
strncmp implementation:

int
simple_strncmp (const char *s1, const char *s2, size_t size)
{
   size_t len1 = strnlen (s1, size);
   size_t len2 = strnlen (s2, size);
   if (len1 < len2)
     len1++;
   else if (len1 > len2)
     len1 = len2 + 1;
   return memcmp (s1, s2, len1);
}

Ie. both strings must be valid up until the given size or NUL-terminated if
smaller. This works on the example above even if the first string is only 1 byte.

> The strnlen function has the same problem if you want to use it to limit
> readhead, e.g. in sscanf to fix bug 17577.  (POSIX also speaks of an
> array argument.)  In stdio-common/Xprintf_buffer_puts_1.c, we already
> use it to avoid a similar performance glitch.  It's not the first such
> uses, there is already a similar call (with similar rationale) in
> string/strcasestr.c, and for wcsnlen in
> stdio-common/vfprintf-process-arg.c.
>
> I think we should support all these as extensions.  The alternative
> would be to add new functions that stop reading after the first null
> byte (particularly for the strnlen optimization).

Existing functions already do stop after the first NUL byte. Even if the C
standard doesn't explicitly disallow it, it doesn't seem valid to read beyond
it (a lot of code could fail if we did).

Cheers,
Wilco