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]https://github.com/higuoxing/guc_crash/tree/pg. Patch for fixing it is attatched.
[1]: https://github.com/higuoxing/guc_crash/tree/pg
--
Best Regards,
Xing
Attachments:
0001-Don-t-use-strcmp-with-nullable-pointers.patchtext/x-patch; charset=US-ASCII; name=0001-Don-t-use-strcmp-with-nullable-pointers.patchDownload
From dcd7a49190f0e19ba0a1e697cac45724450f6365 Mon Sep 17 00:00:00 2001
From: Xing Guo <higuoxing@gmail.com>
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
On Wed, Nov 1, 2023 at 5:25 PM Xing Guo <higuoxing@gmail.com> 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.
Can we set a string guc to NULL? If not, `*lconf->variable == NULL` would
be unnecessary.
--
Best Regards,
--
Regards
Junwang Zhao
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.
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
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 <aleksander@timescale.com>
wrote:
Show quoted text
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.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
Attachments:
v2-0001-Don-t-use-strcmp-with-nullable-pointers.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Don-t-use-strcmp-with-nullable-pointers.patchDownload
From bf0a9f848e69097a034692ed7b0be0ad81a5d991 Mon Sep 17 00:00:00 2001
From: Xing Guo <higuoxing@gmail.com>
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 : "<null>", *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
Xing Guo <higuoxing@gmail.com> 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
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 <tgl@sss.pgh.pa.us> wrote:
Show quoted text
Xing Guo <higuoxing@gmail.com> 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
Xing Guo <higuoxing@gmail.com> 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
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
Attachments:
v3-0001-Don-t-use-strcmp-with-nullable-pointers.patchtext/x-diff; charset=us-ascii; name=v3-0001-Don-t-use-strcmp-with-nullable-pointers.patchDownload
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 : "<null>", *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 config_generic gen;
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 <tgl@sss.pgh.pa.us> 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
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
Nathan Bossart <nathandbossart@gmail.com> 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
On Wed, Nov 01, 2023 at 10:39:04PM -0400, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> 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
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 <nathandbossart@gmail.com> wrote:
On Wed, Nov 01, 2023 at 10:39:04PM -0400, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> 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
Attachments:
v3-0001-Consider-treating-NULL-is-a-valid-boot_val-for-st.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Consider-treating-NULL-is-a-valid-boot_val-for-st.patchDownload
From d620d3a2b44cef63024baca4ccdf55bf7724737f Mon Sep 17 00:00:00 2001
From: Xing Guo <higuoxing@gmail.com>
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 : "<null>", *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 config_generic gen;
--
2.42.0
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