From 535281f3525748044071ea6c44f65fc8b6ca6f37 Mon Sep 17 00:00:00 2001 From: Henson Choi Date: Sat, 29 Aug 2026 19:11:59 +0900 Subject: [PATCH] Settle one contract for the RPRPattern copy, out and read functions RPRPattern needs hand-written copy/out/read because of its two arrays, and the three had drifted apart from each other and from the header in three ways. None is reachable today, and that is the reason to fix them: what makes each one safe is a fact outside these functions. plannodes.h declares eight fields per element. _copyRPRPattern() carries all eight because it memcpy()s the struct; out and read handle seven, leaving reserved zeroed. Nothing said which was the rule, so the header now does: reserved is padding, the round trip drops it, and a field that takes the byte over has to join the seven first. A StaticAssertDecl on sizeof would not help -- repurposing reserved leaves the struct at sixteen bytes, so it would pass while out and read went on dropping the value. varNames went out through outToken(), which spells NULL as <> and the empty string as "", and came back through debackslash(), which knows neither; read now uses nullable_string(). The same trio disagreed again in that field: out accepted {numVars > 0, varNames == NULL} and wrote <> for it, read restored that state, and copy dies on it at pstrdup(NULL). Rather than teach read to carry the state across, out no longer offers it and read rejects it -- makeRPRPattern() guarantees the array either way. That rejection is spread over the whole function. The delimiters were stepped over unchecked and the element array opened with an Assert, so a numVars or numElements disagreeing with the text left the token stream off by one for everything after it, silently in a build without assertions. Each delimiter is checked now. Last, flags is written with %u and was read with atoi(); readfuncs.c states the rule above atoui(), which is what this needs. The local declaration splits flags out as unsigned int, or the value would be narrowed back a line later. No behaviour changes. Every RPR plan already goes through all three functions under debug_copy_parse_plan_trees and debug_write_read_parse_plan_trees, and passing now means more than it did, since a disagreeing count stops rather than slides. --- src/backend/nodes/copyfuncs.c | 8 ++++-- src/backend/nodes/outfuncs.c | 30 +++++++++++---------- src/backend/nodes/readfuncs.c | 49 ++++++++++++++++++++++------------- src/include/nodes/plannodes.h | 6 +++++ 4 files changed, 60 insertions(+), 33 deletions(-) diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 17d45930d7b..62008758160 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -177,12 +177,16 @@ _copyRPRPattern(const RPRPattern *from) COPY_SCALAR_FIELD(numElements); /* Deep copy the varNames array (DEFINE clause is required) */ - Assert(from->numVars > 0); + Assert(from->numVars > 0 && from->varNames != NULL); newnode->varNames = palloc0_array(char *, from->numVars); for (int i = 0; i < from->numVars; i++) newnode->varNames[i] = pstrdup(from->varNames[i]); - /* Deep copy the elements array (always has at least one element + FIN) */ + /* + * Deep copy the elements array (always has at least one element + FIN). + * This carries the whole struct, reserved byte included, where out/read + * carry seven fields and zero that byte -- see RPRPatternElement. + */ Assert(from->numElements >= 2); newnode->elements = palloc_array(RPRPatternElement, from->numElements); memcpy(newnode->elements, from->elements, diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 0b145329cbf..9ba635192ab 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -737,23 +737,27 @@ _outRPRPattern(StringInfo str, const RPRPattern *node) WRITE_INT_FIELD(maxDepth); WRITE_INT_FIELD(numElements); - /* Write varNames array as list of strings */ + /* + * Write varNames array as list of strings. makeRPRPattern() guarantees + * the array, so the list has exactly one spelling and the read side has + * no second shape to interpret. + */ appendStringInfoString(str, " :varNames"); - if (node->numVars > 0 && node->varNames != NULL) + Assert(node->numVars > 0 && node->varNames != NULL); + appendStringInfoString(str, " ("); + for (int i = 0; i < node->numVars; i++) { - appendStringInfoString(str, " ("); - for (int i = 0; i < node->numVars; i++) - { - if (i > 0) - appendStringInfoChar(str, ' '); - outToken(str, node->varNames[i]); - } - appendStringInfoChar(str, ')'); + if (i > 0) + appendStringInfoChar(str, ' '); + outToken(str, node->varNames[i]); } - else - appendStringInfoString(str, " <>"); + appendStringInfoChar(str, ')'); - /* Write elements array (makeRPRPattern guarantees numElements >= 2) */ + /* + * Write elements array (makeRPRPattern guarantees numElements >= 2). + * Seven fields go out; the reserved byte is padding and stays behind -- + * see RPRPatternElement in plannodes.h. + */ appendStringInfoString(str, " :elements"); Assert(node->numElements > 0 && node->elements != NULL); appendStringInfoChar(str, ' '); diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index d390073e344..a5b84ed79ee 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -577,40 +577,47 @@ _readRPRPattern(ReadNodeContext *ctx) READ_INT_FIELD(maxDepth); READ_INT_FIELD(numElements); - /* Read varNames array */ + /* + * Read varNames array. _outRPRPattern() always writes the list, so every + * token here has one spelling and any other input is malformed. The + * delimiters are checked rather than counted on, because a numVars that + * disagrees with the list would otherwise leave the token stream off by + * one for everything that follows. + */ token = pg_strtok(ctx, &length); /* skip :varNames */ - token = pg_strtok(ctx, &length); /* get '(' or '<>' */ - if (local_node->numVars > 0 && token[0] == '(') - { - local_node->varNames = palloc_array(char *, local_node->numVars); - for (int i = 0; i < local_node->numVars; i++) - { - token = pg_strtok(ctx, &length); - local_node->varNames[i] = debackslash(token, length); - } - token = pg_strtok(ctx, &length); /* skip ')' */ - } - else + token = pg_strtok(ctx, &length); /* get '(' */ + if (local_node->numVars <= 0 || token == NULL || token[0] != '(') + elog(ERROR, "unexpected varNames in RPRPattern"); + local_node->varNames = palloc_array(char *, local_node->numVars); + for (int i = 0; i < local_node->numVars; i++) { - local_node->varNames = NULL; + token = pg_strtok(ctx, &length); + if (token == NULL) + elog(ERROR, "unexpected end of RPRPattern varNames"); + local_node->varNames[i] = nullable_string(token, length); } + token = pg_strtok(ctx, &length); /* get ')' */ + if (token == NULL || token[0] != ')') + elog(ERROR, "unterminated varNames in RPRPattern"); /* Read elements array */ token = pg_strtok(ctx, &length); /* skip :elements */ token = pg_strtok(ctx, &length); /* get '(' */ /* out always emits the array (makeRPRPattern guarantees numElements >= 2) */ - Assert(local_node->numElements > 0 && token[0] == '('); + if (local_node->numElements <= 0 || token == NULL || token[0] != '(') + elog(ERROR, "unexpected elements in RPRPattern"); + /* palloc0 also zeroes reserved, which the round trip drops */ local_node->elements = palloc0_array(RPRPatternElement, local_node->numElements); for (int i = 0; i < local_node->numElements; i++) { RPRPatternElement *elem = &local_node->elements[i]; int varId, - flags, depth, min, max, next, jump; + unsigned int flags; /* written with %u, unlike the others */ /* Parse "(varId depth flags min max next jump)" */ token = pg_strtok(ctx, &length); @@ -618,7 +625,7 @@ _readRPRPattern(ReadNodeContext *ctx) token = pg_strtok(ctx, &length); depth = atoi(token); token = pg_strtok(ctx, &length); - flags = atoi(token); + flags = atoui(token); token = pg_strtok(ctx, &length); min = atoi(token); token = pg_strtok(ctx, &length); @@ -627,7 +634,9 @@ _readRPRPattern(ReadNodeContext *ctx) next = atoi(token); token = pg_strtok(ctx, &length); jump = atoi(token); - token = pg_strtok(ctx, &length); /* skip ')' */ + token = pg_strtok(ctx, &length); /* get ')' */ + if (token == NULL || token[0] != ')') + elog(ERROR, "unterminated element in RPRPattern"); elem->varId = (RPRVarId) varId; elem->flags = (RPRElemFlags) flags; @@ -639,7 +648,11 @@ _readRPRPattern(ReadNodeContext *ctx) /* Read next element's '(' or end */ if (i < local_node->numElements - 1) + { token = pg_strtok(ctx, &length); /* get '(' */ + if (token == NULL || token[0] != '(') + elog(ERROR, "unexpected end of RPRPattern elements"); + } } READ_BOOL_FIELD(isAbsorbable); diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 2d40d82c524..a6e57cf3493 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -1266,6 +1266,12 @@ typedef int16 RPRElemIdx; /* element array index */ * * Layout optimized for alignment (no padding holes): * varId(1) + depth(1) + flags(1) + reserved(1) + min(4) + max(4) + next(2) + jump(2) + * + * reserved is padding and is not serialized: the round trip drops it. + * _outRPRPattern() writes the other seven fields and _readRPRPattern() zeroes + * this one, so that format string is the whole of what crosses. + * _copyRPRPattern() memcpy()s the struct and therefore carries all eight. A + * field that takes this byte over has to join the seven first. */ typedef struct RPRPatternElement {