Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 272c426

Browse files
committed
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 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>
1 parent 0eaa511 commit 272c426

File tree

1 file changed

+31
-21
lines changed
  • src/backend/utils/misc

1 file changed

+31
-21
lines changed

src/backend/utils/misc/guc.c

+31-21
Original file line numberDiff line numberDiff line change
@@ -8885,7 +8885,9 @@ can_skip_gucvar(struct config_generic * gconf)
88858885

88868886
/*
88878887
* estimate_variable_size:
8888-
* Estimate max size for dumping the given GUC variable.
8888+
* Compute space needed for dumping the given GUC variable.
8889+
*
8890+
* It's OK to overestimate, but not to underestimate.
88898891
*/
88908892
static Size
88918893
estimate_variable_size(struct config_generic * gconf)
@@ -8896,9 +8898,8 @@ estimate_variable_size(struct config_generic * gconf)
88968898
if (can_skip_gucvar(gconf))
88978899
return 0;
88988900

8899-
size = 0;
8900-
8901-
size = add_size(size, strlen(gconf->name) + 1);
8901+
/* Name, plus trailing zero byte. */
8902+
size = strlen(gconf->name) + 1;
89028903

89038904
/* Get the maximum display length of the GUC value. */
89048905
switch (gconf->vartype)
@@ -8919,7 +8920,7 @@ estimate_variable_size(struct config_generic * gconf)
89198920
* small values. Maximum value is 2147483647, i.e. 10 chars.
89208921
* Include one byte for sign.
89218922
*/
8922-
if (abs(*conf->variable) < 1000)
8923+
if (Abs(*conf->variable) < 1000)
89238924
valsize = 3 + 1;
89248925
else
89258926
valsize = 10 + 1;
@@ -8929,19 +8930,28 @@ estimate_variable_size(struct config_generic * gconf)
89298930
case PGC_REAL:
89308931
{
89318932
/*
8932-
* We are going to print it with %.17g. Account for sign,
8933-
* decimal point, and e+nnn notation. E.g.
8934-
* -3.9932904234000002e+110
8933+
* We are going to print it with %e with REALTYPE_PRECISION
8934+
* fractional digits. Account for sign, leading digit,
8935+
* decimal point, and exponent with up to 3 digits. E.g.
8936+
* -3.99329042340000021e+110
89358937
*/
8936-
valsize = REALTYPE_PRECISION + 1 + 1 + 5;
8938+
valsize = 1 + 1 + 1 + REALTYPE_PRECISION + 5;
89378939
}
89388940
break;
89398941

89408942
case PGC_STRING:
89418943
{
89428944
struct config_string *conf = (struct config_string *) gconf;
89438945

8944-
valsize = strlen(*conf->variable);
8946+
/*
8947+
* If the value is NULL, we transmit it as an empty string.
8948+
* Although this is not physically the same value, GUC
8949+
* generally treats a NULL the same as empty string.
8950+
*/
8951+
if (*conf->variable)
8952+
valsize = strlen(*conf->variable);
8953+
else
8954+
valsize = 0;
89458955
}
89468956
break;
89478957

@@ -8954,17 +8964,17 @@ estimate_variable_size(struct config_generic * gconf)
89548964
break;
89558965
}
89568966

8957-
/* Allow space for terminating zero-byte */
8967+
/* Allow space for terminating zero-byte for value */
89588968
size = add_size(size, valsize + 1);
89598969

89608970
if (gconf->sourcefile)
89618971
size = add_size(size, strlen(gconf->sourcefile));
89628972

8963-
/* Allow space for terminating zero-byte */
8973+
/* Allow space for terminating zero-byte for sourcefile */
89648974
size = add_size(size, 1);
89658975

8966-
/* Include line whenever we include file. */
8967-
if (gconf->sourcefile)
8976+
/* Include line whenever file is nonempty. */
8977+
if (gconf->sourcefile && gconf->sourcefile[0])
89688978
size = add_size(size, sizeof(gconf->sourceline));
89698979

89708980
size = add_size(size, sizeof(gconf->source));
@@ -9082,7 +9092,7 @@ serialize_variable(char **destptr, Size *maxbytes,
90829092
{
90839093
struct config_real *conf = (struct config_real *) gconf;
90849094

9085-
do_serialize(destptr, maxbytes, "%.*g",
9095+
do_serialize(destptr, maxbytes, "%.*e",
90869096
REALTYPE_PRECISION, *conf->variable);
90879097
}
90889098
break;
@@ -9091,7 +9101,9 @@ serialize_variable(char **destptr, Size *maxbytes,
90919101
{
90929102
struct config_string *conf = (struct config_string *) gconf;
90939103

9094-
do_serialize(destptr, maxbytes, "%s", *conf->variable);
9104+
/* NULL becomes empty string, see estimate_variable_size() */
9105+
do_serialize(destptr, maxbytes, "%s",
9106+
*conf->variable ? *conf->variable : "");
90959107
}
90969108
break;
90979109

@@ -9108,7 +9120,7 @@ serialize_variable(char **destptr, Size *maxbytes,
91089120
do_serialize(destptr, maxbytes, "%s",
91099121
(gconf->sourcefile ? gconf->sourcefile : ""));
91109122

9111-
if (gconf->sourcefile)
9123+
if (gconf->sourcefile && gconf->sourcefile[0])
91129124
do_serialize_binary(destptr, maxbytes, &gconf->sourceline,
91139125
sizeof(gconf->sourceline));
91149126

@@ -9175,7 +9187,7 @@ read_gucstate(char **srcptr, char *srcend)
91759187
for (ptr = *srcptr; ptr < srcend && *ptr != '\0'; ptr++)
91769188
;
91779189

9178-
if (ptr > srcend)
9190+
if (ptr >= srcend)
91799191
elog(ERROR, "could not find null terminator in GUC state");
91809192

91819193
/* Set the new position to the byte following the terminating NUL */
@@ -9229,9 +9241,7 @@ RestoreGUCState(void *gucstate)
92299241
{
92309242
int result;
92319243

9232-
if ((varname = read_gucstate(&srcptr, srcend)) == NULL)
9233-
break;
9234-
9244+
varname = read_gucstate(&srcptr, srcend);
92359245
varvalue = read_gucstate(&srcptr, srcend);
92369246
varsourcefile = read_gucstate(&srcptr, srcend);
92379247
if (varsourcefile[0])

0 commit comments

Comments
 (0)