Horton

Thread

Commits

  1. Use -Wno-format-truncation and -Wno-stringop-truncation, if available.

  2. Avoid unnecessary use of strncpy in a couple of places in ecpg.

  3. Use snprintf not sprintf in pg_waldump's timestamptz_to_str.

  1. GCC 8 warnings

    Devrim GÜNDÜZ <devrim@gunduz.org> — 2018-04-24T09:57:36Z

    Hi,

While building stable releases and v11 on Fedora 28, I am seeing some warnings.
What is the project's policy about fixing those warnings in older branches? 

To contribute to world peace, I did not attach the text to the email. Here is
what I see in today's git snapshot:

https://gunduz.org/temp/pg-gcc8-v11.txt

...and this is from 9.6:

https://gunduz.org/temp/pg-gcc8-9.6.txt

Regards,
-- 
Devrim Gündüz
EnterpriseDB: https://www.enterprisedb.com
PostgreSQL Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

  2. Re: GCC 8 warnings

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-04-27T16:52:37Z

    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

  3. Re: GCC 8 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-28T16:16:39Z

    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

  4. Re: GCC 8 warnings

    Andres Freund <andres@anarazel.de> — 2018-04-28T16:41:35Z

    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)
>  {
> -	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.

+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

  5. Re: GCC 8 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-06-16T17:29:55Z

    Andres Freund <andres@anarazel.de> 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 [-999999, 999999]
  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

  6. Re: GCC 8 warnings

    Andres Freund <andres@anarazel.de> — 2018-06-16T18:52:18Z

    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

  7. Re: GCC 8 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-06-16T19:00:11Z

    Andres Freund <andres@anarazel.de> 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