[prev in list] [next in list] [prev in thread] [next in thread] 

List:       glibc-alpha
Subject:    Re: -Wformat-overflow error building glibc sysdeps/posix/tempname.c
From:       Martin Sebor <msebor () gmail ! com>
Date:       2017-01-27 23:49:46
Message-ID: b7f5b353-cd49-f43b-75a4-61a8ffe26b79 () gmail ! com
[Download RAW message or body]

On 01/27/2017 02:02 PM, Joseph Myers wrote:
> Some recent GCC change (after r244906, no later than r244960) resulted in
> glibc's sysdeps/posix/tempname.c failing to build with an error (for
> 32-bit systems only, not 64-bit):
> 
> ../sysdeps/posix/tempname.c: In function '__path_search':
> ../sysdeps/posix/tempname.c:169:24: error: '%.*s' directive output between 0 and 5 \
> bytes may cause result to exceed 'INT_MAX' [-Werror=format-overflow=] sprintf \
> (tmpl, "%.*s/%.*sXXXXXX", (int) dlen, dir, (int) plen, pfx); ^~~~
> 
> I don't see how such a warning can be useful.  The code is
> 
> /* check we have room for "${dir}/${pfx}XXXXXX\0" */
> if (tmpl_len < dlen + 1 + plen + 6 + 1)
> {
> __set_errno (EINVAL);
> return -1;
> }
> 
> sprintf (tmpl, "%.*s/%.*sXXXXXX", (int) dlen, dir, (int) plen, pfx);
> 
> where there is in fact a check that the length is sufficient.  GCC has no
> way of knowing that tmpl_len is the length of the buffer tmpl (and even if
> it did, the bound is rather complicated), but surely in such a case where
> the bounds are unknown, warning is unhelpful, especially when the format
> uses %.*s not %s so is clearly intending to limit output length?

Right.  That's also the logic behind the warning: precision is used
to constrain the output so the warning checks to make sure that even
for a string of unknown length the bound makes sense.  I.e., it uses
the precision in lieu of the likely length of the string.  It does
that both when the precision is constant and when it's in some (known)
range, and it uses the upper bound of the range.  The trouble in this
case is that the upper bound is excessive.  Maybe at level 1 it should
use the lower bound instead.  Let me look into it.

> There are theoretical issues that (int) dlen could result in a negative
> value from TMPDIR longer than INT_MAX bytes (not an issue on 32-bit
> systems where the warning appears and I don't think the Linux kernel
> allows such a large environment) and that the number of bytes written
> might not be representable in the return value of sprintf, but I don't
> think that sort of issue is useful to warn for at -Wall.

Those consideration don't come into play here.

Martin


[prev in list] [next in list] [prev in thread] [next in thread]