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 

fixing more format truncation issues

[Peter Eisentraut]

In 6275f5d28a1577563f53f2171689d4f890a46881, we fixed warnings from the
options -Wformat-overflow and -Wformat-truncation, which are part of
-Wall in gcc 7.

Here, I propose to dial this up a bit by adding -Wformat-overflow=2
-Wformat-truncation=2, which use some more worst-case approaches for
estimating the buffer sizes.

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.

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.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 99316e6db5a98f5daaaf61a75a01b88e7b2f39bf Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 28 Feb 2018 23:11:24 -0500
Subject: [PATCH] Fix more format truncation issues

Add compiler warning options -Wformat-overflow=2 -Wformat-truncation=2,
supported since GCC 7, and fix the warnings that they create.

The issues are all harmless, but some dubious coding patterns are
cleaned up.
---
 configure| 71 
 configure.in |  3 ++
 contrib/pgstattuple/pgstattuple.c|  2 +-
 src/backend/commands/explain.c   |  5 ++-
 src/backend/utils/adt/dbsize.c   |  2 +-
 src/backend/utils/adt/float.c| 24 +--
 src/backend/utils/adt/formatting.c   | 33 +--
 src/backend/utils/misc/guc.c |  4 +-
 src/bin/initdb/initdb.c  |  6 +--
 src/bin/pg_dump/pg_backup_archiver.c |  2 +-
 src/bin/pg_dump/pg_backup_tar.c  |  2 +-
 src/bin/pgbench/pgbench.c|  4 +-
 src/include/postmaster/bgworker.h|  2 +-
 src/interfaces/libpq/fe-secure-openssl.c |  2 +-
 src/pl/tcl/pltcl.c   |  2 +-
 15 files changed, 111 insertions(+), 53 deletions(-)

diff --git a/configure b/configure
index 7dcca506f8..48a7546513 100755
--- a/configure
+++ b/configure
@@ -4595,6 +4595,77 @@ if test x"$pgac_cv_prog_cc_cflags__Wformat_security" = 
x"yes"; then
   CFLAGS="$CFLAGS -Wformat-security"
 fi
 
+  # gcc 7+
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CC supports 
-Wformat-overflow=2" >&5
+$as_echo_n "checking whether $CC supports -Wformat-overflow=2... " >&6; }
+if ${pgac_cv_prog_cc_cflags__Wformat_overflow_2+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+CFLAGS="$pgac_save_CFLAGS -Wformat-overflow=2"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_cc_cflags__Wformat_overflow_2=yes
+else
+  pgac_cv_prog_cc_cflags__Wformat_overflow_2=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: 
$pgac_cv_prog_cc_cflags__Wformat_overflow_2" >&5
+$as_echo "$pgac_cv_prog_cc_cflags__Wformat_overflow_2" >&6; }
+if test x"$pgac_cv_prog_cc_cflags__Wformat_overflow_2" = x"yes"; then
+  CFLAGS="$CFLAGS -Wformat-overflow=2"
+fi
+
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CC supports 
-Wformat-truncation=2" >&5
+$as_echo_n "checking whether $CC supports -Wformat-truncation=2... " >&6; }
+if ${pgac_cv_prog_cc_cflags__Wformat_truncation_2+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+CFLAGS="$pgac_save_CFLAGS -Wformat-truncation=2"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_cc_cflags__Wformat_truncation_2=yes
+else
+  pgac_cv_prog_cc_cflags__Wformat_truncation_2=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: 
$pgac_cv_prog_cc_cflags__Wformat_truncation_2" >&5
+$as_echo "$pgac_cv_prog_cc_cflags__Wformat_truncation_2" >&6; }
+if test x"$pgac_cv_prog_cc_cflags__Wformat_truncation_2" = x"yes"; then
+  CFLAGS="$CFLAGS -Wformat-truncation=2"
+fi
+
   # Disable strict-aliasing rules; needed for gcc 3.3+
   { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CC supports