Re: fixing more format truncation issues

[Peter Eisentraut]

On 3/15/18 12:12, Tom Lane wrote:
> FWIW, I noticed while fooling with pre-release Fedora 28 that gcc 8.0.1
> emits a whole boatload of these warnings by default.

I have some patches for that.  I will send them in once gcc 8 is a bit
closer to release.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: fixing more format truncation issues

[Michael Paquier]

On Thu, Mar 15, 2018 at 12:12:18PM -0400, Tom Lane wrote:
> FWIW, I noticed while fooling with pre-release Fedora 28 that gcc 8.0.1
> emits a whole boatload of these warnings by default.  This patch doesn't
> seem to have moved the needle very much, either.  In a quick look, it
> seemed like a lot of them were things we simply wouldn't be interested
> in fixing ("yeah, the path length limit is MAXPGPATH, tough").  So
> I'm thinking we're going to be needing -Wno-format-truncation soon.

Maybe it is worth a try on Debian as well.  The base packages use GCC 7
but I can see that 8 is also available.
--
Michael


signature.asc
Description: PGP signature

Re: fixing more format truncation issues

[Tom Lane]

Peter Eisentraut  writes:
> On 3/14/18 02:52, Michael Paquier wrote:
>> Enabling them by default would generate some useless noise if the patch
>> is let as-is as a couple of them are not addressed.  Please see the full
>> report attached.  Is that intentional?  I am using GCC 7.3 here.

> Well that's weird.  Apparently, the warnings depend on a bit on build
> options and other platform circumstances.  That seems a bit cumbersome.
> So I just committed the code changes but didn't actually add the
> compiler options to configure.in.  I'll close this patch now; it's not
> worth going further right now.

FWIW, I noticed while fooling with pre-release Fedora 28 that gcc 8.0.1
emits a whole boatload of these warnings by default.  This patch doesn't
seem to have moved the needle very much, either.  In a quick look, it
seemed like a lot of them were things we simply wouldn't be interested
in fixing ("yeah, the path length limit is MAXPGPATH, tough").  So
I'm thinking we're going to be needing -Wno-format-truncation soon.

regards, tom lane

Re: fixing more format truncation issues

[Peter Eisentraut]

On 3/14/18 02:52, Michael Paquier wrote:
> Enabling them by default would generate some useless noise if the patch
> is let as-is as a couple of them are not addressed.  Please see the full
> report attached.  Is that intentional?  I am using GCC 7.3 here.

Well that's weird.  Apparently, the warnings depend on a bit on build
options and other platform circumstances.  That seems a bit cumbersome.
So I just committed the code changes but didn't actually add the
compiler options to configure.in.  I'll close this patch now; it's not
worth going further right now.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: fixing more format truncation issues

[Michael Paquier]

On Wed, Feb 28, 2018 at 11:14:23PM -0500, Peter Eisentraut wrote:
> AFAICT, the issues addressed here either can't really happen without
> trying very hard, or would cause harmless output truncation.  Still, it
> seems good to clean this up properly and not rely on made-up buffer size
> guesses that turn out to be wrong, even if we don't want to adopt the
> warning options by default.

Good idea.

> One issue that is of external interest is that I increase BGW_MAXLEN
> from 64 to 96.  Apparently, the old value would cause the bgw_name of
> logical replication workers to be truncated in some circumstances.  I
> have also seen truncated background worker names with third-party
> packages, so giving some more room here would be useful.

OK, no complains about that.

