Re: GCC 8 warnings

[Tom Lane]

Andres Freund  writes:
> On 2018-06-16 13:29:55 -0400, Tom Lane wrote:
>> +  # Similarly disable useless truncation warnings from gcc 8+
>> +  PGAC_PROG_CC_VAR_OPT(NOT_THE_CFLAGS, [-Wformat-truncation])
>> +  if test -n "$NOT_THE_CFLAGS"; then
>> +CFLAGS="$CFLAGS -Wno-format-truncation"
>> +  fi
>> +  PGAC_PROG_CC_VAR_OPT(NOT_THE_CFLAGS, [-Wstringop-truncation])
>> +  if test -n "$NOT_THE_CFLAGS"; then
>> +CFLAGS="$CFLAGS -Wno-stringop-truncation"
>> +  fi

> I've not had a lot of coffee yet this morning, but couldn't there be an
> issue where a compiler supported one of these flags? Because
> NOT_THE_CFLAGS is reused, it'd trigger lateron as well, right?  Seems to
> me we'd need to reset it.

Yeah, I found that out as soon as I checked this on another platform ;-).
Will fix.

regards, tom lane

Re: GCC 8 warnings

[Andres Freund]

Hi,

On 2018-06-16 13:29:55 -0400, Tom Lane wrote:
> I propose the attached patch to disable these warnings if the compiler
> knows the switch for them.  I did not turn them off for CXX though;
> anyone think there's a need to?

No, not for now.  I don't think it's likely that the amount of C++ will
grow significantly anytime soon. And it seems unlikely that the llvm
interfacing code will use C stringops.  Not that I think it'd hurt much
to add it.


> Probably all of this ought to be back-patched as applicable, since
> people will doubtless try to compile back branches with gcc 8.

Yea, It's already pretty annoying.


> diff --git a/configure.in b/configure.in
> index 862d8b128d..dae29a4ab1 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -502,6 +502,15 @@ if test "$GCC" = yes -a "$ICC" = no; then
>if test -n "$NOT_THE_CFLAGS"; then
>  CFLAGS="$CFLAGS -Wno-unused-command-line-argument"
>fi
> +  # Similarly disable useless truncation warnings from gcc 8+
> +  PGAC_PROG_CC_VAR_OPT(NOT_THE_CFLAGS, [-Wformat-truncation])
> +  if test -n "$NOT_THE_CFLAGS"; then
> +CFLAGS="$CFLAGS -Wno-format-truncation"
> +  fi
> +  PGAC_PROG_CC_VAR_OPT(NOT_THE_CFLAGS, [-Wstringop-truncation])
> +  if test -n "$NOT_THE_CFLAGS"; then
> +CFLAGS="$CFLAGS -Wno-stringop-truncation"
> +  fi

I've not had a lot of coffee yet this morning, but couldn't there be an
issue where a compiler supported one of these flags? Because
NOT_THE_CFLAGS is reused, it'd trigger lateron as well, right?  Seems to
me we'd need to reset it.

Greetings,

Andres Freund

Re: GCC 8 warnings

[Tom Lane]

Andres Freund  writes:
> On 2018-04-28 12:16:39 -0400, Tom Lane wrote:
>> In the meantime, I think our response to GCC 8 should just be to
>> enable -Wno-format-truncation.  Certainly so in the back branches.
>> There isn't one of these changes that is actually fixing a bug,
>> which to me says that that warning is unhelpful.

> Agreed. Or at least turn down its aggressiveness to the cases where it's
> actually sure truncation happens.

I got around to poking into this today.  Unfortunately, it doesn't seem
that there's any more-conservative level of -Wformat-truncation available.
Likewise for -Wstringop-truncation, which is the other rich new source
of useless warnings (it appears to be predicated on the assumption that
you never actually want the defined semantics of strncpy).  Hence,
I propose the attached patch to disable these warnings if the compiler
knows the switch for them.  I did not turn them off for CXX though;
anyone think there's a need to?

Testing with Fedora 28's gcc (currently 8.1.1), this leaves three new
warnings, which seem worth fixing.  Two of them are gratuitous use of
strncpy where memcpy would serve, in ecpg, and one is an sprintf in
pg_waldump that should be snprintf for safety's sake:

