Thread: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p
pg_logicalinspect: Fix possible crash when passing a directory path. Previously, pg_logicalinspect functions were too trusting of their input and blindly passed it to SnapBuildRestoreSnapshot(). If the input pointed to a directory, the server could a PANIC error while attempting to fsync_fname() with isdir=false on a directory. This commit adds validation checks for input filenames and passes the LSN extracted from the filename to SnapBuildRestoreSnapshot() instead of the filename itself. It also adds regression tests for various input patterns and permission checks. Bug: #18828 Reported-by: Robins Tharakan <tharakan@gmail.com> Co-authored-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com> Co-authored-by: Masahiko Sawada <sawada.mshk@gmail.com> Discussion: https://postgr.es/m/18828-0f4701c635064211@postgresql.org Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/bd65cb3cd48a7a5ce48b26f8031ad3968efed87e Modified Files -------------- contrib/pg_logicalinspect/Makefile | 1 + .../expected/pg_logicalinspect.out | 81 ++++++++++++++++++++++ contrib/pg_logicalinspect/pg_logicalinspect.c | 55 ++++++++++++--- .../pg_logicalinspect/sql/pg_logicalinspect.sql | 48 +++++++++++++ src/backend/replication/logical/snapbuild.c | 14 ++-- src/include/replication/snapbuild_internal.h | 2 +- 6 files changed, 183 insertions(+), 18 deletions(-)
On Wed, 12 Mar 2025 at 05:57, Masahiko Sawada <msawada@postgresql.org> wrote: > contrib/pg_logicalinspect/pg_logicalinspect.c | 55 ++++++++++++--- This introduces a new compiler warning for compilers that don't know the ereport(ERROR) does not return. The attached is enough to fix it. David
Attachment
Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p
From
Masahiko Sawada
Date:
On Tue, Mar 11, 2025 at 7:08 PM David Rowley <dgrowleyml@gmail.com> wrote: > > On Wed, 12 Mar 2025 at 05:57, Masahiko Sawada <msawada@postgresql.org> wrote: > > contrib/pg_logicalinspect/pg_logicalinspect.c | 55 ++++++++++++--- > > This introduces a new compiler warning for compilers that don't know > the ereport(ERROR) does not return. > > The attached is enough to fix it. Thank you for the report. Can we generate the warning using some gcc's warning flags? I'd like to add a check to my personal pre-commit test. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Masahiko Sawada <sawada.mshk@gmail.com> writes:
> On Tue, Mar 11, 2025 at 7:08 PM David Rowley <dgrowleyml@gmail.com> wrote:
>> This introduces a new compiler warning for compilers that don't know
>> the ereport(ERROR) does not return.
> Thank you for the report. Can we generate the warning using some gcc's
> warning flags? I'd like to add a check to my personal pre-commit test.
I don't know of an easy way. I experimented with doing this:
diff --git a/src/include/c.h b/src/include/c.h
index a14c6315162..467b1f58ae8 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -315,7 +315,7 @@
#elif defined(_MSC_VER) && !defined(USE_ASSERT_CHECKING)
#define pg_unreachable() __assume(0)
#else
-#define pg_unreachable() abort()
+#define pg_unreachable() ((void) 0)
#endif
/*
which seems like it ought to be enough to provoke such warnings
(in assert-enabled builds). I got upwards of sixty build warnings
this way, but the place David mentions was *not* among them.
So I'm confused.
regards, tom lane
On Wed, 12 Mar 2025 at 18:18, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I don't know of an easy way. I experimented with doing this: > > diff --git a/src/include/c.h b/src/include/c.h > index a14c6315162..467b1f58ae8 100644 > --- a/src/include/c.h > +++ b/src/include/c.h > @@ -315,7 +315,7 @@ > #elif defined(_MSC_VER) && !defined(USE_ASSERT_CHECKING) > #define pg_unreachable() __assume(0) > #else > -#define pg_unreachable() abort() > +#define pg_unreachable() ((void) 0) > #endif > > /* > > which seems like it ought to be enough to provoke such warnings > (in assert-enabled builds). I got upwards of sixty build warnings > this way, but the place David mentions was *not* among them. > So I'm confused. Doing that for me does show the new warning. drowley@amd7945hx:~/pg_src$ meson setup -Dcassert=true build > /dev/null && cd build && ninja | grep pg_logicalinspect.c [1962/2356] Compiling C object contrib/pg_logicalinspect/pg_logicalinspect.so.p/pg_logicalinspect.c.o ../contrib/pg_logicalinspect/pg_logicalinspect.c: In function ‘parse_snapshot_filename’: ../contrib/pg_logicalinspect/pg_logicalinspect.c:88:1: warning: control reaches end of non-void function [-Wreturn-type] drowley@amd7945hx:~/pg_src$ gcc --version gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0 David
Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p
From
Peter Eisentraut
Date:
On 12.03.25 03:08, David Rowley wrote: > On Wed, 12 Mar 2025 at 05:57, Masahiko Sawada <msawada@postgresql.org> wrote: >> contrib/pg_logicalinspect/pg_logicalinspect.c | 55 ++++++++++++--- > > This introduces a new compiler warning for compilers that don't know > the ereport(ERROR) does not return. Which compiler is that?
On Wed, 12 Mar 2025 at 21:15, Peter Eisentraut <peter@eisentraut.org> wrote: > > On 12.03.25 03:08, David Rowley wrote: > > This introduces a new compiler warning for compilers that don't know > > the ereport(ERROR) does not return. > > Which compiler is that? C:\Users\drowley\pg_src>cl Microsoft (R) C/C++ Optimizing Compiler Version 19.43.34808 for x64 Copyright (C) Microsoft Corporation. All rights reserved. I suspect drongo will also show the same warning once it runs on the latest commit shortly. David
Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p
From
Masahiko Sawada
Date:
On Tue, Mar 11, 2025 at 10:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Masahiko Sawada <sawada.mshk@gmail.com> writes: > > On Tue, Mar 11, 2025 at 7:08 PM David Rowley <dgrowleyml@gmail.com> wrote: > >> This introduces a new compiler warning for compilers that don't know > >> the ereport(ERROR) does not return. > > > Thank you for the report. Can we generate the warning using some gcc's > > warning flags? I'd like to add a check to my personal pre-commit test. > > I don't know of an easy way. I experimented with doing this: > > diff --git a/src/include/c.h b/src/include/c.h > index a14c6315162..467b1f58ae8 100644 > --- a/src/include/c.h > +++ b/src/include/c.h > @@ -315,7 +315,7 @@ > #elif defined(_MSC_VER) && !defined(USE_ASSERT_CHECKING) > #define pg_unreachable() __assume(0) > #else > -#define pg_unreachable() abort() > +#define pg_unreachable() ((void) 0) > #endif > > /* > > which seems like it ought to be enough to provoke such warnings > (in assert-enabled builds). I got upwards of sixty build warnings > this way, but the place David mentions was *not* among them. > So I'm confused. Thanks. In my environment, I got the warning by this way. And I've pushed the fix. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p
From
Peter Eisentraut
Date:
On 12.03.25 09:37, David Rowley wrote: > On Wed, 12 Mar 2025 at 21:15, Peter Eisentraut <peter@eisentraut.org> wrote: >> >> On 12.03.25 03:08, David Rowley wrote: >>> This introduces a new compiler warning for compilers that don't know >>> the ereport(ERROR) does not return. >> >> Which compiler is that? > > C:\Users\drowley\pg_src>cl > Microsoft (R) C/C++ Optimizing Compiler Version 19.43.34808 for x64 > Copyright (C) Microsoft Corporation. All rights reserved. > > I suspect drongo will also show the same warning once it runs on the > latest commit shortly. Ok, this is weird, because we have pg_unreachable() support for MSVC: #if defined(HAVE__BUILTIN_UNREACHABLE) && !defined(USE_ASSERT_CHECKING) #define pg_unreachable() __builtin_unreachable() #elif defined(_MSC_VER) && !defined(USE_ASSERT_CHECKING) #define pg_unreachable() __assume(0) #else #define pg_unreachable() abort() #endif Is there a way to reshuffle those conditionals to make this actually do something useful on MSVC? Are you compiling with assertions on in this case? Does anything change about this if you don't use assertions (or vice versa)?
On Thu, 13 Mar 2025 at 21:33, Peter Eisentraut <peter@eisentraut.org> wrote: > Ok, this is weird, because we have pg_unreachable() support for MSVC: > > #if defined(HAVE__BUILTIN_UNREACHABLE) && !defined(USE_ASSERT_CHECKING) > #define pg_unreachable() __builtin_unreachable() > #elif defined(_MSC_VER) && !defined(USE_ASSERT_CHECKING) > #define pg_unreachable() __assume(0) > #else > #define pg_unreachable() abort() > #endif > > Is there a way to reshuffle those conditionals to make this actually do > something useful on MSVC? I've just been experimenting with this and it seems the problem isn't with pg_unreachable(), it's with the compiler not understanding that the particular pg_unreachable() is always reached. What's happening is down to the multi-eval protection code for elevel in ereport_domain(). Because elevel is assigned to the variable "elevel_" the compiler seems to lose its proof that the pg_unreachable() is always reached. Adjusting that condition to use the elevel parameter directly makes the warning disappear. I looked around to see if MSVC might have something to allow us to fix this, but didn't find anything. There does not seem to be any sort of __builtin_constant_p with MSVC, otherwise we could've done something similar to the HAVE__BUILTIN_CONSTANT_P version of ereport_domain just above. > Are you compiling with assertions on in this case? Does anything change > about this if you don't use assertions (or vice versa)? It happens with both. David