From 13671b4b22ae4bd345c62e7c0b41d717b8a2e19b Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 19 Nov 2016 14:26:19 -0500 Subject: [PATCH] Code review for GUC serialization/deserialization code. The serialization code dumped core for a string-valued GUC whose value is NULL, which is a legal state. The infrastructure isn't capable of transmitting that state exactly, but fortunately, transmitting an empty string instead should be close enough (compare, eg, commit e45e990e4). The code potentially underestimated the space required to format a real-valued variable, both because it made an unwarranted assumption that %g output would never be longer than %e output, and because it didn't count right even for %e format. In practice this would pretty much always be masked by overestimates for other variables, but it's still wrong. Also fix boundary-case error in read_gucstate, incorrect handling of the case where guc_sourcefile is non-NULL but zero length (not clear that can happen, but if it did, this code would get totally confused), and confusingly useless check for a NULL result from read_gucstate. Andreas Seltenreich discovered the core dump; other issues noted while reading nearby code. Back-patch to 9.5 where this code was introduced. Michael Paquier and Tom Lane Discussion: <871sy78wno.fsf@credativ.de> --- src/backend/utils/misc/guc.c | 52 +++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3c695c1615..da74f00ab2 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -8903,7 +8903,9 @@ can_skip_gucvar(struct config_generic * gconf) /* * estimate_variable_size: - * Estimate max size for dumping the given GUC variable. + * Compute space needed for dumping the given GUC variable. + * + * It's OK to overestimate, but not to underestimate. */ static Size estimate_variable_size(struct config_generic * gconf) @@ -8914,9 +8916,8 @@ estimate_variable_size(struct config_generic * gconf) if (can_skip_gucvar(gconf)) return 0; - size = 0; - - size = add_size(size, strlen(gconf->name) + 1); + /* Name, plus trailing zero byte. */ + size = strlen(gconf->name) + 1; /* Get the maximum display length of the GUC value. */ switch (gconf->vartype) @@ -8937,7 +8938,7 @@ estimate_variable_size(struct config_generic * gconf) * small values. Maximum value is 2147483647, i.e. 10 chars. * Include one byte for sign. */ - if (abs(*conf->variable) < 1000) + if (Abs(*conf->variable) < 1000) valsize = 3 + 1; else valsize = 10 + 1; @@ -8947,11 +8948,12 @@ estimate_variable_size(struct config_generic * gconf) case PGC_REAL: { /* - * We are going to print it with %.17g. Account for sign, - * decimal point, and e+nnn notation. E.g. - * -3.9932904234000002e+110 + * We are going to print it with %e with REALTYPE_PRECISION + * fractional digits. Account for sign, leading digit, + * decimal point, and exponent with up to 3 digits. E.g. + * -3.99329042340000021e+110 */ - valsize = REALTYPE_PRECISION + 1 + 1 + 5; + valsize = 1 + 1 + 1 + REALTYPE_PRECISION + 5; } break; @@ -8959,7 +8961,15 @@ estimate_variable_size(struct config_generic * gconf) { struct config_string *conf = (struct config_string *) gconf; - valsize = strlen(*conf->variable); + /* + * If the value is NULL, we transmit it as an empty string. + * Although this is not physically the same value, GUC + * generally treats a NULL the same as empty string. + */ + if (*conf->variable) + valsize = strlen(*conf->variable); + else + valsize = 0; } break; @@ -8972,17 +8982,17 @@ estimate_variable_size(struct config_generic * gconf) break; } - /* Allow space for terminating zero-byte */ + /* Allow space for terminating zero-byte for value */ size = add_size(size, valsize + 1); if (gconf->sourcefile) size = add_size(size, strlen(gconf->sourcefile)); - /* Allow space for terminating zero-byte */ + /* Allow space for terminating zero-byte for sourcefile */ size = add_size(size, 1); - /* Include line whenever we include file. */ - if (gconf->sourcefile) + /* Include line whenever file is nonempty. */ + if (gconf->sourcefile && gconf->sourcefile[0]) size = add_size(size, sizeof(gconf->sourceline)); size = add_size(size, sizeof(gconf->source)); @@ -9100,7 +9110,7 @@ serialize_variable(char **destptr, Size *maxbytes, { struct config_real *conf = (struct config_real *) gconf; - do_serialize(destptr, maxbytes, "%.*g", + do_serialize(destptr, maxbytes, "%.*e", REALTYPE_PRECISION, *conf->variable); } break; @@ -9109,7 +9119,9 @@ serialize_variable(char **destptr, Size *maxbytes, { struct config_string *conf = (struct config_string *) gconf; - do_serialize(destptr, maxbytes, "%s", *conf->variable); + /* NULL becomes empty string, see estimate_variable_size() */ + do_serialize(destptr, maxbytes, "%s", + *conf->variable ? *conf->variable : ""); } break; @@ -9126,7 +9138,7 @@ serialize_variable(char **destptr, Size *maxbytes, do_serialize(destptr, maxbytes, "%s", (gconf->sourcefile ? gconf->sourcefile : "")); - if (gconf->sourcefile) + if (gconf->sourcefile && gconf->sourcefile[0]) do_serialize_binary(destptr, maxbytes, &gconf->sourceline, sizeof(gconf->sourceline)); @@ -9193,7 +9205,7 @@ read_gucstate(char **srcptr, char *srcend) for (ptr = *srcptr; ptr < srcend && *ptr != '\0'; ptr++) ; - if (ptr > srcend) + if (ptr >= srcend) elog(ERROR, "could not find null terminator in GUC state"); /* Set the new position to the byte following the terminating NUL */ @@ -9247,9 +9259,7 @@ RestoreGUCState(void *gucstate) { int result; - if ((varname = read_gucstate(&srcptr, srcend)) == NULL) - break; - + varname = read_gucstate(&srcptr, srcend); varvalue = read_gucstate(&srcptr, srcend); varsourcefile = read_gucstate(&srcptr, srcend); if (varsourcefile[0])