Re: [HACKERS] GCC 7 warnings
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GCC 7 warnings
Peter Eisentraut 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GCC 7 warnings
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GCC 7 warnings
Peter Eisentraut writes: > On 4/12/17 00:12, Tom Lane wrote: >> Now a human can see that saved_timeval.tv_usec must be 0..99, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GCC 7 warnings
On 4/12/17 00:12, Tom Lane wrote: > The change in setup_formatted_log_time seems a bit weird: > > - charmsbuf[8]; > + charmsbuf[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..99, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GCC 7 warnings
Peter Eisentraut 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: - charmsbuf[8]; + charmsbuf[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..99, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GCC 7 warnings
On 4/11/17 13:57, Alvaro Herrera wrote: > Tom Lane wrote: >> Peter Eisentraut 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 v2-0001-Fix-new-warnings-from-GCC-7.patch Description: invalid/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GCC 7 warnings
Tom Lane wrote: > Peter Eisentraut 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 Herrerahttps://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
Re: [HACKERS] GCC 7 warnings
Peter Eisentraut 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GCC 7 warnings
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GCC 7 warnings
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GCC 7 warnings
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 signature.asc Description: PGP signature
[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 In file included from /usr/include/stdio.h:498:0, from ../../src/include/c.h:81, from ../../src/include/postgres_fe.h:25, from file_utils.c:15: file_utils.c: In function 'walkdir.constprop': file_utils.c:177:32: warning: '__builtin_snprintf' output may be truncated before the last format character [-Wformat-truncation=] snprintf(subpath, MAXPGPATH, "%s/%s", path, de->d_name); ^ file_utils.c:177:3: note: '__builtin_snprintf' output 2 or more bytes (assuming 1025) into a destination of size 1024 snprintf(subpath, MAXPGPATH, "%s/%s", path, de->d_name); ^ In file included from /usr/include/stdio.h:498:0, from ../../src/include/c.h:81, from ../../src/include/postgres.h:47, from pgtz.c:13: pgtz.c: In function 'pg_tzenumerate_next': pgtz.c:432:33: warning: '__builtin_snprintf' output may be truncated before the last format character [-Wformat-truncation=] snprintf(fullname, MAXPGPATH, "%s/%s", ^ pgtz.c:432:3: note: '__builtin_snprintf' output 2 or more bytes (assuming 1025) into a destination of size 1024 snprintf(fullname, MAXPGPATH, "%s/%s", ^ In file included from /usr/include/stdio.h:498:0, from ../../../../src/include/c.h:81, from ../../../../src/include/postgres.h:47, from rewriteheap.c:103: rewriteheap.c: In function 'CheckPointLogicalRewriteHeap': rewriteheap.c:1237:29: warning: '%s' directive output may be truncated writing up to 1023 bytes into a region of size 1004 [-Wformat-truncation=] snprintf(path, MAXPGPATH, "pg_logical/mappings/%s", mapping_de->d_name); ^ rewriteheap.c:1237:3: note: '__builtin_snprintf' output between 21 and 1044 bytes into a destination of size 1024 snprintf(path, MAXPGPATH, "pg_logical/mappings/%s", mapping_de->d_name); ^ In file included from /usr/include/stdio.h:498:0, from ../../../../src/include/c.h:81, from ../../../../src/include/postgres.h:47, from xlog.c:15: xlog.c: In function 'do_pg_start_backup': xlog.c:10403:41: warning: '%s' directive output may be truncated writing up to 1023 bytes into a region of size 1014 [-Wformat-truncation=] snprintf(fullpath, sizeof(fullpath), "pg_tblspc/%s", de->d_name); ^ xlog.c:10403:4: note: '__builtin_snprintf' output between 11 and 1034 bytes into a destination of size 1024 snprintf(fullpath, sizeof(fullpath), "pg_tblspc/%s", de->d_name); ^ xlog.c: In function 'do_pg_stop_backup': ../../../../src/include/access/xlog_internal.h:131:20: warning: '%s' directive output may be truncated writing up to 1023 bytes into a region of size 1017 [-Wformat-truncation=] #define XLOGDIR"pg_wal" ^ xlog.c:4123:31: note: in expansion of macro 'XLOGDIR' snprintf(path, MAXPGPATH, XLOGDIR "/%s", xlde->d_name); ^~~ In file included from xlog.c:35:0: ../../../../src/include/access/xlog_internal.h:203:38: note: format string is defined here snprintf(path, MAXPGPATH, XLOGDIR "/%08X%08X%08X.%08X.backup", tli, \ ^~ In file included from /usr/include/stdio.h:498:0, from ../../../../src/include/c.h:81, from ../../../../src/include/postgres.h:47, from xlog.c:15: xlog.c:4123:5: note: '__builtin_snprintf' output between 8 and 1031 bytes into a destination of size 1024 snprintf(path, MAXPGPATH, XLOGDIR "/%s", xlde->d_name); ^ In file included from /usr/include/stdio.h:498:0, from ../../../src/include/c.h:81, from ../../../src/include/postgres.h:47, from pgstat.c:19: pgstat.c: In function 'pgstat_reset_remove_files': pgstat.c:639:30: warning: