missing assert in makeString

Started by Pavel Stehule11 months ago5 messages

pavel.stehule@gmail.com

11 months ago

1 attachment(s)

I investigated the crashes in "xmlnamespaces to xmlelement" patch and it
looks like there was a badly used makeString function. The argument should
not be null, elsewhere serialization to string fails - and deserialization
doesn't support this case.

https://cirrus-ci.com/task/6543809942650880

I propose to add an assert there like (make check-world passed)

diff --git a/src/backend/nodes/value.c b/src/backend/nodes/value.c
index 5a8c1ce2478..620a97ece79 100644
--- a/src/backend/nodes/value.c
+++ b/src/backend/nodes/value.c
@@ -64,6 +64,8 @@ makeString(char *str)
 {
    String     *v = makeNode(String);

+ Assert(str);
+
v->sval = str;
return v;
}

Regards

Pavel

Attachments:

0001-initial.patchtext/x-patch; charset=US-ASCII; name=0001-initial.patchDownload

From 62ee71bd9ee36bf3c33f3d00a6f8f81782ccf91f Mon Sep 17 00:00:00 2001
From: "okbob@github.com" <pavel.stehule@gmail.com>
Date: Wed, 19 Feb 2025 07:04:23 +0100
Subject: [PATCH] initial

---
 src/backend/nodes/value.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/backend/nodes/value.c b/src/backend/nodes/value.c
index 5a8c1ce2478..620a97ece79 100644
--- a/src/backend/nodes/value.c
+++ b/src/backend/nodes/value.c
@@ -64,6 +64,8 @@ makeString(char *str)
 {
 	String	   *v = makeNode(String);
 
+	Assert(str);
+
 	v->sval = str;
 	return v;
 }
-- 
2.48.1

Tom Lane

tgl@sss.pgh.pa.us

11 months ago

In reply to: Pavel Stehule (#1)

Re: missing assert in makeString

Pavel Stehule <pavel.stehule@gmail.com> writes:

I investigated the crashes in "xmlnamespaces to xmlelement" patch and it
looks like there was a badly used makeString function. The argument should
not be null, elsewhere serialization to string fails - and deserialization
doesn't support this case.
I propose to add an assert there like (make check-world passed)

Hmmm ... while I don't necessarily object to this patch, we have a lot
of makeFoo() functions that build nodes, and hardly any of them have
asserts like this one. Why makeString() in particular? Is the fault
on the serialization side, instead? If there's a general expectation
that a String node's value isn't null, how come the original patch
worked at all?

regards, tom lane

Pavel Stehule

pavel.stehule@gmail.com

11 months ago

In reply to: Tom Lane (#2)

Re: missing assert in makeString

st 19. 2. 2025 v 7:48 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> writes:

I investigated the crashes in "xmlnamespaces to xmlelement" patch and it
looks like there was a badly used makeString function. The argument

should

not be null, elsewhere serialization to string fails - and

deserialization

doesn't support this case.
I propose to add an assert there like (make check-world passed)

Hmmm ... while I don't necessarily object to this patch, we have a lot
of makeFoo() functions that build nodes, and hardly any of them have
asserts like this one. Why makeString() in particular? Is the fault
on the serialization side, instead? If there's a general expectation
that a String node's value isn't null, how come the original patch
worked at all?

Other makeFoo functions have arguments of Node * type, and there the NULL
is not problematic,
or has arguments of valuable types, or holds flag xxxisnull and handles
nulls correctly.

Similar is makeAlias(const char *aliasname, List *colnames),
makeColumnDef(...)

but these functions crashes early due usage of pstrdup

DefElem doesn't do pstrdup and it can crash too, so the assertions should
be there too.

Unfortunately, the original patch had not completed tests - there was
missing forcing an expression serialization. So it worked. Surprisingly it
fails on BSD, where expression serialization was forced (I didn't
investigate what is different).

I think makeFoo() should produce a result that doesn't fail anywhere (when
it was not modified badly later). So they should accept only data that we
correctly serialize and deserialize.

Moreover, it fails early. The out func crashes too late, and it needs
special regress tests. Unfortunately, there is not any flag that can
signal, so some tests are missing.

Regards

Pavel

Show quoted text

regards, tom lane

Andres Freund

andres@anarazel.de

11 months ago

In reply to: Tom Lane (#2)

Re: missing assert in makeString

Hi,

On 2025-02-19 01:48:53 -0500, Tom Lane wrote:

Pavel Stehule <pavel.stehule@gmail.com> writes:

I investigated the crashes in "xmlnamespaces to xmlelement" patch and it
looks like there was a badly used makeString function. The argument should
not be null, elsewhere serialization to string fails - and deserialization
doesn't support this case.
I propose to add an assert there like (make check-world passed)

Hmmm ... while I don't necessarily object to this patch, we have a lot
of makeFoo() functions that build nodes, and hardly any of them have
asserts like this one.

I also suspect that adding is-not-NULL asserts isn't that helpful on its own,
because you still need to reach that function with it set to NULL. We probably
should use pg_attribute_nonnull() much more widely, so that compilers and
static analyzers can help.

Why makeString() in particular? Is the fault on the serialization side,
instead? If there's a general expectation that a String node's value isn't
null, how come the original patch worked at all?

It's worth noting that the CI task just failed on freebsd, which builds with:

CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS

PG_TEST_INITDB_EXTRA_OPTS: -c debug_copy_parse_plan_trees=on -c debug_write_read_parse_plan_trees=on -c debug_raw_expression_coverage_test=on

Greetings,

Andres Freund

Pavel Stehule

pavel.stehule@gmail.com

11 months ago

In reply to: Andres Freund (#4)

Re: missing assert in makeString

st 19. 2. 2025 v 19:05 odesílatel Andres Freund <andres@anarazel.de> napsal:

Hi,

On 2025-02-19 01:48:53 -0500, Tom Lane wrote:

Pavel Stehule <pavel.stehule@gmail.com> writes:

I investigated the crashes in "xmlnamespaces to xmlelement" patch and

it

looks like there was a badly used makeString function. The argument

should

not be null, elsewhere serialization to string fails - and

deserialization

doesn't support this case.
I propose to add an assert there like (make check-world passed)

Hmmm ... while I don't necessarily object to this patch, we have a lot
of makeFoo() functions that build nodes, and hardly any of them have
asserts like this one.

I also suspect that adding is-not-NULL asserts isn't that helpful on its
own,
because you still need to reach that function with it set to NULL. We
probably
should use pg_attribute_nonnull() much more widely, so that compilers and
static analyzers can help.

Any mechanism that can help is welcome. Unfortunately, I miss knowledge
about C static analyzers.
This issue can be pretty messy for beginners - who try to write their own
first patches. The error is raised
far to the point where this problem was created, and it needs a special
test or special configuration setting.

Probably this issue is less often than before, because out functions are
generated automatically, but there
can be bad design. Another possibility is implementing correct support for
NULL there, but probably it needs
format change, and it can be an issue for pg_upgrade.

Regards

Pavel

Show quoted text

Why makeString() in particular? Is the fault on the serialization side,
instead? If there's a general expectation that a String node's value

isn't

null, how come the original patch worked at all?

It's worth noting that the CI task just failed on freebsd, which builds
with:

CPPFLAGS: -DRELCACHE_FORCE_RELEASE
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS

PG_TEST_INITDB_EXTRA_OPTS: -c debug_copy_parse_plan_trees=on -c
debug_write_read_parse_plan_trees=on -c
debug_raw_expression_coverage_test=on

Greetings,

Andres Freund