Re: Improve error handling in test modules: test_extensible, test_bitmapset

On Sun, Sep 06, 2026 at 11:22:58AM +0100, Zsolt Parragi wrote:
> The recently added test extensible module has a small oversight: it
> lacks input validation, which made it easy to crash the backend with
> invalid input with the test module loaded. While this doesn't seem
> like a critical issue for a test module, the commit message also
> mentions that this could be a template for extension developers, so I
> think it is worth fixing.

+static bool
+test_ext_node_expect_token(ReadNodeContext *ctx, const char *expected)
+{
+ int length;
+ const char *token = pg_strtok(ctx, &length);
+
+ return token != NULL && length == (int) strlen(expected) &&
+ memcmp(token, expected, length) == 0;

I am not much a fan of spreading pg_strtok() calls more than
necessary in this module. How about extending
test_ext_node_next_token() with an "expected" argument to enforce some
validation? If "expected" is NULL, fall back to the default of
failing if the end has been reached and a NULL token is found, for the
read callback.

+ return token != NULL && length == (int) strlen(expected) &&
+ memcmp(token, expected, length) == 0;

And perhaps also split these in multiple lines, just because it would
feel slightly cleaner.. I know I'm picky on these things.

I am not convinced that we need to check all the input function calls
in 0001 and 0002. For 0001, I'd just pick up one for simplicity of
parsing the test. I can see that you are mostly doing that in 0002,
with test_bms_num_members(). Let's just pick up one, reduce the
duplicates.
--
Michael