Thread: [HACKERS] GCC 7 warnings
The release of GCC 7 is approaching [0], and the number of warnings in PostgreSQL has gone up since we last looked [1]. Output attached. (My version is 7.0.1 20170408.) Most of the issues have to do with concatenating two or more strings of potential size MAXPGPATH into another buffer of size MAXPGPATH, which could lead to truncation. Possible fixes: a) Ignore, hoping GCC will change before final release. (unlikely at this point) b) Add compiler option to disable this particular warning, worry about it later. (Might be an option for backpatching.) c) Expand the target buffer sizes until the warning goes away. (Sample patch attached.) d) Replace most of the problematic code with psprintf() and dynamically sized buffers. Comments? [0]: https://gcc.gnu.org/ml/gcc/2017-03/msg00066.html [1]: https://www.postgresql.org/message-id/CAFj8pRA=xV0_-aDF5UtGVY8HGvg+ovA_js_P8z4jLHxvX0Wa=A@mail.gmail.com -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Hi Peter, > c) Expand the target buffer sizes until the warning goes away. (Sample > patch attached.) I personally think it's a great patch. Unfortunately I don't have GCC 7.0 right now but at least it doesn't break anything on 6.3.1. Since there is no rush I would suggest to add an entry to the next commitfest, so this patch wouldn't accidentally be lost. As a side node I believe it would be great to replace all sprintf calls to snprintf just to be on a safe side. IIRC we did something similar to str* procedures not a long time ago. Unless there are any objections regarding this idea I'm willing to write such a patch. -- Best regards, Aleksander Alekseev
Hi, On 2017-04-10 11:03:23 -0400, Peter Eisentraut wrote: > The release of GCC 7 is approaching [0], and the number of warnings in > PostgreSQL has gone up since we last looked [1]. Output attached. (My > version is 7.0.1 20170408.) > > Most of the issues have to do with concatenating two or more strings of > potential size MAXPGPATH into another buffer of size MAXPGPATH, which > could lead to truncation. Hm, interesting - I don't see this with gcc-7 (Debian 7-20170407-1) 7.0.1 20170407 (experimental) [trunk revision 246759] although I do recall having seen them at some point. Not sure what's up there. I've to add -Wno-implicit-fallthrough to avoid noisyness, though. Regards, Andres
On 2017-04-10 09:10:07 -0700, Andres Freund wrote: > Hi, > > On 2017-04-10 11:03:23 -0400, Peter Eisentraut wrote: > > The release of GCC 7 is approaching [0], and the number of warnings in > > PostgreSQL has gone up since we last looked [1]. Output attached. (My > > version is 7.0.1 20170408.) > > > > Most of the issues have to do with concatenating two or more strings of > > potential size MAXPGPATH into another buffer of size MAXPGPATH, which > > could lead to truncation. > > Hm, interesting - I don't see this with > gcc-7 (Debian 7-20170407-1) 7.0.1 20170407 (experimental) [trunk revision 246759] > although I do recall having seen them at some point. Not sure what's up > there. Hm. That's with -Wformat-truncation=1 (which is what's added by -Wextra afaics), I see a good chunk more with -Wformat-truncation=2 - but that's not enabled by -Wall/extra, so I don't really see a problem right now? - Andres
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Possible fixes:
> a) Ignore, hoping GCC will change before final release. (unlikely at
> this point)
> b) Add compiler option to disable this particular warning, worry about
> it later. (Might be an option for backpatching.)
> c) Expand the target buffer sizes until the warning goes away. (Sample
> patch attached.)
> d) Replace most of the problematic code with psprintf() and dynamically
> sized buffers.
+1 for (c) as you have it. Later we might think about selectively
doing (d), but it seems like more work for probably not much benefit.
regards, tom lane
Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > > d) Replace most of the problematic code with psprintf() and dynamically > > sized buffers. > > +1 for (c) as you have it. Later we might think about selectively > doing (d), but it seems like more work for probably not much benefit. Yeah -- also it's possible some of these code paths must not attempt to palloc() for robustness reasons. I would go for c) only for now, and only try d) for very specific cases where there are no such concerns. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4/11/17 13:57, Alvaro Herrera wrote: > Tom Lane wrote: >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > >>> d) Replace most of the problematic code with psprintf() and dynamically >>> sized buffers. >> >> +1 for (c) as you have it. Later we might think about selectively >> doing (d), but it seems like more work for probably not much benefit. > > Yeah -- also it's possible some of these code paths must not attempt to > palloc() for robustness reasons. I would go for c) only for now, and > only try d) for very specific cases where there are no such concerns. Attached is a more refined patch that I propose for PG10 now. Compared to the previous rushed version, this one uses some more precise arithmetic to size some of the buffers. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Attached is a more refined patch that I propose for PG10 now. Compared
> to the previous rushed version, this one uses some more precise
> arithmetic to size some of the buffers.
Generally +1 for switching the snprintf calls to use sizeof() rather
than repeating the declared sizes of the arrays.
The change in setup_formatted_log_time seems a bit weird:
- char msbuf[8];
+ char msbuf[10];
The associated call is
sprintf(msbuf, ".%03d", (int) (saved_timeval.tv_usec / 1000));
Now a human can see that saved_timeval.tv_usec must be 0..999999, so
that the %d format item must always emit exactly 3 characters, which
means that really 5 bytes would be enough. I wouldn't expect a
compiler to know that, but if it's making a generic assumption about
the worst-case width of %d, shouldn't it conclude that we might need
as many as 13 bytes for the buffer? Why does msbuf[10] satisfy it
if msbuf[8] doesn't?
IOW, if we're going to touch this at all, I'd be inclined to go with
msbuf[16] or so, as being more likely to satisfy compilers that have
decided to try to warn about this. And maybe we should use snprintf,
just for luck.
regards, tom lane
On 4/12/17 00:12, Tom Lane wrote: > The change in setup_formatted_log_time seems a bit weird: > > - char msbuf[8]; > + char msbuf[10]; > > The associated call is > > sprintf(msbuf, ".%03d", (int) (saved_timeval.tv_usec / 1000)); > > Now a human can see that saved_timeval.tv_usec must be 0..999999, so > that the %d format item must always emit exactly 3 characters, which > means that really 5 bytes would be enough. I wouldn't expect a > compiler to know that, but if it's making a generic assumption about > the worst-case width of %d, shouldn't it conclude that we might need > as many as 13 bytes for the buffer? Why does msbuf[10] satisfy it > if msbuf[8] doesn't? Because the /1000 takes off three digits? The full message from an isolated test case is test.c: In function 'f': test.c:11:15: warning: '%03d' directive writing between 3 and 8 bytes into a region of size 7 [-Wformat-overflow=] sprintf(buf, ".%03d", (int) (tv.tv_usec / 1000)); ^ test.c:11:15: note: directive argument in the range [-2147483, 2147483] test.c:11:2: note: '__builtin___sprintf_chk' output between 5 and 10 bytes into a destination of size 8 sprintf(buf, ".%03d", (int) (tv.tv_usec / 1000)); ^ (This is with -O2. With -O0 it only asks for 5 bytes.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 4/12/17 00:12, Tom Lane wrote:
>> Now a human can see that saved_timeval.tv_usec must be 0..999999, so
>> that the %d format item must always emit exactly 3 characters, which
>> means that really 5 bytes would be enough. I wouldn't expect a
>> compiler to know that, but if it's making a generic assumption about
>> the worst-case width of %d, shouldn't it conclude that we might need
>> as many as 13 bytes for the buffer? Why does msbuf[10] satisfy it
>> if msbuf[8] doesn't?
> Because the /1000 takes off three digits?
Huh ... I would not really have expected it to figure that out, but
this makes it pretty clear that it did:
> test.c:11:15: note: directive argument in the range [-2147483, 2147483]
> (This is with -O2. With -O0 it only asks for 5 bytes.)
But that reinforces my suspicion that the intelligence brought to bear
here will be variable. I'd still go for msbuf[16]; it's not like
anybody's going to notice the extra stack consumption.
regards, tom lane
On 4/10/17 11:03, Peter Eisentraut wrote: > The release of GCC 7 is approaching [0], and the number of warnings in > PostgreSQL has gone up since we last looked [1]. Output attached. (My > version is 7.0.1 20170408.) GCC 7 has been released. Should we backpatch these warning fixes? The commit in question is 6275f5d28a1577563f53f2171689d4f890a46881. (I haven't actually checked whether backpatches well.) PG 9.2 and newer compile warning-free with GCC 6, so there would be some value in preserving this with GCC 7. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 4/10/17 11:03, Peter Eisentraut wrote:
>> The release of GCC 7 is approaching [0], and the number of warnings in
>> PostgreSQL has gone up since we last looked [1]. Output attached. (My
>> version is 7.0.1 20170408.)
> GCC 7 has been released.
> Should we backpatch these warning fixes? The commit in question is
> 6275f5d28a1577563f53f2171689d4f890a46881. (I haven't actually checked
> whether backpatches well.)
Seems like that patch represents generally better coding practice,
and applying it would reduce cross-branch differences that would be
hazards for future patches, so I'm mostly +1 for this.
But I'd suggest waiting till after next week's releases. If there
are any problems induced by this, we'd be more likely to find them
with another three months' time before it hits the wild.
regards, tom lane
On 5/4/17 00:21, Tom Lane wrote: > But I'd suggest waiting till after next week's releases. If there > are any problems induced by this, we'd be more likely to find them > with another three months' time before it hits the wild. done -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services