Code review for GUC serialization/deserialization code.

tglsfdc · tglsfdc · commit b9ee42e70a41 · 2016-11-19T14:26:20.000-05:00
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 e45e990). 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>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
@@ -8675,7 +8675,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)
@@ -8686,9 +8688,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)
@@ -8709,7 +8710,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;
@@ -8719,19 +8720,28 @@ 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;
 
 		case PGC_STRING:
 			{
 				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;
 
@@ -8744,17 +8754,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));
@@ -8872,7 +8882,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;
@@ -8881,7 +8891,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;
 
@@ -8898,7 +8910,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));
 
@@ -8965,7 +8977,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 */
@@ -9019,9 +9031,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])