common.c: In function 'pgtypes_fmt_replace':
common.c:48:5: warning: 'strncpy' specified bound depends on the length of the 
source argument [-Wstringop-overflow=]
 strncpy(*output, replace_val.str_val, i + 1);
 ^~~~
common.c:42:8: note: length computed here
i = strlen(replace_val.str_val);
^~~
In function 'get_char_item',
inlined from 'ECPGget_desc' at descriptor.c:334:10:
descriptor.c:221:6: warning: 'strncpy' specified bound depends on the length of 
the source argument [-Wstringop-overflow=]
  strncpy(variable->arr, value, strlen(value));
  ^~~~
compat.c: In function 'timestamptz_to_str':
compat.c:61:19: warning: '%06d' directive writing between 6 and 7 bytes into a 
region of size between 0 and 128 [-Wformat-overflow=]
  sprintf(buf, "%s.%06d %s", ts, (int) (dt % USECS_PER_SEC), zone);
   ^~~~
compat.c:61:15: note: directive argument in the range [-99, 99]
  sprintf(buf, "%s.%06d %s", ts, (int) (dt % USECS_PER_SEC), zone);
   ^~~~
compat.c:61:2: note: 'sprintf' output between 9 and 266 bytes into a 
destination of size 129
  sprintf(buf, "%s.%06d %s", ts, (int) (dt % USECS_PER_SEC), zone);
  ^~~~

Probably all of this ought to be back-patched as applicable, since
people will doubtless try to compile back branches with gcc 8.

regards, tom lane

diff --git a/configure.in b/configure.in
index 862d8b128d..dae29a4ab1 100644
--- a/configure.in
+++ b/configure.in
@@ -502,6 +502,15 @@ if test "$GCC" = yes -a "$ICC" = no; then
   if test -n "$NOT_THE_CFLAGS"; then
 CFLAGS="$CFLAGS -Wno-unused-command-line-argument"
   fi
+  # Similarly disable useless truncation warnings from gcc 8+
+  PGAC_PROG_CC_VAR_OPT(NOT_THE_CFLAGS, [-Wformat-truncation])
+  if test -n "$NOT_THE_CFLAGS"; then
+CFLAGS="$CFLAGS -Wno-format-truncation"
+  fi
+  PGAC_PROG_CC_VAR_OPT(NOT_THE_CFLAGS, [-Wstringop-truncation])
+  if test -n "$NOT_THE_CFLAGS"; then
+CFLAGS="$CFLAGS -Wno-stringop-truncation"
+  fi
 elif test "$ICC" = yes; then
   # Intel's compiler has a bug/misoptimization in checking for
   # division by NAN (NaN == 0), -mp1 fixes it, so add it to the CFLAGS.

Re: GCC 8 warnings

[Andres Freund]

Hi,

On 2018-04-28 12:16:39 -0400, Tom Lane wrote:
> -SharedFilePath(char *path, SharedFileSet *fileset, const char *name)
> +SharedFilePath(char *path, size_t size, SharedFileSet *fileset, const char 
> *name)
>  {
> - chardirpath[MAXPGPATH];
> + chardirpath[MAXPGPATH + 100];
>  
> - SharedFileSetPath(dirpath, fileset, ChooseTablespace(fileset, name));
> - snprintf(path, MAXPGPATH, "%s/%s", dirpath, name);
> + SharedFileSetPath(dirpath, sizeof(dirpath), fileset, 
> ChooseTablespace(fileset, name));
> + snprintf(path, size, "%s/%s", dirpath, name);
>  }
> 
> we basically haven't got any coherent strategy at all, other than
> "whack it till GCC 8 stops complaining".  Some of these strings
> might be longer than MAXPGPATH, and we're not very clear on which.
> Worse, the recursive rule that paths are shorter than MAXPGPATH has
> collapsed entirely, so that any reasoning on the assumption that the
> input strings are shorter than MAXPGPATH is now suspect as can be.

> So basically, I think that any argument that these changes make us
> more secure against unwanted path truncation is just horsepucky.
> There's no longer any clear understanding of the maximum supported
> string length, and without global analysis of that you can't say
> that truncation will never happen.

