meson: Specify -Wformat as a common warning flag for extensions

Hi,

Could someone take a look at this?

Patch is attached in the original e-mail:
/messages/by-id/20240122.141139.931086145628347157.kou@clear-code.com

Thanks,
--
kou

In <20240122.141139.931086145628347157.kou@clear-code.com>
"meson: Specify -Wformat as a common warning flag for extensions" on Mon, 22 Jan 2024 14:11:39 +0900 (JST),
Sutou Kouhei <kou@clear-code.com> wrote:

On Sun Jan 21, 2024 at 11:11 PM CST, Sutou Kouhei wrote:

Hi,

I'm an extension developer. If I use PostgreSQL built with
Meson, I get the following warning:

cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]

Because "pg_config --cflags" includes -Wformat-security but
doesn't include -Wformat.

Can we specify -Wformat as a common warning flag too? If we
do it, "pg_config --cflags" includes both of
-Wformat-security and -Wformat. So I don't get the warning.

The GCC documentation[0]https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html says the following:

If -Wformat is specified, also warn about uses of format functions
that represent possible security problems. At present, this warns
about calls to printf and scanf functions where the format string is
not a string literal and there are no format arguments, as in printf
(foo);. This may be a security hole if the format string came from
untrusted input and contains ‘%n’. (This is currently a subset of what
-Wformat-nonliteral warns about, but in future warnings may be added
to -Wformat-security that are not included in -Wformat-nonliteral.)

It sounds like a legitimate issue. I have confirmed the issue exists
with a pg_config compiled with Meson. I can also confirm that this issue
exists in the autotools build.

Here is a v2 of your patch which includes the fix for autotools. I will
mark this "Ready for Committer" in the commitfest. Thanks!

[0]: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

--
Tristan Partin
Neon (https://neon.tech)

On Thu, Mar 07, 2024 at 11:39:39PM -0600, Tristan Partin wrote:

It sounds like a legitimate issue. I have confirmed the issue exists with a
pg_config compiled with Meson. I can also confirm that this issue exists in
the autotools build.

First time I'm hearing about that, but I'll admit that I am cheating
because -Wformat is forced in my local builds for some time now. I'm
failing to see the issue with meson and ./configure even if I remove
the switch, though, using a recent version of gcc at 13.2.0, but
perhaps Debian does something underground. Are there version and/or
environment requirements to be aware of?

Forcing -Wformat implies more stuff that can be disabled with
-Wno-format-contains-nul, -Wno-format-extra-args, and
-Wno-format-zero-length, but the thing is that we're usually very
conservative with such additions in the scripts. See also
8b6f5f25102f, done, I guess, as an answer to this thread:
/messages/by-id/4D431505.9010002@dunslane.net

A quick look at the past history of pgsql-hackers does not mention
that as a problem, either, but I may have missed something.
--
Michael

Hi,

In <Zeqw9vGrYlb250aO@paquier.xyz>
"Re: meson: Specify -Wformat as a common warning flag for extensions" on Fri, 8 Mar 2024 15:32:22 +0900,
Michael Paquier <michael@paquier.xyz> wrote:

Are there version and/or
environment requirements to be aware of?

I'm using Debian GNU/Linux sid and I can reproduce with gcc
8-13:

$ for x in {8..13}; do; echo gcc-${x}; gcc-${x} -Wformat-security -E - < /dev/null > /dev/null; done
gcc-8
cc1: warning: -Wformat-security ignored without -Wformat [-Wformat-security]
gcc-9
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
gcc-10
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
gcc-11
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
gcc-12
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
gcc-13
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
$

I tried this on Ubuntu 22.04 too but this isn't reproduced:

$ gcc-11 -Wformat-security -E - < /dev/null > /dev/null
$

It seems that Ubuntu enables -Wformat by default:

$ gcc-11 -Wno-format -Wformat-security -E - < /dev/null > /dev/null
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]

I tried this on AlmaLinux 9 too and this is reproduced:

$ gcc --version
gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2)
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ gcc -Wformat-security -E - < /dev/null > /dev/null
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]

Forcing -Wformat implies more stuff that can be disabled with
-Wno-format-contains-nul, -Wno-format-extra-args, and
-Wno-format-zero-length, but the thing is that we're usually very
conservative with such additions in the scripts. See also
8b6f5f25102f, done, I guess, as an answer to this thread:
/messages/by-id/4D431505.9010002@dunslane.net

