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