Subject: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p

From: Masahiko Sawada <[email-masked]>
Date: Tue, 11 Mar 2025 16:57:06 +0000

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 <[email-masked]>
Co-authored-by: Bertrand Drouvot <[email-masked]>
Co-authored-by: Masahiko Sawada <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]

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(-)

---

From: David Rowley <[email-masked]>
Date: Wed, 12 Mar 2025 15:08:22 +1300

> 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

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 11 Mar 2025 21:56:14 -0700

> 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

---

From: Tom Lane <[email-masked]>
Date: Wed, 12 Mar 2025 01:18:09 -0400

> 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

---

From: David Rowley <[email-masked]>
Date: Wed, 12 Mar 2025 19:38:50 +1300

> index a14c6315162..467b1f58ae8 100644

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

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 12 Mar 2025 09:15:15 +0100

> the ereport(ERROR) does not return.

Which compiler is that?

---

From: David Rowley <[email-masked]>
Date: Wed, 12 Mar 2025 21:37:35 +1300

> > the ereport(ERROR) does not return.

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

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 12 Mar 2025 14:26:37 -0700

> index a14c6315162..467b1f58ae8 100644

Thanks. In my environment, I got the warning by this way.

And I've pushed the fix.

Regards,

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 13 Mar 2025 09:33:55 +0100

> I suspect drongo will also show the same warning once it runs on the

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)?

---

From: David Rowley <[email-masked]>
Date: Fri, 21 Mar 2025 00:17:56 +1300

> 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.

> about this if you don't use assertions (or vice versa)?

It happens with both.

David

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 Jul 2025 10:56:20 -0400

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

> 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

---

From: David Rowley <[email-masked]>
Date: Thu, 17 Jul 2025 12:52:07 +1200

> 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/

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 Jul 2025 23:19:56 -0400

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

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

> 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

---

From: David Rowley <[email-masked]>
Date: Thu, 17 Jul 2025 16:09:18 +1200

> we could require it in MSVC builds?  Which MSVC versions would

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/

---

From: Tom Lane <[email-masked]>
Date: Thu, 17 Jul 2025 00:20:30 -0400

> 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.

> 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.

> 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

---

From: David Rowley <[email-masked]>
Date: Mon, 21 Jul 2025 12:39:36 +1200

> 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