Re: GCC 8 warnings - Mailing list pgsql-hackers

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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)
 {
-    char        dirpath[MAXPGPATH];
+    char        dirpath[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