Thread: fixing more format truncation issues
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
Attachment
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
Attachment
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
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
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
Attachment
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