public inbox for [email protected]  

help / color / mirror / Atom feed

From: Alexander Lakhin <[email protected]>
To: [email protected]
Cc: Tomas Vondra <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Rachel Heaton <[email protected]>
Subject: Re: Add LZ4 compression in pg_dump
Date: Sat, 11 Mar 2023 21:00:00 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <1aBKUlb8BGVUUTkRJwEmU3ud1NyeHVCoC7bpOKAuRYlHHJ7hRiDVXYgad-wmjx1K3pykd_ctyY4Lqy9xA7JGLXOm1Eul-imdZ_7TVr5LY8U=@pm.me>
References: <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>
	<AcPUJxzDanXfXdqYOrrZ0L9J9qcnCblkc5z5x9IgKUm7d89pa6B7y8fhUOb2JGDef8h3VP6Ts7j-BYSMmtn50jVuHAT2nBleGosIdVyhG7Q=@pm.me>
	<OSZPR01MB631087FC3B53AD8B1E3439F7FDA39@OSZPR01MB6310.jpnprd01.prod.outlook.com>
	<SnTBJOQtJQf_mnXh6icTznqC05gnGqXKWDwjEA15uIx3zf0cJBwY1aK7ULztKoqlXrGM39lHtEpLGmS48A7585FXo1sO6W2YR5RtiDoWpDU=@pm.me>
	<[email protected]>
	<mYBKRYonRy5OiDDYxcxOOJ5xM9iLnuDsGP92zC7mjw8ew1wiccYZIpTvQEmtgRoAXRhaj91SAKwtFluUtRDZ3r6pRdGYLncWK7rXIQrHhFY=@pm.me>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<1aBKUlb8BGVUUTkRJwEmU3ud1NyeHVCoC7bpOKAuRYlHHJ7hRiDVXYgad-wmjx1K3pykd_ctyY4Lqy9xA7JGLXOm1Eul-imdZ_7TVr5LY8U=@pm.me>

Hi Georgios,

11.03.2023 13:50, [email protected] wrote:
> I can not answer about the buildfarms. Do you think that adding an explicit
> check for this warning in meson would help? I am a bit uncertain as I think
> that type-limits are included in extra.
>
> @@ -1748,6 +1748,7 @@ common_warning_flags = [
>     '-Wshadow=compatible-local',
>     # This was included in -Wall/-Wformat in older GCC versions
>     '-Wformat-security',
> +  '-Wtype-limits',
>   ]
I'm not sure that I can promote additional checks (or determine where
to put them), but if some patch introduces a warning of a type that wasn't
present before, I think it's worth to eliminate the warning (if it is
sensible) to keep the source code check baseline at the same level
or even lift it up gradually.
I've also found that the same commit introduced a single instance of
the analyzer-possible-null-argument warning:
CPPFLAGS="-Og -fanalyzer -Wno-analyzer-malloc-leak -Wno-analyzer-file-leak 
-Wno-analyzer-null-dereference -Wno-analyzer-shift-count-overflow 
-Wno-analyzer-free-of-non-heap -Wno-analyzer-null-argument 
-Wno-analyzer-double-free -Wanalyzer-possible-null-argument" ./configure 
--with-lz4 -q && make -s -j8
compress_io.c: In function ‘hasSuffix’:
compress_io.c:158:47: warning: use of possibly-NULL ‘filename’ where non-null 
expected [CWE-690] [-Wanalyzer-possible-null-argument]
   158 |         int                     filenamelen = strlen(filename);
       | ^~~~~~~~~~~~~~~~
   ‘InitDiscoverCompressFileHandle’: events 1-3
...

(I use gcc-11.3.)
As I can see, many existing uses of strdup() are followed by a check for
null result, so maybe it's a common practice and a similar check should
be added in InitDiscoverCompressFileHandle().
(There also a couple of other warnings introduced with the lz4 compression
patches, but those ones are not unique, so I maybe they aren't worth fixing.)

>> It is a good thing that the restore fails with bad input. Yet it should
>> have failed earlier. The attached makes certain it does fail earlier.
>>
Thanks! Your patch definitely fixes the issue.

Best regards,
Alexander

view thread (40+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Add LZ4 compression in pg_dump
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox