Re: Don't pass NULL pointer to strcmp().
Looking closer, I realized that my proposed change in RestoreGUCState is unnecessary, because guc_free() is already permissive about being passed a NULL. That leaves us with one live bug in get_explain_guc_options, two probably-unreachable hazards in check_GUC_init and write_one_nondefault_variable, and two API changes in GetConfigOption and GetConfigOptionResetString. I'm dubious that back-patching the API changes would be a good idea, so I applied that to HEAD only. The rest I backpatched as far as relevant. Thanks for the report! regards, tom lane
Re: Don't pass NULL pointer to strcmp().
Hi,
Seems that Tom's patch cannot be applied to the current master branch.
I just re-generate the patch for others to play with.
On 11/2/23, Nathan Bossart wrote:
> On Wed, Nov 01, 2023 at 10:39:04PM -0400, Tom Lane wrote:
>> Nathan Bossart writes:
>>> What if we disallowed NULL string GUCs in v17?
>>
>> Well, we'd need to devise some other solution for hacks like the
>> one used by timezone_abbreviations (see comment in
>> check_timezone_abbreviations). I think it's not worth the trouble,
>> especially seeing that 95% of guc.c is already set up for this.
>> The bugs are mostly in newer code like get_explain_guc_options,
>> and I think that's directly traceable to the lack of any comments
>> or docs about this behavior.
>
> Eh, yeah, it's probably not worth it if we find ourselves trading one set
> of hacks for another.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>
--
Best Regards,
Xing
From d620d3a2b44cef63024baca4ccdf55bf7724737f Mon Sep 17 00:00:00 2001
From: Xing Guo
Date: Thu, 2 Nov 2023 16:42:00 +0800
Subject: [PATCH v3] Consider treating NULL is a valid boot_val for string
GUCs.
---
src/backend/utils/misc/guc.c | 28
src/include/utils/guc_tables.h | 10 ++
2 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..28056af19c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1473,7 +1473,9 @@ check_GUC_init(struct config_generic *gconf)
{
struct config_string *conf = (struct config_string *) gconf;
-if (*conf->variable != NULL && strcmp(*conf->variable, conf->boot_val) != 0)
+if (*conf->variable != NULL &&
+ (conf->boot_val == NULL ||
+ strcmp(*conf->variable, conf->boot_val) != 0))
{
elog(LOG, "GUC (PGC_STRING) %s, boot_val=%s, C-var=%s",
conf->gen.name, conf->boot_val ? conf->boot_val : "", *conf->variable);
@@ -4213,8 +4215,7 @@ SetConfigOption(const char *name, const char *value,
/*
* Fetch the current value of the option `name', as a string.
*
- * If the option doesn't exist, return NULL if missing_ok is true (NOTE that
- * this cannot be distinguished from a string variable with a NULL value!),
+ * If the option doesn't exist, return NULL if missing_ok is true,
* otherwise throw an ereport and don't return.
*
* If restrict_privileged is true, we also enforce that only superusers and
@@ -4257,7 +4258,8 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged)
return buffer;
case PGC_STRING:
- return *((struct config_string *) record)->variable;
+ return *((struct config_string *) record)->variable ?
+*((struct config_string *) record)->variable : "";
case PGC_ENUM:
return config_enum_lookup_by_value((struct config_enum *) record,
@@ -4304,7 +4306,8 @@ GetConfigOptionResetString(const char *name)
return buffer;
case PGC_STRING:
- return ((struct config_string *) record)->reset_val;
+ return ((struct config_string *) record)->reset_val ?
+((struct config_string *) record)->reset_val : "";
case PGC_ENUM:
return config_enum_lookup_by_value((struct config_enum *) record,
@@ -5255,7 +5258,14 @@ get_explain_guc_options(int *num)
{
struct config_string *lconf = (struct config_string *) conf;
- modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+ if (lconf->boot_val == NULL &&
+ *lconf->variable == NULL)
+ modified = false;
+ else if (lconf->boot_val == NULL ||
+ *lconf->variable == NULL)
+ modified = true;
+ else
+ modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
}
break;
@@ -5482,7 +5492,8 @@ write_one_nondefault_variable(FILE *fp, struct config_generic *gconf)
{
struct config_string *conf = (struct config_string *) gconf;
-fprintf(fp, "%s", *conf->variable);
+if (*conf->variable)
+ fprintf(fp, "%s", *conf->variable);
}
break;
@@ -6142,7 +6153,8 @@ RestoreGUCState(void *gucstate)
{
struct config_string *conf = (struct config_string *) gconf;
- guc_free(*conf->variable);
+ if (*conf->variable)
+ guc_free(*conf->variable);
if (conf->reset_val && conf->reset_val != *conf->variable)
guc_free(conf->reset_val);
if (conf->reset_extra && conf->reset_extra != gconf->extra)
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 1ec9575570..0c38255961 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -240,6 +240,16 @@ struct config_real
void *reset_extra;
};
+/*
+ * A note about string GUCs: the boot_val is allowed to be NULL, which leads
+ * to the reset_val and the actual variable value (*variable) also being NULL.
+ * However, there is no way to set a NULL value subsequently using
+ * set_config_option or any other GUC API. Also, GUC APIs such as SHOW will
Re: Don't pass NULL pointer to strcmp().
On Wed, Nov 01, 2023 at 10:39:04PM -0400, Tom Lane wrote: > Nathan Bossart writes: >> What if we disallowed NULL string GUCs in v17? > > Well, we'd need to devise some other solution for hacks like the > one used by timezone_abbreviations (see comment in > check_timezone_abbreviations). I think it's not worth the trouble, > especially seeing that 95% of guc.c is already set up for this. > The bugs are mostly in newer code like get_explain_guc_options, > and I think that's directly traceable to the lack of any comments > or docs about this behavior. Eh, yeah, it's probably not worth it if we find ourselves trading one set of hacks for another. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Don't pass NULL pointer to strcmp().
Nathan Bossart writes: > On Wed, Nov 01, 2023 at 09:57:18PM -0400, Tom Lane wrote: >> After digging around for a bit, I think part of the problem is a lack >> of a clearly defined spec for what should happen with NULL string GUCs. > What if we disallowed NULL string GUCs in v17? Well, we'd need to devise some other solution for hacks like the one used by timezone_abbreviations (see comment in check_timezone_abbreviations). I think it's not worth the trouble, especially seeing that 95% of guc.c is already set up for this. The bugs are mostly in newer code like get_explain_guc_options, and I think that's directly traceable to the lack of any comments or docs about this behavior. regards, tom lane
Re: Don't pass NULL pointer to strcmp().
On Wed, Nov 01, 2023 at 09:57:18PM -0400, Tom Lane wrote: > I wrote: >> Hmm ... if we're doing it ourselves, I suppose we've got to consider >> it supported :-(. But I'm still wondering how many seldom-used >> code paths didn't get the message. An example here is that this >> could lead to GetConfigOptionResetString returning NULL, which >> I think is outside its admittedly-vague API spec. > > After digging around for a bit, I think part of the problem is a lack > of a clearly defined spec for what should happen with NULL string GUCs. > In the attached v3, I attempted to remedy that by adding a comment in > guc_tables.h (which is maybe not the best place but I didn't see a > better one). That led me to a couple more changes beyond what you had. What if we disallowed NULL string GUCs in v17? That'd simplify the spec and future-proof against similar bugs, but it might also break a fair number of extensions. If there aren't any other reasons to continue supporting it, maybe it's the right long-term approach, though. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Don't pass NULL pointer to strcmp().
Thank you Tom! Your comment "NULL doesn't have semantics that are visibly different from an empty string" is exactly what I want to confirm :-) On 11/2/23, Tom Lane wrote: > I wrote: >> Hmm ... if we're doing it ourselves, I suppose we've got to consider >> it supported :-(. But I'm still wondering how many seldom-used >> code paths didn't get the message. An example here is that this >> could lead to GetConfigOptionResetString returning NULL, which >> I think is outside its admittedly-vague API spec. > > After digging around for a bit, I think part of the problem is a lack > of a clearly defined spec for what should happen with NULL string GUCs. > In the attached v3, I attempted to remedy that by adding a comment in > guc_tables.h (which is maybe not the best place but I didn't see a > better one). That led me to a couple more changes beyond what you had. > > It's possible that some of these are unreachable --- for example, > given that a NULL could only be the default value, I'm not sure that > the fix in write_one_nondefault_variable is a live bug. But we ought > to code all this stuff defensively, and most of it already was > NULL-safe. > > regards, tom lane > > -- Best Regards, Xing
Re: Don't pass NULL pointer to strcmp().
I wrote:
> Hmm ... if we're doing it ourselves, I suppose we've got to consider
> it supported :-(. But I'm still wondering how many seldom-used
> code paths didn't get the message. An example here is that this
> could lead to GetConfigOptionResetString returning NULL, which
> I think is outside its admittedly-vague API spec.
After digging around for a bit, I think part of the problem is a lack
of a clearly defined spec for what should happen with NULL string GUCs.
In the attached v3, I attempted to remedy that by adding a comment in
guc_tables.h (which is maybe not the best place but I didn't see a
better one). That led me to a couple more changes beyond what you had.
It's possible that some of these are unreachable --- for example,
given that a NULL could only be the default value, I'm not sure that
the fix in write_one_nondefault_variable is a live bug. But we ought
to code all this stuff defensively, and most of it already was
NULL-safe.
regards, tom lane
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..ccffaaad02 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1473,7 +1473,9 @@ check_GUC_init(struct config_generic *gconf)
{
struct config_string *conf = (struct config_string *) gconf;
-if (*conf->variable != NULL && strcmp(*conf->variable, conf->boot_val) != 0)
+if (*conf->variable != NULL &&
+ (conf->boot_val == NULL ||
+ strcmp(*conf->variable, conf->boot_val) != 0))
{
elog(LOG, "GUC (PGC_STRING) %s, boot_val=%s, C-var=%s",
conf->gen.name, conf->boot_val ? conf->boot_val : "", *conf->variable);
@@ -4213,8 +4215,7 @@ SetConfigOption(const char *name, const char *value,
/*
* Fetch the current value of the option `name', as a string.
*
- * If the option doesn't exist, return NULL if missing_ok is true (NOTE that
- * this cannot be distinguished from a string variable with a NULL value!),
+ * If the option doesn't exist, return NULL if missing_ok is true,
* otherwise throw an ereport and don't return.
*
* If restrict_privileged is true, we also enforce that only superusers and
@@ -4257,7 +4258,8 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged)
return buffer;
case PGC_STRING:
- return *((struct config_string *) record)->variable;
+ return *((struct config_string *) record)->variable ?
+*((struct config_string *) record)->variable : "";
case PGC_ENUM:
return config_enum_lookup_by_value((struct config_enum *) record,
@@ -4304,7 +4306,8 @@ GetConfigOptionResetString(const char *name)
return buffer;
case PGC_STRING:
- return ((struct config_string *) record)->reset_val;
+ return ((struct config_string *) record)->reset_val ?
+((struct config_string *) record)->reset_val : "";
case PGC_ENUM:
return config_enum_lookup_by_value((struct config_enum *) record,
@@ -5255,7 +5258,14 @@ get_explain_guc_options(int *num)
{
struct config_string *lconf = (struct config_string *) conf;
- modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+ if (lconf->boot_val == NULL &&
+ *lconf->variable == NULL)
+ modified = false;
+ else if (lconf->boot_val == NULL ||
+ *lconf->variable == NULL)
+ modified = true;
+ else
+ modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
}
break;
@@ -5482,7 +5492,8 @@ write_one_nondefault_variable(FILE *fp, struct config_generic *gconf)
{
struct config_string *conf = (struct config_string *) gconf;
-fprintf(fp, "%s", *conf->variable);
+if (*conf->variable)
+ fprintf(fp, "%s", *conf->variable);
}
break;
@@ -6142,7 +6153,8 @@ RestoreGUCState(void *gucstate)
{
struct config_string *conf = (struct config_string *) gconf;
- guc_free(*conf->variable);
+ if (*conf->variable)
+ guc_free(*conf->variable);
if (conf->reset_val && conf->reset_val != *conf->variable)
guc_free(conf->reset_val);
if (conf->reset_extra && conf->reset_extra != gconf->extra)
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 1ec9575570..0c38255961 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -240,6 +240,16 @@ struct config_real
void *reset_extra;
};
+/*
+ * A note about string GUCs: the boot_val is allowed to be NULL, which leads
+ * to the reset_val and the actual variable value (*variable) also being NULL.
+ * However, there is no way to set a NULL value subsequently using
+ * set_config_option or any other GUC API. Also, GUC APIs such as SHOW will
+ * display a NULL value as an empty string. Callers that choose to use a NULL
+ * boot_val should overwrite the setting later in startup, or else be careful
+ * that NULL doesn't have semantics that are visibly different from an empty
+ * string.
+ */
struct config_string
{
struct
Re: Don't pass NULL pointer to strcmp().
Xing Guo writes: > There're extensions that set their boot_val to NULL. E.g., postgres_fdw Hmm ... if we're doing it ourselves, I suppose we've got to consider it supported :-(. But I'm still wondering how many seldom-used code paths didn't get the message. An example here is that this could lead to GetConfigOptionResetString returning NULL, which I think is outside its admittedly-vague API spec. regards, tom lane
Re: Don't pass NULL pointer to strcmp().
Hi Tom, There're extensions that set their boot_val to NULL. E.g., postgres_fdw ( https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/contrib/postgres_fdw/option.c#L582), plperl ( https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/plperl/plperl.c#L422C13-L422C13, https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/plperl/plperl.c#L444C12-L444C12, https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/plperl/plperl.c#L452C6-L452C6) (Can we treat plperl as an extension?), pltcl ( https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/tcl/pltcl.c#L465C14-L465C14, https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/tcl/pltcl.c#L472C12-L472C12 ). TBH, I don't know if NULL is a valid boot_val for string variables, I just came across some extensions that use NULL as their boot_val. If the boot_val can't be NULL in extensions, we should probably add some assertions or comments about it? Best Regards, Xing On Wed, Nov 1, 2023 at 11:30 PM Tom Lane wrote: > Xing Guo writes: > > Thanks for your comments. I have updated the patch accordingly. > > I'm leery of accepting this patch, as I see no reason that we > should consider it valid for an extension to have a string GUC > with a boot_val of NULL. > > I realize that we have a few core GUCs that are like that, but > I'm pretty sure that every one of them has special-case code > that initializes the GUC to something non-null a bit later on > in startup. I don't think there are any cases where a string > GUC's persistent value will be null, and I don't like the > idea of considering that to be an allowed case. It would > open the door to more crash situations, and it brings up the > old question of how could a user tell NULL from empty string > (via SHOW or current_setting() or whatever). Besides, what's > the benefit really? > > regards, tom lane >
Re: Don't pass NULL pointer to strcmp().
Xing Guo writes: > Thanks for your comments. I have updated the patch accordingly. I'm leery of accepting this patch, as I see no reason that we should consider it valid for an extension to have a string GUC with a boot_val of NULL. I realize that we have a few core GUCs that are like that, but I'm pretty sure that every one of them has special-case code that initializes the GUC to something non-null a bit later on in startup. I don't think there are any cases where a string GUC's persistent value will be null, and I don't like the idea of considering that to be an allowed case. It would open the door to more crash situations, and it brings up the old question of how could a user tell NULL from empty string (via SHOW or current_setting() or whatever). Besides, what's the benefit really? regards, tom lane
Re: Don't pass NULL pointer to strcmp().
Hi Aleksander and Junwang,
Thanks for your comments. I have updated the patch accordingly.
Best Regards,
Xing
On Wed, Nov 1, 2023 at 7:44 PM Aleksander Alekseev
wrote:
> Hi,
>
> > > I found that there's a nullable pointer being passed to strcmp() and
> > > can make the server crash. It can be reproduced on the latest master
> > > branch by crafting an extension[1]. Patch for fixing it is attatched.
> > >
> > > [1] https://github.com/higuoxing/guc_crash/tree/pg
>
> Thanks for reporting. I can confirm that the issue reproduces on the
> `master` branch and the proposed patch fixes it.
>
> > Can we set a string guc to NULL? If not, `*lconf->variable == NULL` would
> > be unnecessary.
>
> Judging by the rest of the code we better keep it, at least for consistenc.
>
> I see one more place with a similar code in guc.c around line 1472.
> Although I don't have exact steps to trigger a crash I suggest adding
> a similar check there.
>
> --
> Best regards,
> Aleksander Alekseev
>
From bf0a9f848e69097a034692ed7b0be0ad81a5d991 Mon Sep 17 00:00:00 2001
From: Xing Guo
Date: Wed, 1 Nov 2023 21:00:13 +0800
Subject: [PATCH v2] Don't use strcmp() with nullable pointers.
Passing a NULL pointer to strcmp() is an undefined behavior. It can make
the PostgreSQL server crash. This patch helps fix it.
---
src/backend/utils/misc/guc.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..1d62523412 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1473,7 +1473,8 @@ check_GUC_init(struct config_generic *gconf)
{
struct config_string *conf = (struct config_string *) gconf;
-if (*conf->variable != NULL && strcmp(*conf->variable, conf->boot_val) != 0)
+if (*conf->variable != NULL &&
+ (conf->boot_val == NULL || strcmp(*conf->variable, conf->boot_val) != 0))
{
elog(LOG, "GUC (PGC_STRING) %s, boot_val=%s, C-var=%s",
conf->gen.name, conf->boot_val ? conf->boot_val : "", *conf->variable);
@@ -5255,7 +5256,9 @@ get_explain_guc_options(int *num)
{
struct config_string *lconf = (struct config_string *) conf;
- modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+ modified = (lconf->boot_val == NULL ||
+*lconf->variable == NULL ||
+strcmp(lconf->boot_val, *(lconf->variable)) != 0);
}
break;
--
2.42.0
Re: Don't pass NULL pointer to strcmp().
Hi, > > I found that there's a nullable pointer being passed to strcmp() and > > can make the server crash. It can be reproduced on the latest master > > branch by crafting an extension[1]. Patch for fixing it is attatched. > > > > [1] https://github.com/higuoxing/guc_crash/tree/pg Thanks for reporting. I can confirm that the issue reproduces on the `master` branch and the proposed patch fixes it. > Can we set a string guc to NULL? If not, `*lconf->variable == NULL` would > be unnecessary. Judging by the rest of the code we better keep it, at least for consistenc. I see one more place with a similar code in guc.c around line 1472. Although I don't have exact steps to trigger a crash I suggest adding a similar check there. -- Best regards, Aleksander Alekseev
Re: Don't pass NULL pointer to strcmp().
On Wed, Nov 1, 2023 at 5:25 PM Xing Guo wrote: > > Hi hackers, > > I found that there's a nullable pointer being passed to strcmp() and > can make the server crash. It can be reproduced on the latest master > branch by crafting an extension[1]. Patch for fixing it is attatched. > > [1] https://github.com/higuoxing/guc_crash/tree/pg > Can we set a string guc to NULL? If not, `*lconf->variable == NULL` would be unnecessary. > -- > Best Regards, > Xing -- Regards Junwang Zhao
Don't pass NULL pointer to strcmp().
Hi hackers,
I found that there's a nullable pointer being passed to strcmp() and
can make the server crash. It can be reproduced on the latest master
branch by crafting an extension[1]. Patch for fixing it is attatched.
[1] https://github.com/higuoxing/guc_crash/tree/pg
--
Best Regards,
Xing
From dcd7a49190f0e19ba0a1e697cac45724450f6365 Mon Sep 17 00:00:00 2001
From: Xing Guo
Date: Wed, 1 Nov 2023 16:41:49 +0800
Subject: [PATCH] Don't use strcmp() with nullable pointers.
Passing a NULL pointer to strcmp() is an undefined behavior. It can make
the PostgreSQL server crash. This patch helps fix it.
---
src/backend/utils/misc/guc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..b277c48925 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5255,7 +5255,9 @@ get_explain_guc_options(int *num)
{
struct config_string *lconf = (struct config_string *) conf;
- modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+ modified = (lconf->boot_val == NULL ||
+*lconf->variable == NULL ||
+strcmp(lconf->boot_val, *(lconf->variable)) != 0);
}
break;
--
2.42.0