I think that this is not a problem. Because the comment
added by 8b6f5f25102f ("This was included in -Wall/-Wformat
in older GCC versions") implies that we want to always use
-Wformat-security. -Wformat-security isn't worked without
-Wformat:

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-security

If -Wformat is specified, also warn about uses of format
functions that represent possible security problems.

Thanks,
--
kou

On Fri Mar 8, 2024 at 12:32 AM CST, Michael Paquier wrote:

On Thu, Mar 07, 2024 at 11:39:39PM -0600, Tristan Partin wrote:

It sounds like a legitimate issue. I have confirmed the issue exists with a
pg_config compiled with Meson. I can also confirm that this issue exists in
the autotools build.

First time I'm hearing about that, but I'll admit that I am cheating
because -Wformat is forced in my local builds for some time now. I'm
failing to see the issue with meson and ./configure even if I remove
the switch, though, using a recent version of gcc at 13.2.0, but
perhaps Debian does something underground. Are there version and/or
environment requirements to be aware of?

Forcing -Wformat implies more stuff that can be disabled with
-Wno-format-contains-nul, -Wno-format-extra-args, and
-Wno-format-zero-length, but the thing is that we're usually very
conservative with such additions in the scripts. See also
8b6f5f25102f, done, I guess, as an answer to this thread:
/messages/by-id/4D431505.9010002@dunslane.net

A quick look at the past history of pgsql-hackers does not mention
that as a problem, either, but I may have missed something.

Ok, I figured this out. -Wall implies -Wformat=1. We set warning_level
to 1 in the Meson project() call, which implies -Wall, and set -Wall in
CFLAGS for autoconf. That's the reason we don't get issues building
Postgres. A user making use of the pg_config --cflags option, as Sutou
is, *will* run into the aforementioned issues, since we don't propogate
-Wall into pg_config.

$ gcc $(pg_config --cflags) -E - < /dev/null > /dev/null
cc1: warning: ‘-Wformat-security’ ignored without ‘-Wformat’ [-Wformat-security]
$ gcc -Wall $(pg_config --cflags) -E - < /dev/null > /dev/null
(nothing printed)

--
Tristan Partin
Neon (https://neon.tech)

Hi,

In <CZOHWDYQJQCQ.23A5RRV1E05N2@neon.tech>
"Re: meson: Specify -Wformat as a common warning flag for extensions" on Fri, 08 Mar 2024 10:05:27 -0600,
"Tristan Partin" <tristan@neon.tech> wrote:

Ok, I figured this out. -Wall implies -Wformat=1. We set warning_level
to 1 in the Meson project() call, which implies -Wall, and set -Wall
in CFLAGS for autoconf. That's the reason we don't get issues building
Postgres. A user making use of the pg_config --cflags option, as Sutou
is, *will* run into the aforementioned issues, since we don't
propogate -Wall into pg_config.

$ gcc $(pg_config --cflags) -E - < /dev/null > /dev/null
cc1: warning: ‘-Wformat-security’ ignored without ‘-Wformat’
[-Wformat-security]
$ gcc -Wall $(pg_config --cflags) -E - < /dev/null > /dev/null
(nothing printed)

Thanks for explaining this. You're right. This is the reason
why we don't need this for PostgreSQL itself but we need
this for PostgreSQL extensions. Sorry. I should have
explained this in the first e-mail...

What should we do to proceed this patch?

Thanks,
--
kou

On Tue Mar 12, 2024 at 6:56 PM CDT, Sutou Kouhei wrote:

Hi,

In <CZOHWDYQJQCQ.23A5RRV1E05N2@neon.tech>
"Re: meson: Specify -Wformat as a common warning flag for extensions" on Fri, 08 Mar 2024 10:05:27 -0600,
"Tristan Partin" <tristan@neon.tech> wrote:

Ok, I figured this out. -Wall implies -Wformat=1. We set warning_level
to 1 in the Meson project() call, which implies -Wall, and set -Wall
in CFLAGS for autoconf. That's the reason we don't get issues building
Postgres. A user making use of the pg_config --cflags option, as Sutou
is, *will* run into the aforementioned issues, since we don't
propogate -Wall into pg_config.

$ gcc $(pg_config --cflags) -E - < /dev/null > /dev/null
cc1: warning: ‘-Wformat-security’ ignored without ‘-Wformat’
[-Wformat-security]
$ gcc -Wall $(pg_config --cflags) -E - < /dev/null > /dev/null
(nothing printed)

Thanks for explaining this. You're right. This is the reason
why we don't need this for PostgreSQL itself but we need
this for PostgreSQL extensions. Sorry. I should have
explained this in the first e-mail...

What should we do to proceed this patch?

Perhaps adding some more clarification in the comments that I wrote.

-  # -Wformat-security requires -Wformat, so check for it
+  # -Wformat-secuirty requires -Wformat. We compile with -Wall in 
+  # Postgres, which includes -Wformat=1. -Wformat is shorthand for 
+  # -Wformat=1. The set of flags which includes -Wformat-security is 
+  # persisted into pg_config --cflags, which is commonly used by 
+  # PGXS-based extensions. The lack of -Wformat in the persisted flags
+  # will produce a warning on many GCC versions, so even though adding 
+  # -Wformat here is a no-op for Postgres, it silences other use cases.

That might be too long-winded though :).

--
Tristan Partin
Neon (https://neon.tech)

Hi,

In <CZSDSNYEUHUL.399XLPGCJSJ5H@neon.tech>
"Re: meson: Specify -Wformat as a common warning flag for extensions" on Wed, 13 Mar 2024 00:43:11 -0500,
"Tristan Partin" <tristan@neon.tech> wrote:

Perhaps adding some more clarification in the comments that I wrote.

-  # -Wformat-security requires -Wformat, so check for it
+ # -Wformat-secuirty requires -Wformat. We compile with -Wall in + #
Postgres, which includes -Wformat=1. -Wformat is shorthand for + #
-Wformat=1. The set of flags which includes -Wformat-security is + #
persisted into pg_config --cflags, which is commonly used by + #
PGXS-based extensions. The lack of -Wformat in the persisted flags
+ # will produce a warning on many GCC versions, so even though adding
+ # -Wformat here is a no-op for Postgres, it silences other use
cases.

That might be too long-winded though :).

Thanks for the wording! I used it for the v3 patch.

Thanks,
--
kou

On 08.03.24 17:05, Tristan Partin wrote:

Ok, I figured this out. -Wall implies -Wformat=1. We set warning_level
to 1 in the Meson project() call, which implies -Wall, and set -Wall in
CFLAGS for autoconf. That's the reason we don't get issues building
Postgres. A user making use of the pg_config --cflags option, as Sutou
is, *will* run into the aforementioned issues, since we don't propogate
-Wall into pg_config.

(The actual mechanism for extensions is that they get CFLAGS from
Makefile.global, but pg_config has the same underlying issue.)

I think the fix then is to put -Wall into CFLAGS in Makefile.global.
Looking at a diff of Makefile.global between an autoconf and a meson
build, I also see that under meson, CFLAGS doesn't get -O2 -g (or
similar, depending on settings). This presumably has the same
underlying issue that meson handles those flags internally.

For someone who wants to write a fix for this, the relevant variable is
var_cflags in our meson scripts. And var_cxxflags as well.

Hi,

In <49e97fd0-c17e-4cbc-aeee-80ac51400736@eisentraut.org>
"Re: meson: Specify -Wformat as a common warning flag for extensions" on Wed, 13 Mar 2024 08:38:28 +0100,
Peter Eisentraut <peter@eisentraut.org> wrote:

I think the fix then is to put -Wall into CFLAGS in
Makefile.global. Looking at a diff of Makefile.global between an
autoconf and a meson build, I also see that under meson, CFLAGS
doesn't get -O2 -g (or similar, depending on settings). This
presumably has the same underlying issue that meson handles those
flags internally.

For someone who wants to write a fix for this, the relevant variable
is var_cflags in our meson scripts. And var_cxxflags as well.

How about the attached v4 patch?

Thanks,
--
kou

Hi,

On 2024-03-15 18:36:55 +0900, Sutou Kouhei wrote:

+warning_level = get_option('warning_level')
+# See https://mesonbuild.com/Builtin-options.html#details-for-warning_level for
+# warning_level values.
+if warning_level == '1'
+  common_builtin_flags += ['-Wall', '/W2']
+elif warning_level == '2'
+  common_builtin_flags += ['-Wall', '-Wextra', '/W3']
+elif warning_level == '3'
+  common_builtin_flags += ['-Wall', '-Wextra', '-Wpedantic', '/W4']
+elif warning_level == 'everything'
+  common_builtin_flags += ['-Weverything', '/Wall']
+endif

