Re: [HACKERS] [sqlsmith] Crash on GUC serialization
Robert Haaswrites: > On Sat, Nov 19, 2016 at 12:31 PM, Tom Lane wrote: >> Thanks for the report! Looks like the serialization code has overlooked >> the fact that string-valued GUCs can be NULL. Surprising we didn't >> find that before ... > Why do we allow this, anyway? I think it simplifies some initialization cases. Not sure how hard it would be to remove that. But this isn't the first such bug ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [sqlsmith] Crash on GUC serialization
On Sat, Nov 19, 2016 at 12:31 PM, Tom Lanewrote: > Andreas Seltenreich writes: >> sqlsmith just made a GUC that tricks the serialization code into >> dereferencing a nullpointer. Here's a recipe: > > Thanks for the report! Looks like the serialization code has overlooked > the fact that string-valued GUCs can be NULL. Surprising we didn't > find that before ... I had no idea that was possible. I think if we'd had a regression test that set a GUC to such a value, I would have caught this in the raft of bug fixes I submitted around December of last year, so maybe we should add one. Why do we allow this, anyway? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [sqlsmith] Crash on GUC serialization
Michael Paquierwrites: > On Sat, Nov 19, 2016 at 9:31 AM, Tom Lane wrote: >> Thanks for the report! Looks like the serialization code has overlooked >> the fact that string-valued GUCs can be NULL. Surprising we didn't >> find that before ... > I was half-way through it when you sent your email. It seems to me > that we need to take care only of the case for PGC_STRING, per the > attached. Meh; that might be stopping the crash for you but you didn't consider what to do in serialize_variable. Some machines would dump core there instead, and others would print "(null)" which wouldn't fit in the allocated space. The bigger problem here is that set_config_option cannot be used to set a string GUC's value to NULL --- it will treat value == NULL as a request to set to the reset_val. The best we can do is to convert the value into an empty string, which isn't quite the same thing, though it may be close enough (compare e45e990e4). I'm also pretty unexcited about serializing float variables this way --- assuming that sprintf("%.17g") will reconstruct doubles exactly is just asking for trouble IMO. So I can't escape the itchy feeling that this entire chunk of code needs to be thrown away and rewritten differently. But it'd take some refactoring of the code under set_config_option to do it nicely, which is more work than we probably want to put in now. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [sqlsmith] Crash on GUC serialization
On Sat, Nov 19, 2016 at 9:51 AM, Andreas Seltenreichwrote: > Michael Paquier writes: > >> [2. text/plain; fix-guc-string-eval.patch] > > I'm afraid taking care of the length computation is not sufficient. > ISTM like it'll still try to serialize the NULL pointer later on in > serialize_variable: > > ,[ guc.c:9108 ] > | case PGC_STRING: > | { > | struct config_string *conf = (struct config_string *) gconf; > | do_serialize(destptr, maxbytes, "%s", *conf->variable); > ` Hm, yes. Using an empty string strikes as being the best match. -- Michael diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3c695c1..95f36b8 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -8959,7 +8959,10 @@ estimate_variable_size(struct config_generic * gconf) { struct config_string *conf = (struct config_string *) gconf; - valsize = strlen(*conf->variable); + if (*conf->variable && **conf->variable) + valsize = strlen(*conf->variable); + else + valsize = 0; } break; @@ -9109,7 +9112,9 @@ serialize_variable(char **destptr, Size *maxbytes, { struct config_string *conf = (struct config_string *) gconf; - do_serialize(destptr, maxbytes, "%s", *conf->variable); + do_serialize(destptr, maxbytes, "%s", +*conf->variable && **conf->variable ? +*conf->variable : ""); } break; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [sqlsmith] Crash on GUC serialization
Michael Paquier writes: > [2. text/plain; fix-guc-string-eval.patch] I'm afraid taking care of the length computation is not sufficient. ISTM like it'll still try to serialize the NULL pointer later on in serialize_variable: ,[ guc.c:9108 ] | case PGC_STRING: | { | struct config_string *conf = (struct config_string *) gconf; | do_serialize(destptr, maxbytes, "%s", *conf->variable); ` -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [sqlsmith] Crash on GUC serialization
On Sat, Nov 19, 2016 at 9:31 AM, Tom Lanewrote: > Andreas Seltenreich writes: >> sqlsmith just made a GUC that tricks the serialization code into >> dereferencing a nullpointer. Here's a recipe: > > Thanks for the report! Looks like the serialization code has overlooked > the fact that string-valued GUCs can be NULL. Surprising we didn't > find that before ... I was half-way through it when you sent your email. It seems to me that we need to take care only of the case for PGC_STRING, per the attached. -- Michael diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3c695c1..3316092 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -8959,7 +8959,10 @@ estimate_variable_size(struct config_generic * gconf) { struct config_string *conf = (struct config_string *) gconf; - valsize = strlen(*conf->variable); + if (*conf->variable && **conf->variable) + valsize = strlen(*conf->variable); + else + valsize = 0; } break; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [sqlsmith] Crash on GUC serialization
Andreas Seltenreichwrites: > sqlsmith just made a GUC that tricks the serialization code into > dereferencing a nullpointer. Here's a recipe: Thanks for the report! Looks like the serialization code has overlooked the fact that string-valued GUCs can be NULL. Surprising we didn't find that before ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [sqlsmith] Crash on GUC serialization
Hi, sqlsmith just made a GUC that tricks the serialization code into dereferencing a nullpointer. Here's a recipe: --8<---cut here---start->8--- set min_parallel_relation_size to 0; set max_parallel_workers_per_gather to 2; set force_parallel_mode to on; begin; select set_config('foo.baz', null, b) from (values (false), (true)) g(b); commit; select 1; --8<---cut here---end--->8--- regards, Andreas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers