Thread: 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

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

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

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

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

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

Michael Paquier <michael@paquier.xyz> 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

On Thu, May 26, 2022 at 11:01:44PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> 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

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

Michael Paquier <michael@paquier.xyz> 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

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

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

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