+cflags_builtin = cc.get_supported_arguments(common_builtin_flags)
+if llvm.found()
+  cxxflags_builtin = cpp.get_supported_arguments(common_builtin_flags)
+endif

This seems like a fair amount of extra configure tests. Particularly because
/W* isn't ever interesting for Makefile.global - they're msvc flags - because
you can't use that with msvc.

I'm also doubtful that it's worth supporting warning_level=3/everything, you
end up with a completely flood of warnings that way.

Greetings,

Andres Freund

Hi Andres,

Thanks for reviewing this!

In <20240407232635.fq4kc5556lahaoej@awork3.anarazel.de>
"Re: meson: Specify -Wformat as a common warning flag for extensions" on Sun, 7 Apr 2024 16:26:35 -0700,
Andres Freund <andres@anarazel.de> wrote:

This seems like a fair amount of extra configure tests. Particularly because
/W* isn't ever interesting for Makefile.global - they're msvc flags - because
you can't use that with msvc.

I'm also doubtful that it's worth supporting warning_level=3/everything, you
end up with a completely flood of warnings that way.

OK. I've removed "/W*" flags and warning_level==3/everything
cases.

How about the attached v5 patch?

Thanks,
--
kou

On 07.04.24 18:01, Sutou Kouhei wrote:

+# We don't have "warning_level == 3" and "warning_level ==
+# 'everything'" here because we don't use these warning levels.
+if warning_level == '1'
+  common_builtin_flags += ['-Wall']
+elif warning_level == '2'
+  common_builtin_flags += ['-Wall', '-Wextra']
+endif

I would trim this even further and always export just '-Wall'. The
other options aren't really something we support.

The other stanzas, on '-g' and '-O*', look good to me.

Hi,

In <4707d4ed-f268-43c0-b4dd-cdbc7520f508@eisentraut.org>
"Re: meson: Specify -Wformat as a common warning flag for extensions" on Tue, 28 May 2024 23:31:05 -0700,
Peter Eisentraut <peter@eisentraut.org> wrote:

On 07.04.24 18:01, Sutou Kouhei wrote:

+# We don't have "warning_level == 3" and "warning_level ==
+# 'everything'" here because we don't use these warning levels.
+if warning_level == '1'
+  common_builtin_flags += ['-Wall']
+elif warning_level == '2'
+  common_builtin_flags += ['-Wall', '-Wextra']
+endif

I would trim this even further and always export just '-Wall'. The
other options aren't really something we support.

OK. How about the v6 patch? It always uses '-Wall'.

Thanks,
--
kou

On 29.05.24 08:47, Sutou Kouhei wrote:

In <4707d4ed-f268-43c0-b4dd-cdbc7520f508@eisentraut.org>
"Re: meson: Specify -Wformat as a common warning flag for extensions" on Tue, 28 May 2024 23:31:05 -0700,
Peter Eisentraut <peter@eisentraut.org> wrote:

On 07.04.24 18:01, Sutou Kouhei wrote:

+# We don't have "warning_level == 3" and "warning_level ==
+# 'everything'" here because we don't use these warning levels.
+if warning_level == '1'
+  common_builtin_flags += ['-Wall']
+elif warning_level == '2'
+  common_builtin_flags += ['-Wall', '-Wextra']
+endif

I would trim this even further and always export just '-Wall'. The
other options aren't really something we support.

OK. How about the v6 patch? It always uses '-Wall'.

Yes, this looks good to me.

All: I think we should backpatch this. Otherwise, meson-based installs
will get suboptimal behavior for extension builds via pgxs.

On 29.05.24 08:47, Sutou Kouhei wrote:

In <4707d4ed-f268-43c0-b4dd-cdbc7520f508@eisentraut.org>
"Re: meson: Specify -Wformat as a common warning flag for extensions" on Tue, 28 May 2024 23:31:05 -0700,
Peter Eisentraut <peter@eisentraut.org> wrote:

On 07.04.24 18:01, Sutou Kouhei wrote:

+# We don't have "warning_level == 3" and "warning_level ==
+# 'everything'" here because we don't use these warning levels.
+if warning_level == '1'
+  common_builtin_flags += ['-Wall']
+elif warning_level == '2'
+  common_builtin_flags += ['-Wall', '-Wextra']
+endif

I would trim this even further and always export just '-Wall'. The
other options aren't really something we support.

OK. How about the v6 patch? It always uses '-Wall'.

I have committed this. Thanks.