+1


> Perhaps there's an argument for trying to get rid of MAXPGPATH
> altogether, replacing *all* of these fixed-length buffers with
> psprintf-type coding.  I kinda doubt it's worth the trouble,
> but if somebody wants to work on it I wouldn't say no.

Same.


> In the meantime, I think our response to GCC 8 should just be to
> enable -Wno-format-truncation.  Certainly so in the back branches.
> There isn't one of these changes that is actually fixing a bug,
> which to me says that that warning is unhelpful.

Agreed. Or at least turn down its aggressiveness to the cases where it's
actually sure truncation happens.

Greetings,

Andres Freund

Re: GCC 8 warnings

[Tom Lane]

Peter Eisentraut  writes:
> On 4/24/18 05:57, Devrim Gündüz wrote:
>> While building stable releases and v11 on Fedora 28, I am seeing some 
>> warnings.

> Attached is a patch to fix these warnings in master.  These are very
> similar to the class of warnings we fixed last time around for GCC 7.

I took a look through this, and frankly this is seeming to me like mostly
pedantry, with zero benefit for readability and possibly actually negative
impact for reliability.

Here's what's bothering me.  The changes like this one:

-   result = palloc(MAXPGPATH);
-   snprintf(result, MAXPGPATH, "%s/tsearch_data/%s.%s",
-sharepath, basename, extension);
-
-   return result;
+   return psprintf("%s/tsearch_data/%s.%s", sharepath, basename, 
extension);

seem like good coding improvements on their own; most likely, if psprintf
had existed from the beginning, this code would have been written like
that to start with.  But notice what we did here: before, there was a
guarantee that the result was shorter than MAXPGPATH.  Now there isn't.

Up to now, the design principle for all this code has been "we assume
that all path strings are shorter than MAXPGPATH, and we're not going
to waste brain cells reasoning about what happens if they are not".
As long as MAXPGPATH is significantly longer than any path people might
use in practice, I think that's a defensible design strategy.  However,
after patches like this one:

-SharedFilePath(char *path, SharedFileSet *fileset, const char *name)
+SharedFilePath(char *path, size_t size, SharedFileSet *fileset, const char 
*name)
 {
-   chardirpath[MAXPGPATH];
+   chardirpath[MAXPGPATH + 100];
 
-   SharedFileSetPath(dirpath, fileset, ChooseTablespace(fileset, name));
-   snprintf(path, MAXPGPATH, "%s/%s", dirpath, name);
+   SharedFileSetPath(dirpath, sizeof(dirpath), fileset, 
ChooseTablespace(fileset, name));
+   snprintf(path, size, "%s/%s", dirpath, name);
 }

we basically haven't got any coherent strategy at all, other than
"whack it till GCC 8 stops complaining".  Some of these strings
might be longer than MAXPGPATH, and we're not very clear on which.
Worse, the recursive rule that paths are shorter than MAXPGPATH has
collapsed entirely, so that any reasoning on the assumption that the
input strings are shorter than MAXPGPATH is now suspect as can be.

So basically, I think that any argument that these changes make us
more secure against unwanted path truncation is just horsepucky.
There's no longer any clear understanding of the maximum supported
string length, and without global analysis of that you can't say
that truncation will never happen.

Perhaps there's an argument for trying to get rid of MAXPGPATH
altogether, replacing *all* of these fixed-length buffers with
psprintf-type coding.  I kinda doubt it's worth the trouble,
but if somebody wants to work on it I wouldn't say no.

In the meantime, I think our response to GCC 8 should just be to
enable -Wno-format-truncation.  Certainly so in the back branches.
There isn't one of these changes that is actually fixing a bug,
which to me says that that warning is unhelpful.

regards, tom lane

Re: GCC 8 warnings

[Peter Eisentraut]

On 4/24/18 05:57, Devrim Gündüz wrote:
> While building stable releases and v11 on Fedora 28, I am seeing some 
> warnings.

Attached is a patch to fix these warnings in master.  These are very
similar to the class of warnings we fixed last time around for GCC 7.