@@ -89,7 +89,7 @@ static Datum
 build_pgstattuple_type(pgstattuple_type *stat, FunctionCallInfo fcinfo)
 {
 #define NCOLUMNS   9
-#define NCHARS 32
+#define NCHARS 314

So this one is caused by the output of %.2f...

Enabling them by default would generate some useless noise if the patch
is let as-is as a couple of them are not addressed.  Please see the full
report attached.  Is that intentional?  I am using GCC 7.3 here.

interval.c: In function ‘AppendSeconds’:
interval.c:759:22: warning: ‘%0*d’ directive output between 1 and
2147483648 bytes may exceed minimum required size of 4095
[-Wformat-overflow=]
sprintf(cp, "%02d.%0*d", abs(sec), precision, (int) Abs(fsec));

pg_rusage.c:64:5: note: in expansion of macro ‘_’
 _("CPU: user: %d.%02d s, system: %d.%02d s, elapsed: %d.%02d s"),
  ^
pg_rusage.c:63:2: note: ‘snprintf’ output between 51 and 108
 bytes into a destination of size 100
   snprintf(result, sizeof(result),
--
Michael
:0:0: note: this is the location of the previous definition
In file included from ../../../src/include/postgres.h:46:0,
 from be-secure-openssl.c:17:
be-secure-openssl.c: In function ‘SSLerrmessage’:
../../../src/include/c.h:1009:20: warning: ‘%lu’ directive output may be 
truncated writing between 1 and 20 bytes into a region of size 17 
[-Wformat-truncation=]
 #define gettext(x) (x)
^
../../../src/include/c.h:1015:14: note: in expansion of macro ‘gettext’
 #define _(x) gettext(x)
  ^~~
be-secure-openssl.c:1023:35: note: in expansion of macro ‘_’
  snprintf(errbuf, sizeof(errbuf), _("SSL error code %lu"), ecode);
   ^
be-secure-openssl.c:1023:2: note: ‘snprintf’ output between 17 and 36 bytes 
into a destination of size 32
  snprintf(errbuf, sizeof(errbuf), _("SSL error code %lu"), ecode);
  ^~~~
postgres.c: In function ‘check_log_duration’:
postgres.c:2156:36: warning: ‘snprintf’ output may be truncated before the last 
format character [-Wformat-truncation=]
snprintf(msec_str, 32, "%ld.%03d",
^
postgres.c:2156:4: note: ‘snprintf’ output between 6 and 33 bytes into a 
destination of size 32
snprintf(msec_str, 32, "%ld.%03d",
^~
   secs * 1000 + msecs, usecs % 1000);
   ~~
formatting.c: In function ‘DCH_to_char’:
formatting.c:2455:17: warning: ‘%0*d’ directive output between 1 and 2147483648 
bytes may exceed minimum required size of 4095 [-Wformat-overflow=]
 sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : (tm->tm_hour >= 0) ? 2 : 3,
 ^~~~
formatting.c:2455:16: note: assuming directive output of 11 bytes
 sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : (tm->tm_hour >= 0) ? 2 : 3,
^~
formatting.c:2463:17: warning: ‘%0*d’ directive output between 1 and 2147483648 
bytes may exceed minimum required size of 4095 [-Wformat-overflow=]
 sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : (tm->tm_hour >= 0) ? 2 : 3,
 ^~~~
formatting.c:2463:16: note: assuming directive output of 11 bytes
 sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : (tm->tm_hour >= 0) ? 2 : 3,
^~
formatting.c:2470:17: warning: ‘%0*d’ directive output between 1 and 2147483648 
bytes may exceed minimum required size of 4095 [-Wformat-overflow=]
 sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : (tm->tm_min >= 0) ? 2 : 3,
 ^~~~
formatting.c:2470:16: note: assuming directive output of 11 bytes
 sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : (tm->tm_min >= 0) ? 2 : 3,
^~
formatting.c:2477:17: warning: ‘%0*d’ directive output between 1 and 2147483648 
bytes may exceed minimum required size of 4095 [-Wformat-overflow=]
 sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : (tm->tm_sec >= 0) ? 2 : 3,
 ^~~~
formatting.c:2477:16: note: assuming directive output of 11 bytes
 sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : (tm->tm_sec >= 0) ? 2 : 3,
^~
formatting.c:2538:19: warning: ‘%0*d’ directive output between 1 and 2147483648 
bytes may exceed minimum