Re: Assert name/short_desc to prevent SHOW ALL segfault
On Sat, Jul 02, 2022 at 12:33:28PM +0900, Michael Paquier wrote: > Now that v16 is open for business, I have been able to look at it > again, and applied the patch on HEAD. My apologies for the wait. Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Assert name/short_desc to prevent SHOW ALL segfault
On Sat, May 28, 2022 at 05:50:18AM -0700, Nathan Bossart wrote: > Yeah, I see no reason that this should go into v15. I created a new > commitfest entry so that this isn't forgotten: > > https://commitfest.postgresql.org/38/3655/ > > And I've reposted 0002 here so that we get some cfbot coverage in the > meantime. Now that v16 is open for business, I have been able to look at it again, and applied the patch on HEAD. My apologies for the wait. -- Michael signature.asc Description: PGP signature
Re: Assert name/short_desc to prevent SHOW ALL segfault
On Sat, May 28, 2022 at 12:26:34PM +0900, Michael Paquier wrote: > On Fri, May 27, 2022 at 10:43:17AM -0700, Nathan Bossart wrote: >> Makes sense. Here's a new patch set. 0001 is the part intended for >> back-patching, and 0002 is the rest (i.e., adding pg_attribute_nonnull()). >> I switched to using __has_attribute to discover whether nonnull was > > Okay, I have looked at 0001 this morning and applied it down to 12. > The change in GetConfigOptionByNum() is not required in 10 and 11, as > the strings of pg_show\all_settings() have begun to be translated in > 12~. Thanks! >> supported, as that seemed cleaner. I didn't see any need for a new >> configure check, but maybe I am missing something. > > And I've learnt today that we enforce a definition of __has_attribute > at the top of c.h, and that we already rely on that. So I agree that > what you are doing in 0002 should be enough. Should we wait until 16~ > opens for business though? I don't see a strong argument to push > forward with that now that we are in beta mode on HEAD. Yeah, I see no reason that this should go into v15. I created a new commitfest entry so that this isn't forgotten: https://commitfest.postgresql.org/38/3655/ And I've reposted 0002 here so that we get some cfbot coverage in the meantime. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From 88ac1a95e84998ce473d461424b81b91a0a3e3cc Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Fri, 27 May 2022 10:10:09 -0700 Subject: [PATCH v5 1/1] Introduce pg_attribute_nonnull() and use it for DefineCustomXXXVariable(). pg_attribute_nonnull() can be used to generate compiler warnings when a function is called with the specified arguments set to NULL. An empty argument list indicates that all pointer arguments must not be NULL. pg_attribute_nonnull() only works for compilers that support the nonnull function attribute. If nonnull is not supported, pg_attribute_nonnull() has no effect. In addition to introducing pg_attribute_nonnull(), this change uses it for the DefineCustomXXXVariable() functions to generate warnings when the "name" and "valueAddr" arguments are set to NULL. Idea by: Andres Freund Author: Nathan Bossart Reviewed by: Michael Paquier, Tom Lane Discussion: https://postgr.es/m/20220525061739.ur7x535vtzyzkmqo%40alap3.anarazel.de --- src/include/c.h | 11 +++ src/include/utils/guc.h | 10 +- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/include/c.h b/src/include/c.h index 4f16e589b3..0c64557c62 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -144,6 +144,17 @@ #define pg_attribute_no_sanitize_alignment() #endif +/* + * pg_attribute_nonnull means the compiler should warn if the function is called + * with the listed arguments set to NULL. If no arguments are listed, the + * compiler should warn if any pointer arguments are set to NULL. + */ +#if __has_attribute (nonnull) +#define pg_attribute_nonnull(...) __attribute__((nonnull(__VA_ARGS__))) +#else +#define pg_attribute_nonnull(...) +#endif + /* * Append PG_USED_FOR_ASSERTS_ONLY to definitions of variables that are only * used in assert-enabled builds, to avoid compiler warnings about unused diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index efcbad7842..be691c5e9c 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -303,7 +303,7 @@ extern void DefineCustomBoolVariable(const char *name, int flags, GucBoolCheckHook check_hook, GucBoolAssignHook assign_hook, - GucShowHook show_hook); + GucShowHook show_hook) pg_attribute_nonnull(1, 4); extern void DefineCustomIntVariable(const char *name, const char *short_desc, @@ -316,7 +316,7 @@ extern void DefineCustomIntVariable(const char *name, int flags, GucIntCheckHook check_hook, GucIntAssignHook assign_hook, - GucShowHook show_hook); + GucShowHook show_hook) pg_attribute_nonnull(1, 4); extern void DefineCustomRealVariable(const char *name, const char *short_desc, @@ -329,7 +329,7 @@ extern void DefineCustomRealVariable(const char *name, int flags, GucRealCheckHook check_hook, GucRealAssignHook assign_hook, - GucShowHook show_hook); + GucShowHook show_hook) pg_attribute_nonnull(1, 4); extern void DefineCustomStringVariable(const char *name, const char *short_desc, @@ -340,7 +340,7 @@ extern void DefineCustomStringVariable(const char *name, int flags, GucStringCheckHook check_hook, GucStringAssignHook assign_hook, - GucShowHook show_hook); + GucShowHook show_hook) pg_attribute_nonnull(1, 4); extern void DefineCustomEnumVariable(const char *name, const char *short_desc, @@ -352,7 +352,7 @@ extern void DefineCustomEnumVariable(const char *name, int flags,
Re: Assert name/short_desc to prevent SHOW ALL segfault
Michael Paquier writes: > And I've learnt today that we enforce a definition of __has_attribute > at the top of c.h, and that we already rely on that. So I agree that > what you are doing in 0002 should be enough. Should we wait until 16~ > opens for business though? I don't see a strong argument to push > forward with that now that we are in beta mode on HEAD. Agreed. This part isn't a bug fix. regards, tom lane
Re: Assert name/short_desc to prevent SHOW ALL segfault
On Fri, May 27, 2022 at 10:43:17AM -0700, Nathan Bossart wrote: > Makes sense. Here's a new patch set. 0001 is the part intended for > back-patching, and 0002 is the rest (i.e., adding pg_attribute_nonnull()). > I switched to using __has_attribute to discover whether nonnull was Okay, I have looked at 0001 this morning and applied it down to 12. The change in GetConfigOptionByNum() is not required in 10 and 11, as the strings of pg_show\all_settings() have begun to be translated in 12~. > supported, as that seemed cleaner. I didn't see any need for a new > configure check, but maybe I am missing something. And I've learnt today that we enforce a definition of __has_attribute at the top of c.h, and that we already rely on that. So I agree that what you are doing in 0002 should be enough. Should we wait until 16~ opens for business though? I don't see a strong argument to push forward with that now that we are in beta mode on HEAD. -- Michael signature.asc Description: PGP signature
Re: Assert name/short_desc to prevent SHOW ALL segfault
On Thu, May 26, 2022 at 11:01:44PM -0400, Tom Lane wrote:
> Michael Paquier writes:
>> FWIW, I would be fine to backpatch the NULL handling for short_desc,
>> while treating the addition of nonnull as a HEAD-only change.
>
> Yeah, sounds about right to me. My guess is that we will need
> a configure check for nonnull, but perhaps I'm wrong.
Makes sense. Here's a new patch set. 0001 is the part intended for
back-patching, and 0002 is the rest (i.e., adding pg_attribute_nonnull()).
I switched to using __has_attribute to discover whether nonnull was
supported, as that seemed cleaner. I didn't see any need for a new
configure check, but maybe I am missing something.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From c2760bf3db292fb55b6448f22eaeee38656083b5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 May 2022 09:48:51 -0700
Subject: [PATCH v4 1/2] Properly handle NULL short descriptions for custom
variables.
If a NULL short description is specified in one of the
DefineCustomXXXVariable functions, SHOW ALL will segfault. This
change teaches SHOW ALL to properly handle NULL short descriptions.
Back-patch to all supported versions.
Reported by: Steve Chavez
Author: Steve Chavez
Reviewed by: Nathan Bossart, Michael Paquier, Andred Freund, Tom Lane
Discussion: https://postgr.es/m/CAGRrpzY6hO-Kmykna_XvsTv8P2DshGiU6G3j8yGao4mk0CqjHA%40mail.gmail.com
---
src/backend/utils/misc/guc.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8e9b71375c..55d41ae7d6 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9780,7 +9780,16 @@ ShowAllGUCConfig(DestReceiver *dest)
isnull[1] = true;
}
- values[2] = PointerGetDatum(cstring_to_text(conf->short_desc));
+ if (conf->short_desc)
+ {
+ values[2] = PointerGetDatum(cstring_to_text(conf->short_desc));
+ isnull[2] = false;
+ }
+ else
+ {
+ values[2] = PointerGetDatum(NULL);
+ isnull[2] = true;
+ }
/* send it to dest */
do_tup_output(tstate, values, isnull);
@@ -9792,7 +9801,8 @@ ShowAllGUCConfig(DestReceiver *dest)
pfree(setting);
pfree(DatumGetPointer(values[1]));
}
- pfree(DatumGetPointer(values[2]));
+ if (conf->short_desc)
+ pfree(DatumGetPointer(values[2]));
}
end_tup_output(tstate);
@@ -10002,7 +10012,7 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow)
values[3] = _(config_group_names[conf->group]);
/* short_desc */
- values[4] = _(conf->short_desc);
+ values[4] = conf->short_desc != NULL ? _(conf->short_desc) : NULL;
/* extra_desc */
values[5] = conf->long_desc != NULL ? _(conf->long_desc) : NULL;
--
2.25.1
>From bd05e7092f45f416476cc02a21f8d0bb069a313f Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 May 2022 10:10:09 -0700
Subject: [PATCH v4 2/2] Introduce pg_attribute_nonnull() and use it for
DefineCustomXXXVariable().
pg_attribute_nonnull() can be used to generate compiler warnings
when a function is called with the specified arguments set to NULL.
An empty argument list indicates that all pointer arguments must
not be NULL. pg_attribute_nonnull() only works for compilers that
support the nonnull function attribute. If nonnull is not
supported, pg_attribute_nonnull() has no effect.
In addition to introducing pg_attribute_nonnull(), this change uses
it for the DefineCustomXXXVariable() functions to generate warnings
when the "name" and "valueAddr" arguments are set to NULL.
Idea by: Andres Freund
Author: Nathan Bossart
Reviewed by: Michael Paquier, Tom Lane
Discussion: https://postgr.es/m/20220525061739.ur7x535vtzyzkmqo%40alap3.anarazel.de
---
src/include/c.h | 11 +++
src/include/utils/guc.h | 10 +-
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/include/c.h b/src/include/c.h
index 4f16e589b3..0c64557c62 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -144,6 +144,17 @@
#define pg_attribute_no_sanitize_alignment()
#endif
+/*
+ * pg_attribute_nonnull means the compiler should warn if the function is called
+ * with the listed arguments set to NULL. If no arguments are listed, the
+ * compiler should warn if any pointer arguments are set to NULL.
+ */
+#if __has_attribute (nonnull)
+#define pg_attribute_nonnull(...) __attribute__((nonnull(__VA_ARGS__)))
+#else
+#define pg_attribute_nonnull(...)
+#endif
+
/*
* Append PG_USED_FOR_ASSERTS_ONLY to definitions of variables that are only
* used in assert-enabled builds, to avoid compiler warnings about unused
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index efcbad7842..be691c5e9c 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -303,7 +303,7 @@ extern void DefineCustomBoolVariable(const char *name,
int flags,
GucBoolCheckHook check_hook,
GucBoolAssignHook assign_hook,
- GucShowHook show_hook);
+
Re: Assert name/short_desc to prevent SHOW ALL segfault
Michael Paquier writes: > FWIW, I would be fine to backpatch the NULL handling for short_desc, > while treating the addition of nonnull as a HEAD-only change. Yeah, sounds about right to me. My guess is that we will need a configure check for nonnull, but perhaps I'm wrong. regards, tom lane
Re: Assert name/short_desc to prevent SHOW ALL segfault
On Thu, May 26, 2022 at 02:45:50PM -0700, Nathan Bossart wrote: > On Tue, May 24, 2022 at 11:17:39PM -0700, Andres Freund wrote: >> On 2022-05-24 11:41:49 -0700, Nathan Bossart wrote: >>> I would actually ERROR on this so that we aren't relying on >>> --enable-cassert builds to catch it. >> >> How about adding pg_nonnull(...) (ending up as __attribute__((nonnull(...))? >> Then code passing NULLs would get compiler warnings? It'd be useful in quite >> a >> few more places. > > I attached an attempt at this for the "name" and "valueAddr" arguments for > the DefineCustomXXXVariable functions. It looked like nonnull was > supported by GCC and Clang, but I haven't looked too closely to see whether > we need version checks as well. Adding pg_attribute_nonnull() is a neat idea. It looks like nonnull was added in GCC 4.0, and I can see it first appearing in clang 3.7. The only buildfarm member claiming to use a version of clang older than that is dangomushi, aka 3.6.0. That's my machine, and the information on the buildfarm website is outdated as the version of clang available there is 13.0 as of today. I think that we are going to run into problems with buildfarm member protosciurus, running Solaris 10 under sparc? It claims to use gcc 3.4.3. I would worry also about prairiedog, we've hard our share of compatibility issues with this one in the past. It claims to use gcc 4.0.1 but Apple has its own idea of versioning, and that's our oldest macos member. >>> That being said, if there's no strong reason to enforce that a short >>> description be provided, then why not adjust ShowAllGUCConfig() to set that >>> column to NULL when short_desc is missing? >> >> There's a bunch more places that'd need to be adjusted, if we go that way. I >> don't really have an opinion on it. > > I looked around and didn't see anywhere else obvious that needed adjustment > besides what Michael pointed out (3ac7d024). Am I missing > something? I don't think so. help_config.c is able to handle NULL for the short description already. FWIW, I would be fine to backpatch the NULL handling for short_desc, while treating the addition of nonnull as a HEAD-only change. -- Michael signature.asc Description: PGP signature
Re: Assert name/short_desc to prevent SHOW ALL segfault
On Tue, May 24, 2022 at 11:17:39PM -0700, Andres Freund wrote:
> On 2022-05-24 11:41:49 -0700, Nathan Bossart wrote:
>> I would actually ERROR on this so that we aren't relying on
>> --enable-cassert builds to catch it.
>
> How about adding pg_nonnull(...) (ending up as __attribute__((nonnull(...))?
> Then code passing NULLs would get compiler warnings? It'd be useful in quite a
> few more places.
I attached an attempt at this for the "name" and "valueAddr" arguments for
the DefineCustomXXXVariable functions. It looked like nonnull was
supported by GCC and Clang, but I haven't looked too closely to see whether
we need version checks as well.
>> That being said, if there's no strong reason to enforce that a short
>> description be provided, then why not adjust ShowAllGUCConfig() to set that
>> column to NULL when short_desc is missing?
>
> There's a bunch more places that'd need to be adjusted, if we go that way. I
> don't really have an opinion on it.
I looked around and didn't see anywhere else obvious that needed adjustment
besides what Michael pointed out (3ac7d024). Am I missing something?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 3c2503a1794d40c515f53e632952d59317015d02 Mon Sep 17 00:00:00 2001
From: Steve Chavez
Date: Tue, 24 May 2022 23:46:40 -0500
Subject: [PATCH v3 1/1] Handle a NULL short_desc in ShowAllGUCConfig
Prevent a segfault when using a SHOW ALL, in case a DefineCustomXXXVariable()
was defined with a NULL short_desc.
---
src/backend/utils/misc/guc.c | 16 +---
src/include/c.h | 10 ++
src/include/utils/guc.h | 10 +-
3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8e9b71375c..55d41ae7d6 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9780,7 +9780,16 @@ ShowAllGUCConfig(DestReceiver *dest)
isnull[1] = true;
}
- values[2] = PointerGetDatum(cstring_to_text(conf->short_desc));
+ if (conf->short_desc)
+ {
+ values[2] = PointerGetDatum(cstring_to_text(conf->short_desc));
+ isnull[2] = false;
+ }
+ else
+ {
+ values[2] = PointerGetDatum(NULL);
+ isnull[2] = true;
+ }
/* send it to dest */
do_tup_output(tstate, values, isnull);
@@ -9792,7 +9801,8 @@ ShowAllGUCConfig(DestReceiver *dest)
pfree(setting);
pfree(DatumGetPointer(values[1]));
}
- pfree(DatumGetPointer(values[2]));
+ if (conf->short_desc)
+ pfree(DatumGetPointer(values[2]));
}
end_tup_output(tstate);
@@ -10002,7 +10012,7 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow)
values[3] = _(config_group_names[conf->group]);
/* short_desc */
- values[4] = _(conf->short_desc);
+ values[4] = conf->short_desc != NULL ? _(conf->short_desc) : NULL;
/* extra_desc */
values[5] = conf->long_desc != NULL ? _(conf->long_desc) : NULL;
diff --git a/src/include/c.h b/src/include/c.h
index 4f16e589b3..f253aa930b 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -144,6 +144,16 @@
#define pg_attribute_no_sanitize_alignment()
#endif
+/*
+ * pg_attribute_nonnull means the compiler should warn if the function is called
+ * with the listed arguments set to NULL.
+ */
+#if defined(__clang_major__) || defined(__GNUC__)
+#define pg_attribute_nonnull(...) __attribute__((nonnull(__VA_ARGS__)))
+#else
+#define pg_attribute_nonnull(...)
+#endif
+
/*
* Append PG_USED_FOR_ASSERTS_ONLY to definitions of variables that are only
* used in assert-enabled builds, to avoid compiler warnings about unused
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index efcbad7842..be691c5e9c 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -303,7 +303,7 @@ extern void DefineCustomBoolVariable(const char *name,
int flags,
GucBoolCheckHook check_hook,
GucBoolAssignHook assign_hook,
- GucShowHook show_hook);
+ GucShowHook show_hook) pg_attribute_nonnull(1, 4);
extern void DefineCustomIntVariable(const char *name,
const char *short_desc,
@@ -316,7 +316,7 @@ extern void DefineCustomIntVariable(const char *name,
int flags,
GucIntCheckHook check_hook,
GucIntAssignHook assign_hook,
- GucShowHook show_hook);
+ GucShowHook show_hook) pg_attribute_nonnull(1, 4);
extern void DefineCustomRealVariable(const char *name,
const char *short_desc,
@@ -329,7 +329,7 @@ extern void DefineCustomRealVariable(const char *name,
int flags,
GucRealCheckHook check_hook,
GucRealAssignHook assign_hook,
- GucShowHook show_hook);
+ GucShowHook show_hook) pg_attribute_nonnull(1, 4);
extern void DefineCustomStringVariable(const char *name,
const char *short_desc,
@@ -340,7 +340,7 @@ extern void DefineCustomStringVariable(const char *name,
int flags,
GucStringCheckHook check_hook,
Re: Assert name/short_desc to prevent SHOW ALL segfault
Thank you for the reviews Nathan, Michael.
I agree with handling NULL in ShowAllGUCConfig() instead.
I've attached the updated patch.
--
Steve Chavez
Engineering at https://supabase.com/
On Tue, 24 May 2022 at 20:21, Michael Paquier wrote:
> On Tue, May 24, 2022 at 11:41:49AM -0700, Nathan Bossart wrote:
> > I would actually ERROR on this so that we aren't relying on
> > --enable-cassert builds to catch it. That being said, if there's no
> strong
> > reason to enforce that a short description be provided, then why not
> adjust
> > ShowAllGUCConfig() to set that column to NULL when short_desc is missing?
>
> Well, issuing an ERROR on the stable branches would be troublesome for
> extension developers when reloading after a minor update if they did
> not set their short_desc in a custom GUC. So, while I'd like to
> encourage the use of short_desc, using your suggestion to make the
> code more flexible with NULL is fine by me. GetConfigOptionByNum()
> does that for long_desc by the way, meaning that we also have a
> problem there on a build with --enable-nls for short_desc, no?
> --
> Michael
>
From 7b3d1451938f37d08e692a1f252b2f71c1ae5279 Mon Sep 17 00:00:00 2001
From: Steve Chavez
Date: Tue, 24 May 2022 23:46:40 -0500
Subject: [PATCH] Handle a NULL short_desc in ShowAllGUCConfig
Prevent a segfault when using a SHOW ALL, in case a DefineCustomXXXVariable()
was defined with a NULL short_desc.
---
src/backend/utils/misc/guc.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8e9b71375c..d7dd727d45 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9780,7 +9780,16 @@ ShowAllGUCConfig(DestReceiver *dest)
isnull[1] = true;
}
- values[2] = PointerGetDatum(cstring_to_text(conf->short_desc));
+ if (conf->short_desc)
+ {
+ values[2] = PointerGetDatum(cstring_to_text(conf->short_desc));
+ isnull[2] = false;
+ }
+ else
+ {
+ values[2] = PointerGetDatum(NULL);
+ isnull[2] = true;
+ }
/* send it to dest */
do_tup_output(tstate, values, isnull);
@@ -9792,7 +9801,10 @@ ShowAllGUCConfig(DestReceiver *dest)
pfree(setting);
pfree(DatumGetPointer(values[1]));
}
- pfree(DatumGetPointer(values[2]));
+ if (conf->short_desc)
+ {
+ pfree(DatumGetPointer(values[2]));
+ }
}
end_tup_output(tstate);
--
2.32.0
Re: Assert name/short_desc to prevent SHOW ALL segfault
Hi, On 2022-05-24 11:41:49 -0700, Nathan Bossart wrote: > On Mon, May 23, 2022 at 11:39:16PM -0500, Steve Chavez wrote: > > The DefineCustomStringVariable function(or any > > other DefineCustomXXXVariable) has a short_desc parameter that can be > > NULL and it's not apparent that this will lead to a segfault when SHOW ALL > > is used. > > This happens because the ShowAllGUCConfig function expects a non-NULL > > short_desc. > > > > This happened for the Supabase supautils extension( > > https://github.com/supabase/supautils/issues/24) and any other extension > > that uses the DefineCustomXXXVariable has the same bug risk. > > > > This patch does an Assert on the short_desc(also on the name as an extra > > measure), so a postgres built with --enable-cassert can prevent the above > > issue. > > I would actually ERROR on this so that we aren't relying on > --enable-cassert builds to catch it. How about adding pg_nonnull(...) (ending up as __attribute__((nonnull(...))? Then code passing NULLs would get compiler warnings? It'd be useful in quite a few more places. > That being said, if there's no strong reason to enforce that a short > description be provided, then why not adjust ShowAllGUCConfig() to set that > column to NULL when short_desc is missing? There's a bunch more places that'd need to be adjusted, if we go that way. I don't really have an opinion on it. Greetings, Andres Freund
Re: Assert name/short_desc to prevent SHOW ALL segfault
On Wed, May 25, 2022 at 12:05:55AM -0500, Steve Chavez wrote: > Thank you for the reviews Nathan, Michael. > > I agree with handling NULL in ShowAllGUCConfig() instead. > > I've attached the updated patch. Shouldn't the same check as extra_desc be done in GetConfigOptionByNum() for short_desc (point of upthread)? See 3ac7d024, though -fsanitize=undefined does not apply here, I guess. -- Michael signature.asc Description: PGP signature
Re: Assert name/short_desc to prevent SHOW ALL segfault
On Tue, May 24, 2022 at 11:41:49AM -0700, Nathan Bossart wrote: > I would actually ERROR on this so that we aren't relying on > --enable-cassert builds to catch it. That being said, if there's no strong > reason to enforce that a short description be provided, then why not adjust > ShowAllGUCConfig() to set that column to NULL when short_desc is missing? Well, issuing an ERROR on the stable branches would be troublesome for extension developers when reloading after a minor update if they did not set their short_desc in a custom GUC. So, while I'd like to encourage the use of short_desc, using your suggestion to make the code more flexible with NULL is fine by me. GetConfigOptionByNum() does that for long_desc by the way, meaning that we also have a problem there on a build with --enable-nls for short_desc, no? -- Michael signature.asc Description: PGP signature
Re: Assert name/short_desc to prevent SHOW ALL segfault
On Mon, May 23, 2022 at 11:39:16PM -0500, Steve Chavez wrote: > The DefineCustomStringVariable function(or any > other DefineCustomXXXVariable) has a short_desc parameter that can be > NULL and it's not apparent that this will lead to a segfault when SHOW ALL > is used. > This happens because the ShowAllGUCConfig function expects a non-NULL > short_desc. > > This happened for the Supabase supautils extension( > https://github.com/supabase/supautils/issues/24) and any other extension > that uses the DefineCustomXXXVariable has the same bug risk. > > This patch does an Assert on the short_desc(also on the name as an extra > measure), so a postgres built with --enable-cassert can prevent the above > issue. I would actually ERROR on this so that we aren't relying on --enable-cassert builds to catch it. That being said, if there's no strong reason to enforce that a short description be provided, then why not adjust ShowAllGUCConfig() to set that column to NULL when short_desc is missing? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Assert name/short_desc to prevent SHOW ALL segfault
Hello hackers,
The DefineCustomStringVariable function(or any
other DefineCustomXXXVariable) has a short_desc parameter that can be
NULL and it's not apparent that this will lead to a segfault when SHOW ALL
is used.
This happens because the ShowAllGUCConfig function expects a non-NULL
short_desc.
This happened for the Supabase supautils extension(
https://github.com/supabase/supautils/issues/24) and any other extension
that uses the DefineCustomXXXVariable has the same bug risk.
This patch does an Assert on the short_desc(also on the name as an extra
measure), so a postgres built with --enable-cassert can prevent the above
issue.
---
Steve Chavez
Engineering at https://supabase.com/
From ad8a61125a0fe33853d459f12e521dff771130d4 Mon Sep 17 00:00:00 2001
From: Steve Chavez
Date: Thu, 19 May 2022 08:59:46 -0500
Subject: [PATCH] Assert name/short_desc to prevent SHOWALL segfault
Every DefineCustomXXXVariable function can have a NULL short_desc,
and it's not apparent that this will lead to a segfault when SHOW ALL is
used. This happens because ShowAllGUCConfig expects a non-NULL
short_desc.
Assertions are added to init_custom_variable to ensure name and
short_desc are present at minimum.
---
src/backend/utils/misc/guc.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8e9b71375c..635d39b7d4 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9239,6 +9239,10 @@ init_custom_variable(const char *name,
{
struct config_generic *gen;
+ /* Ensure at least the name and short description are present */
+ Assert(name != NULL);
+ Assert(short_desc != NULL);
+
/*
* Only allow custom PGC_POSTMASTER variables to be created during shared
* library preload; any later than that, we can't ensure that the value
--
2.32.0