GCC 8 is now frozen, so it would be a good time to move ahead with this.

> What is the project's policy about fixing those warnings in older branches? 

We did backpatch the GCC 7 changes.  We could do the same here, but it's
a pretty sizeable number of changes.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From ce6142824b745d83511c73efe88c0e2d0ad0522b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 9 Apr 2018 14:54:41 +
Subject: [PATCH] Fix new warnings from GCC 8

---
 contrib/pg_standby/pg_standby.c   |  4 +--
 src/backend/access/transam/xlog.c |  8 ++---
 src/backend/commands/extension.c  | 32 ++-
 src/backend/postmaster/postmaster.c   |  9 --
 src/backend/storage/file/fd.c |  4 +--
 src/backend/storage/file/reinit.c |  8 ++---
 src/backend/storage/file/sharedfileset.c  | 32 +--
 src/backend/tsearch/ts_utils.c|  7 +---
 src/backend/utils/misc/guc.c  |  2 +-
 src/backend/utils/misc/tzparser.c |  2 +-
 src/backend/utils/time/snapmgr.c  |  2 +-
 src/bin/initdb/findtimezone.c |  2 +-
 src/bin/initdb/initdb.c   |  4 +--
 src/bin/pg_basebackup/pg_basebackup.c |  2 +-
 src/bin/pg_basebackup/receivelog.c| 18 +++
 src/bin/pg_dump/pg_backup_directory.c |  4 +--
 .../pg_verify_checksums/pg_verify_checksums.c |  2 +-
 src/bin/pg_waldump/compat.c   |  4 +--
 src/bin/pgbench/pgbench.c |  6 ++--
 src/bin/psql/startup.c|  6 ++--
 src/interfaces/ecpg/preproc/ecpg.c|  4 +--
 src/interfaces/libpq/fe-connect.c |  8 ++---
 src/interfaces/libpq/fe-secure-openssl.c  |  2 +-
 src/test/regress/pg_regress.c | 14 
 src/timezone/pgtz.c   |  2 +-
 25 files changed, 89 insertions(+), 99 deletions(-)

diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index cb785971a9..fcdf4e6a3f 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -60,7 +60,7 @@ char *nextWALFileName;/* the file we need to get from 
archive */
 char  *restartWALFileName; /* the file from which we can restart restore */
 char  *priorWALFileName;   /* the file we need to get from archive */
 char   WALFilePath[MAXPGPATH * 2]; /* the file path including archive 
*/
-char   restoreCommand[MAXPGPATH];  /* run this to restore */
+char  *restoreCommand; /* run this to restore */
 char   exclusiveCleanupFileName[MAXFNAMELEN];  /* the file we need to 
get

 * from archive */
 
@@ -98,7 +98,7 @@ int   restoreCommandType;
 intnextWALFileType;
 
 #define SET_RESTORE_COMMAND(cmd, arg1, arg2) \
-   snprintf(restoreCommand, MAXPGPATH, cmd " \"%s\" \"%s\"", arg1, arg2)
+   restoreCommand = psprintf(cmd " \"%s\" \"%s\"", arg1, arg2)
 
 struct stat stat_buf;
 
diff --git a/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index c0923d97f2..d29459e8d7 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7702,12 +7702,12 @@ StartupXLOG(void)
if (!XLogArchiveIsReadyOrDone(origfname))
{
charorigpath[MAXPGPATH];
-   charpartialfname[MAXFNAMELEN];
-   charpartialpath[MAXPGPATH];
+   charpartialfname[MAXFNAMELEN + 8];
+   charpartialpath[MAXPGPATH + 8];
 
XLogFilePath(origpath, EndOfLogTLI, 
endLogSegNo, wal_segment_size);
-   snprintf(partialfname, MAXFNAMELEN, 
"%s.partial", origfname);
-   snprintf(partialpath, MAXPGPATH, "%s.partial", 
origpath);
+   snprintf(partialfname, sizeof(partialfname), 
"%s.partial", origfname);
+   snprintf(partialpath, sizeof(partialpath), 
"%s.partial", origpath);
 
/*
 * Make sure there's no .done or .ready file 
for the .partial
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index