From 61b07bb2f72723d3306e3afa817df567770dcf2a Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Sat, 5 Sep 2026 21:34:56 +0000 Subject: [PATCH v2 1/2] test_extensible: Reject other node types before calling stringToNode() The SQL wrappers of this module decode their text argument with stringToNode(), and only then check that the result is a TestExtNode. stringToNode() assumes its input to be valid, though: the read routines of readfuncs.c fetch tokens with pg_strtok() and feed them to atoi() without checking for NULL, so any other node type name, like '{QUERY}' or '{VAR}', crashes the backend on the first missing field. The functions are executable by any user, so this was reachable without any privileges. To fix, look at the leading tokens of the string before handing it to stringToNode(), and reject anything but an EXTENSIBLENODE named TestExtNode, so that only the nodeRead callback of this module, which does check for missing tokens, ever sees the rest of the string. Oversight in commit 0e944fe3579. --- .../expected/test_extensible.out | 9 +++ .../test_extensible/sql/test_extensible.sql | 6 ++ .../modules/test_extensible/test_extensible.c | 64 +++++++++++++++---- 3 files changed, 65 insertions(+), 14 deletions(-) diff --git a/src/test/modules/test_extensible/expected/test_extensible.out b/src/test/modules/test_extensible/expected/test_extensible.out index 1fa3b6d2c55..694efb7142f 100644 --- a/src/test/modules/test_extensible/expected/test_extensible.out +++ b/src/test/modules/test_extensible/expected/test_extensible.out @@ -91,6 +91,15 @@ SELECT test_ext_node_equal(test_ext_node_make('1234'::oid, 2), -- correct node type, missing field SELECT test_ext_node_get_relid('{EXTENSIBLENODE :extnodename TestExtNode}'); ERROR: unexpected end of "TestExtNode" +-- other node types, rejected before reaching their core read routines +SELECT test_ext_node_get_relid('{QUERY}'); +ERROR: argument is not a serialized "TestExtNode" +-- extensible node of another kind +SELECT test_ext_node_get_relid('{EXTENSIBLENODE :extnodename NoSuchExtNode}'); +ERROR: argument is not a serialized "TestExtNode" +-- not a node string at all +SELECT test_ext_node_get_relid('42'); +ERROR: argument is not a serialized "TestExtNode" -- CustomScan tests CREATE TABLE test_extensible_tbl (id integer, val text); INSERT INTO test_extensible_tbl VALUES (1, 'one'), (2, 'two'), (3, 'three'); diff --git a/src/test/modules/test_extensible/sql/test_extensible.sql b/src/test/modules/test_extensible/sql/test_extensible.sql index b23bd1cd087..259f7c26fe6 100644 --- a/src/test/modules/test_extensible/sql/test_extensible.sql +++ b/src/test/modules/test_extensible/sql/test_extensible.sql @@ -33,6 +33,12 @@ SELECT test_ext_node_equal(test_ext_node_make('1234'::oid, 2), -- correct node type, missing field SELECT test_ext_node_get_relid('{EXTENSIBLENODE :extnodename TestExtNode}'); +-- other node types, rejected before reaching their core read routines +SELECT test_ext_node_get_relid('{QUERY}'); +-- extensible node of another kind +SELECT test_ext_node_get_relid('{EXTENSIBLENODE :extnodename NoSuchExtNode}'); +-- not a node string at all +SELECT test_ext_node_get_relid('42'); -- CustomScan tests CREATE TABLE test_extensible_tbl (id integer, val text); diff --git a/src/test/modules/test_extensible/test_extensible.c b/src/test/modules/test_extensible/test_extensible.c index 2a1344f28d5..944ba0cb161 100644 --- a/src/test/modules/test_extensible/test_extensible.c +++ b/src/test/modules/test_extensible/test_extensible.c @@ -99,21 +99,43 @@ test_ext_node_out_cb(StringInfo str, const ExtensibleNode *node) } /* - * Fetch the next token, erroring out instead of returning NULL. + * Fetch the next token of the string being read. * - * Unlike anything in readfuncs.c, this callback is reachable with arbitrary - * strings through SQL function calls, so we need this check. + * Unlike anything in readfuncs.c, this is reachable with arbitrary strings + * through SQL function calls, so the tokens need to be checked. The two + * modes of this function fail differently: + * + * - With "expected" set to NULL, any token is accepted, but running out of + * them is an error, reported here. This is what the nodeRead callback + * needs once the string is known to describe one of our nodes. + * + * - With a non-NULL "expected", a missing or different token is reported + * to the caller as NULL, so that text_to_test_ext_node() can check a + * sequence of tokens and issue a single error for the whole string. */ static const char * -test_ext_node_next_token(ReadNodeContext *ctx) +test_ext_node_next_token(ReadNodeContext *ctx, const char *expected) { int length; const char *token = pg_strtok(ctx, &length); if (token == NULL) + { + if (expected != NULL) + return NULL; + ereport(ERROR, (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION), errmsg("unexpected end of \"%s\"", TEST_EXT_NODE_NAME))); + } + + if (expected != NULL) + { + if (length != (int) strlen(expected)) + return NULL; + if (memcmp(token, expected, length) != 0) + return NULL; + } return token; } @@ -123,11 +145,11 @@ test_ext_node_read_cb(ReadNodeContext *ctx, ExtensibleNode *node) { TestExtNode *tnode = (TestExtNode *) node; - (void) test_ext_node_next_token(ctx); /* skip :relid */ - tnode->relid = atooid(test_ext_node_next_token(ctx)); + (void) test_ext_node_next_token(ctx, NULL); /* skip :relid */ + tnode->relid = atooid(test_ext_node_next_token(ctx, NULL)); - (void) test_ext_node_next_token(ctx); /* skip :repeat_count */ - tnode->repeat_count = atoi(test_ext_node_next_token(ctx)); + (void) test_ext_node_next_token(ctx, NULL); /* skip :repeat_count */ + tnode->repeat_count = atoi(test_ext_node_next_token(ctx, NULL)); } static const ExtensibleNodeMethods test_ext_node_methods = @@ -463,22 +485,36 @@ test_get_custom_scan_methods(PG_FUNCTION_ARGS) } /* - * Decode a TestExtNode via stringToNode(), rejecting a string describing - * some other kind of node instead of misinterpreting it as one of ours. + * Decode a TestExtNode via stringToNode(). + * + * stringToNode() assumes its input to be valid, and the read routines of + * readfuncs.c it dispatches to do not check for missing tokens, so handing + * it a string naming any other node type could crash the backend. Hence, + * look at the leading tokens first and refuse anything that would not end + * up in our own nodeRead callback, the only one hardened against arbitrary + * input. */ static TestExtNode * text_to_test_ext_node(text *txt) { - Node *node = stringToNode(text_to_cstring(txt)); + char *str = text_to_cstring(txt); + ReadNodeContext ctx = {.str = str}; + TestExtNode *tnode; - if (node == NULL || !IsA(node, ExtensibleNode) || - strcmp(((ExtensibleNode *) node)->extnodename, TEST_EXT_NODE_NAME) != 0) + if (test_ext_node_next_token(&ctx, "{") == NULL || + test_ext_node_next_token(&ctx, "EXTENSIBLENODE") == NULL || + test_ext_node_next_token(&ctx, ":extnodename") == NULL || + test_ext_node_next_token(&ctx, TEST_EXT_NODE_NAME) == NULL) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("argument is not a serialized \"%s\"", TEST_EXT_NODE_NAME))); - return (TestExtNode *) node; + tnode = (TestExtNode *) stringToNode(str); + Assert(IsA(tnode, ExtensibleNode) && + strcmp(tnode->base.extnodename, TEST_EXT_NODE_NAME) == 0); + + return tnode; } /* -- 2.43.0