public inbox for [email protected]help / color / mirror / Atom feed
pgsql: pg_logicalinspect: Fix possible crash when passing a directory p 16+ messages / 5 participants [nested] [flat]
* pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-03-11 16:57 Masahiko Sawada <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Masahiko Sawada @ 2025-03-11 16:57 UTC (permalink / raw) To: [email protected] 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 protected]> Co-authored-by: Bertrand Drouvot <[email protected]> Co-authored-by: Masahiko Sawada <[email protected]> Discussion: https://postgr.es/m/[email protected] 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(-) ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-03-12 02:08 David Rowley <[email protected]> parent: Masahiko Sawada <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: David Rowley @ 2025-03-12 02:08 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: [email protected] On Wed, 12 Mar 2025 at 05:57, Masahiko Sawada <[email protected]> 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 Attachments: [application/octet-stream] fix_parse_snapshot_filename_warning.patch (457B, 2-fix_parse_snapshot_filename_warning.patch) download | inline diff: diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c index 3b64d8ad880..13f9bda9459 100644 --- a/contrib/pg_logicalinspect/pg_logicalinspect.c +++ b/contrib/pg_logicalinspect/pg_logicalinspect.c @@ -85,6 +85,8 @@ parse_snapshot_filename(const char *filename) parse_error: ereport(ERROR, errmsg("invalid snapshot file name \"%s\"", filename)); + + return 0; /* keep compiler quiet */ } /* ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-03-12 04:56 Masahiko Sawada <[email protected]> parent: David Rowley <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Masahiko Sawada @ 2025-03-12 04:56 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; [email protected] On Tue, Mar 11, 2025 at 7:08 PM David Rowley <[email protected]> wrote: > > On Wed, 12 Mar 2025 at 05:57, Masahiko Sawada <[email protected]> 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-03-12 05:18 Tom Lane <[email protected]> parent: Masahiko Sawada <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: Tom Lane @ 2025-03-12 05:18 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: David Rowley <[email protected]>; [email protected] Masahiko Sawada <[email protected]> writes: > On Tue, Mar 11, 2025 at 7:08 PM David Rowley <[email protected]> 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-03-12 06:38 David Rowley <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: David Rowley @ 2025-03-12 06:38 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; [email protected] On Wed, 12 Mar 2025 at 18:18, Tom Lane <[email protected]> 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-03-12 08:15 Peter Eisentraut <[email protected]> parent: David Rowley <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Peter Eisentraut @ 2025-03-12 08:15 UTC (permalink / raw) To: David Rowley <[email protected]>; Masahiko Sawada <[email protected]>; +Cc: [email protected] On 12.03.25 03:08, David Rowley wrote: > On Wed, 12 Mar 2025 at 05:57, Masahiko Sawada <[email protected]> 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? ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-03-12 08:37 David Rowley <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: David Rowley @ 2025-03-12 08:37 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; [email protected] On Wed, 12 Mar 2025 at 21:15, Peter Eisentraut <[email protected]> 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-03-12 21:26 Masahiko Sawada <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: Masahiko Sawada @ 2025-03-12 21:26 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: David Rowley <[email protected]>; [email protected] On Tue, Mar 11, 2025 at 10:18 PM Tom Lane <[email protected]> wrote: > > Masahiko Sawada <[email protected]> writes: > > On Tue, Mar 11, 2025 at 7:08 PM David Rowley <[email protected]> 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-03-13 08:33 Peter Eisentraut <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Peter Eisentraut @ 2025-03-13 08:33 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; [email protected] On 12.03.25 09:37, David Rowley wrote: > On Wed, 12 Mar 2025 at 21:15, Peter Eisentraut <[email protected]> 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)? ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-03-20 11:17 David Rowley <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: David Rowley @ 2025-03-20 11:17 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; [email protected] On Thu, 13 Mar 2025 at 21:33, Peter Eisentraut <[email protected]> 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-07-16 14:56 Tom Lane <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Tom Lane @ 2025-07-16 14:56 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Masahiko Sawada <[email protected]>; [email protected] [ this thread was referenced recently, bringing it back top-of-mind ] David Rowley <[email protected]> writes: > On Thu, 13 Mar 2025 at 21:33, Peter Eisentraut <[email protected]> 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-07-17 00:52 David Rowley <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: David Rowley @ 2025-07-17 00:52 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Masahiko Sawada <[email protected]>; [email protected] On Thu, 17 Jul 2025 at 02:56, Tom Lane <[email protected]> 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... #include <stdio.h> #define __builtin_constant_p(x) _Generic((1 ? ((void*)((x)*(uintptr_t)0)) : &(int){1}), int*: 1, void*: 0) #define test(e) printf("%d\n", __builtin_constant_p(e)); #define ERROR 1 int main(void) { int v = 1; test(v); test(ERROR); test(1); return 0; } Attachments: [text/plain] isconst.c (279B, 2-isconst.c) download | inline: #include <stdio.h> #define __builtin_constant_p(x) _Generic((1 ? ((void*)((x)*(uintptr_t)0)) : &(int){1}), int*: 1, void*: 0) #define test(e) printf("%d\n", __builtin_constant_p(e)); #define ERROR 1 int main(void) { int v = 1; test(v); test(ERROR); test(1); return 0; } ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-07-17 03:19 Tom Lane <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Tom Lane @ 2025-07-17 03:19 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Masahiko Sawada <[email protected]>; [email protected] David Rowley <[email protected]> writes: > On Thu, 17 Jul 2025 at 02:56, Tom Lane <[email protected]> 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-07-17 04:09 David Rowley <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: David Rowley @ 2025-07-17 04:09 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Masahiko Sawada <[email protected]>; [email protected] On Thu, 17 Jul 2025 at 15:19, Tom Lane <[email protected]> wrote: > > David Rowley <[email protected]> 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/ Attachments: [application/octet-stream] v1-0001-Detect-elog-ERROR-can-t-return-in-MSVC-when-using.patch (1.8K, 2-v1-0001-Detect-elog-ERROR-can-t-return-in-MSVC-when-using.patch) download | inline diff: From e4ced93b2c35db91cb28f564103a7238efd72b43 Mon Sep 17 00:00:00 2001 From: David Rowley <[email protected]> Date: Thu, 17 Jul 2025 15:49:03 +1200 Subject: [PATCH v1] Detect elog(ERROR) can't return in MSVC when using C11 --- src/backend/backup/basebackup_target.c | 2 +- src/include/utils/elog.h | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/backend/backup/basebackup_target.c b/src/backend/backup/basebackup_target.c index 84b1309d3bd..0b48521f104 100644 --- a/src/backend/backup/basebackup_target.c +++ b/src/backend/backup/basebackup_target.c @@ -146,7 +146,7 @@ BaseBackupGetTargetHandle(char *target, char *target_detail) errmsg("unrecognized target: \"%s\"", target))); /* keep compiler quiet */ - return NULL; + //return NULL; } /* diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 675f4f5f469..c93ba82ecf8 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -137,7 +137,21 @@ struct Node; * prevents gcc from making the unreachability deduction at optlevel -O0. *---------- */ -#ifdef HAVE__BUILTIN_CONSTANT_P + +#if defined(_MSC_VER) && defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112 +#define pg_builtin_constant(x) \ + _Generic((1 ? ((void*)((x)*(uintptr_t)0)) : &(int){1}), int*: 1, void*: 0) +#define ereport_domain(elevel, domain, ...) \ + do { \ + pg_prevent_errno_in_scope(); \ + if (pg_builtin_constant(elevel) && (elevel) >= ERROR ? \ + errstart_cold(elevel, domain) : \ + errstart(elevel, domain)) \ + __VA_ARGS__, errfinish(__FILE__, __LINE__, __func__); \ + if (pg_builtin_constant(elevel) && (elevel) >= ERROR) \ + pg_unreachable(); \ + } while(0) +#elif defined(HAVE__BUILTIN_CONSTANT_P) #define ereport_domain(elevel, domain, ...) \ do { \ pg_prevent_errno_in_scope(); \ -- 2.40.1.windows.1 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-07-17 04:20 Tom Lane <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Tom Lane @ 2025-07-17 04:20 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; [email protected] David Rowley <[email protected]> writes: > On Thu, 17 Jul 2025 at 15:19, Tom Lane <[email protected]> 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: pgsql: pg_logicalinspect: Fix possible crash when passing a directory p @ 2025-07-21 00:39 David Rowley <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: David Rowley @ 2025-07-21 00:39 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; [email protected] On Thu, 17 Jul 2025 at 16:20, Tom Lane <[email protected]> wrote: > > David Rowley <[email protected]> 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 Attachments: [application/octet-stream] v2-0001-Detect-elog-ERROR-can-t-return-in-MSVC-when-using.patch (4.5K, 2-v2-0001-Detect-elog-ERROR-can-t-return-in-MSVC-when-using.patch) download | inline diff: From 26442d786cc65211f0176e803dfa269e9e93c8bd Mon Sep 17 00:00:00 2001 From: David Rowley <[email protected]> Date: Thu, 17 Jul 2025 15:49:03 +1200 Subject: [PATCH v2] Detect elog(ERROR) can't return in MSVC when using C11 --- meson.build | 24 ++++++++++++++++++++++++ src/backend/backup/basebackup_target.c | 4 ++-- src/include/c.h | 17 +++++++++++++++++ src/include/utils/elog.h | 6 +++--- 4 files changed, 46 insertions(+), 5 deletions(-) diff --git a/meson.build b/meson.build index 5365aaf95e6..e23a99c7b1d 100644 --- a/meson.build +++ b/meson.build @@ -1704,6 +1704,18 @@ endif # Compiler tests ############################################################### +# Test to see if the compiler supports C11 +c11_test = ''' +#if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L +#error "C11 not supported" +#endif + +int main(int argc, char **argv) +{ + return 0; +} +''' + # Do we need -std=c99 to compile C99 code? We don't want to add -std=c99 # unnecessarily, because we optionally rely on newer features. c99_test = ''' @@ -1737,6 +1749,18 @@ int main(int argc, char **argv) } ''' +# The current minimum version of MSVC we currently support supports at least +# C11, so lets try to compile with C11. Check if we need to actually pass the +# /std:c11 flag for that to work. If this test ends up adding the C11 flag, +# then the C99 test below should always pass and never add the C99 flag. +if cc.get_id() == 'msvc' and not cc.compiles(c11_test, name: 'c11', args: test_c_args) + if cc.compiles(c11_test, name: 'c11 with /std:c11', + args: test_c_args + ['/std:c11']) + test_c_args += '/std:c11' + cflags += '/std:c11' + endif +endif + if not cc.compiles(c99_test, name: 'c99', args: test_c_args) if cc.compiles(c99_test, name: 'c99 with -std=c99', args: test_c_args + ['-std=c99']) diff --git a/src/backend/backup/basebackup_target.c b/src/backend/backup/basebackup_target.c index 84b1309d3bd..01705368950 100644 --- a/src/backend/backup/basebackup_target.c +++ b/src/backend/backup/basebackup_target.c @@ -145,8 +145,8 @@ BaseBackupGetTargetHandle(char *target, char *target_detail) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("unrecognized target: \"%s\"", target))); - /* keep compiler quiet */ - return NULL; + /* comment out for testing. Not for commit. */ + /* return NULL; */ } /* diff --git a/src/include/c.h b/src/include/c.h index 6d4495bdd9f..48a8cedec2d 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -332,6 +332,23 @@ #define pg_unreachable() abort() #endif +/* + * Define a compiler-independent macro for determining if an expression is a + * compile-time const. With MSVC using C11 and above, we can use a trick with + * _Generic to make this work. We don't define this macro to return 0 when + * unsupported due to the risk of users of the macro misbehaving if we return + * 0 when the expression is a built-in constant. Callers may check if this + * macro is defined by checking if HAVE_PG_BUILTIN_CONSTANT is defined. + */ +#if defined(HAVE__BUILTIN_CONSTANT_P) +#define pg_builtin_constant(x) __builtin_constant_p(x) +#define HAVE_PG_BUILTIN_CONSTANT +#elif defined(_MSC_VER) && defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L +#define pg_builtin_constant(x) \ + _Generic((1 ? ((void *) ((x) * (uintptr_t) 0)) : &(int) {1}), int *: 1, void *: 0) +#define HAVE_PG_BUILTIN_CONSTANT +#endif + /* * pg_assume(expr) states that we assume `expr` to evaluate to true. In assert * enabled builds pg_assume() is turned into an assertion, in optimized builds diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 675f4f5f469..7b575b59ed3 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -137,15 +137,15 @@ struct Node; * prevents gcc from making the unreachability deduction at optlevel -O0. *---------- */ -#ifdef HAVE__BUILTIN_CONSTANT_P +#ifdef HAVE_PG_BUILTIN_CONSTANT #define ereport_domain(elevel, domain, ...) \ do { \ pg_prevent_errno_in_scope(); \ - if (__builtin_constant_p(elevel) && (elevel) >= ERROR ? \ + if (pg_builtin_constant(elevel) && (elevel) >= ERROR ? \ errstart_cold(elevel, domain) : \ errstart(elevel, domain)) \ __VA_ARGS__, errfinish(__FILE__, __LINE__, __func__); \ - if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \ + if (pg_builtin_constant(elevel) && (elevel) >= ERROR) \ pg_unreachable(); \ } while(0) #else /* !HAVE__BUILTIN_CONSTANT_P */ -- 2.40.1.windows.1 ^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2025-07-21 00:39 UTC | newest] Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-03-11 16:57 pgsql: pg_logicalinspect: Fix possible crash when passing a directory p Masahiko Sawada <[email protected]> 2025-03-12 02:08 ` David Rowley <[email protected]> 2025-03-12 04:56 ` Masahiko Sawada <[email protected]> 2025-03-12 05:18 ` Tom Lane <[email protected]> 2025-03-12 06:38 ` David Rowley <[email protected]> 2025-03-12 21:26 ` Masahiko Sawada <[email protected]> 2025-03-12 08:15 ` Peter Eisentraut <[email protected]> 2025-03-12 08:37 ` David Rowley <[email protected]> 2025-03-13 08:33 ` Peter Eisentraut <[email protected]> 2025-03-20 11:17 ` David Rowley <[email protected]> 2025-07-16 14:56 ` Tom Lane <[email protected]> 2025-07-17 00:52 ` David Rowley <[email protected]> 2025-07-17 03:19 ` Tom Lane <[email protected]> 2025-07-17 04:09 ` David Rowley <[email protected]> 2025-07-17 04:20 ` Tom Lane <[email protected]> 2025-07-21 00:39 ` David Rowley <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox