Обсуждение: 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

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

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

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

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

[ this thread was referenced recently, bringing it back top-of-mind ]

David Rowley <dgrowleyml@gmail.com> writes:
> On Thu, 13 Mar 2025 at 21:33, Peter Eisentraut <peter@eisentraut.org> wrote:
>> 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.

Looking again at the code for ereport_domain(), I wondered if
something like this would help MSVC see through it:

 #define ereport_domain(elevel, domain, ...)    \
    do { \
        const int elevel_ = (elevel); \
+       const bool is_error_ = (elevel_ >= ERROR); \
        pg_prevent_errno_in_scope(); \
        if (errstart(elevel_, domain)) \
            __VA_ARGS__, errfinish(__FILE__, __LINE__, __func__); \
-       if (elevel_ >= ERROR) \
+       if (is_error_) \
            pg_unreachable(); \
    } while(0)

This preserves single evaluation of the elevel parameter, and
perhaps it'd move the needle on whether the compiler thinks
is_error_ is a compile-time constant.  I'm just guessing
though, don't have this compiler to test with.

            regards, tom lane

On Thu, 17 Jul 2025 at 02:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Looking again at the code for ereport_domain(), I wondered if
> something like this would help MSVC see through it:
>
>  #define ereport_domain(elevel, domain, ...)    \
>     do { \
>         const int elevel_ = (elevel); \
> +       const bool is_error_ = (elevel_ >= ERROR); \
>         pg_prevent_errno_in_scope(); \
>         if (errstart(elevel_, domain)) \
>             __VA_ARGS__, errfinish(__FILE__, __LINE__, __func__); \
> -       if (elevel_ >= ERROR) \
> +       if (is_error_) \
>             pg_unreachable(); \
>     } while(0)
>
> This preserves single evaluation of the elevel parameter, and
> perhaps it'd move the needle on whether the compiler thinks
> is_error_ is a compile-time constant.  I'm just guessing
> though, don't have this compiler to test with.

I tried this and it unfortunately doesn't fix the issue. I expect that
the compiler is losing the ability to detect const-ness through the
"const" variables, and since is_error_ is being set from elevel_ it's
not seen as compile-time detectability constant either.

I spent a bit more time searching for a solution and did find
something that works well enough for this case in [1]. Unfortunately,
it only works with C11. See attached .c file and output below.

C11 test:

> cl /std:c11 isconst.c && isconst.exe
Microsoft (R) C/C++ Optimizing Compiler Version 19.44.35211 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

isconst.c
Microsoft (R) Incremental Linker Version 14.44.35211.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:isconst.exe
isconst.obj
0
1
1

C99 test:

> cl isconst.c && isconst.exe
Microsoft (R) C/C++ Optimizing Compiler Version 19.44.35211 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

isconst.c
isconst.c(12): error C2059: syntax error: 'type'
isconst.c(13): error C2059: syntax error: 'type'
isconst.c(14): error C2059: syntax error: 'type'

I didn't manage to find anything that works in C99.

David

[1] https://www.reddit.com/r/C_Programming/comments/o3ekqe/i_think_i_found_a_c11_compliant_way_to_detect/

David Rowley <dgrowleyml@gmail.com> writes:
> On Thu, 17 Jul 2025 at 02:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Looking again at the code for ereport_domain(), I wondered if
>> something like this would help MSVC see through it:

> I tried this and it unfortunately doesn't fix the issue.

I didn't have huge hopes for that, but thanks for checking.

> I spent a bit more time searching for a solution and did find
> something that works well enough for this case in [1]. Unfortunately,
> it only works with C11. See attached .c file and output below.

Hmph.  I doubt we are ready to require C11 everywhere, but maybe
we could require it in MSVC builds?  Which MSVC versions would
that eliminate?

            regards, tom lane

On Thu, 17 Jul 2025 at 15:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > I spent a bit more time searching for a solution and did find
> > something that works well enough for this case in [1]. Unfortunately,
> > it only works with C11. See attached .c file and output below.
>
> Hmph.  I doubt we are ready to require C11 everywhere, but maybe
> we could require it in MSVC builds?  Which MSVC versions would
> that eliminate?

Going by [1] it's Visual Studio 2019, which as of 8fd9bb1d9 is now our
minimum supported VS version.

I hacked up a quick patch (attached) which compiles without any
warnings.  Tested on VS2012 with meson setup -Dc_std=c11.

Unsure what other repercussions there are of compiling with C11 and
have not looked into what it would take to adjust the meson build
scripts to default to that for MSVC builds.

David

[1] https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/

David Rowley <dgrowleyml@gmail.com> writes:
> On Thu, 17 Jul 2025 at 15:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hmph.  I doubt we are ready to require C11 everywhere, but maybe
>> we could require it in MSVC builds?  Which MSVC versions would
>> that eliminate?

> Going by [1] it's Visual Studio 2019, which as of 8fd9bb1d9 is now our
> minimum supported VS version.

Hmm, so apparently no cost ... also, if I'm getting this right,
the only downside of compiling with an older compiler would be
seeing a lot of "possibly-uninitialized" warnings.

> I hacked up a quick patch (attached) which compiles without any
> warnings.  Tested on VS2012 with meson setup -Dc_std=c11.

Personally I'd think about providing a definition of
__builtin_constant_p rather than changing elog.h; but we'd
end up at the same place.

> Unsure what other repercussions there are of compiling with C11 and
> have not looked into what it would take to adjust the meson build
> scripts to default to that for MSVC builds.

In theory there should be no repercussions, because we already
support compiling with C11: C99 is a minimum not a maximum.

But I've not checked the meson scripts either.

            regards, tom lane

On Thu, 17 Jul 2025 at 16:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > Unsure what other repercussions there are of compiling with C11 and
> > have not looked into what it would take to adjust the meson build
> > scripts to default to that for MSVC builds.
>
> In theory there should be no repercussions, because we already
> support compiling with C11: C99 is a minimum not a maximum.

Oh, right.

> But I've not checked the meson scripts either.

The attached seems to work.

It's my first time hacking on meson build scripts. I was surprised to
see the "-std=c99" test just does not work for MSVC since the flag is
spelt "/std:c99" for that compiler. The reason it's not causing
trouble is due to the code compiling without that flag, so the test
that passes the flag is never executed.  For the test I added, I had
to use the MSVC naming convention, which seems fine as it's only run
when "cc.get_id() == 'msvc'".

I've not gone over the other users of __builtin_constant_p() to
replace them with the pg_builtin_constant(). That's probably a good
exercise to do as I'm not sure how well the MSVC _Generic trick works
or if there might be some expressions it doesn't work with.

I also opted not to have the pg_builtin_constant() macro return 0 when
it's unsupported as I was worred some code might make too many
assumptions about the variability of the expression if that were to
return false. I don't quite have a problem case in mind for that.
Doing it that way was mostly erring on the side of caution.

For now, I've left in a commented out "return NULL;". I get no
compiler warnings from that.

David