Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Create infrastructure for 'MinimalTuple' representation of in-memory

  2. Teach nodeSort and nodeMaterial to optimize out unnecessary overhead

  1. Check for tuplestorestate nullness before dereferencing

    Alexander Kuznetsov <kuznetsovam@altlinux.org> — 2024-10-14T09:25:50Z

    Hello everyone,

I'd like to propose adding a check for the nullness of tuplestorestate before dereferencing it
in src/backend/executor/nodeModifier.c. The patch is attached.

I am proposing this fix based on the assumption that tuplestorestate could be NULL
since there is a check for it when calculating eof_tuplestore at line 85.
However, since this code hasn't been changed since 2006 and hasn't caused any issues,
it is possible that the check for (tuplestorestate == NULL) is redundant when calculating eof_tuplestore.

-- 
Best regards,
Alexander Kuznetsov

  2. Re: Check for tuplestorestate nullness before dereferencing

    Ilia Evdokimov <ilya.evdokimov@tantorlabs.com> — 2024-10-14T13:41:53Z

    On 14.10.2024 12:25, Alexander Kuznetsov wrote:
> Hello everyone,
>
> I'd like to propose adding a check for the nullness of tuplestorestate 
> before dereferencing it
> in src/backend/executor/nodeModifier.c. The patch is attached.
>
> I am proposing this fix based on the assumption that tuplestorestate 
> could be NULL
> since there is a check for it when calculating eof_tuplestore at line 85.
> However, since this code hasn't been changed since 2006 and hasn't 
> caused any issues,
> it is possible that the check for (tuplestorestate == NULL) is 
> redundant when calculating eof_tuplestore.
>

Hi Alexander,

The 'tuplestorestate' variable may be initialized at line 64 if it is 
NULL. You should consider initializing this variable earlier.

Regards,
Ilia Evdokimov,
Tantor Labs LLC.

  3. Re: Check for tuplestorestate nullness before dereferencing

    Alena Rybakina <a.rybakina@postgrespro.ru> — 2024-10-14T16:21:33Z

    Hi!

On 14.10.2024 16:41, Ilia Evdokimov wrote:
>
> On 14.10.2024 12:25, Alexander Kuznetsov wrote:
>> Hello everyone,
>>
>> I'd like to propose adding a check for the nullness of 
>> tuplestorestate before dereferencing it
>> in src/backend/executor/nodeModifier.c. The patch is attached.
>>
>> I am proposing this fix based on the assumption that tuplestorestate 
>> could be NULL
>> since there is a check for it when calculating eof_tuplestore at line 
>> 85.
>> However, since this code hasn't been changed since 2006 and hasn't 
>> caused any issues,
>> it is possible that the check for (tuplestorestate == NULL) is 
>> redundant when calculating eof_tuplestore.
>>
>
> Hi Alexander,
>
> The 'tuplestorestate' variable may be initialized at line 64 if it is 
> NULL. You should consider initializing this variable earlier.
>
>
To be honest, I'm not sure this change is necessary. Looking at the 
code, I see that in ExecMaterial it is possible to handle a 
tuplestorestate of NULL, and this error can be accessed if the flags are 
not zero, but I think these cases have been worked out.

As I see it, node->eflags can be zero if it passes the output of a 
subquery, during the initialization of the Material node execution, and 
when the subquery is rescanned.

After the subplan scan is complete, we see that the eof_underlying 
variable becomes true, and this part of the code will no longer be 
accessible. tuplestorestate also becomes Null.

I also noticed that tuplestorestate=NULL is an indicator that the scan 
is complete, so if this section of code is called, something is wrong 
earlier in the code.

-- 
Regards,
Alena Rybakina
Postgres Professional

  4. Re: Check for tuplestorestate nullness before dereferencing

    Alexander Kuznetsov <kuznetsovam@altlinux.org> — 2024-10-15T14:09:42Z

    14.10.2024 19:21, Alena Rybakina wrote:
> To be honest, I'm not sure this change is necessary. Looking at the code, I see that in ExecMaterial it is possible to handle a tuplestorestate of NULL, and this error can be accessed if the flags are not zero, but I think these cases have been worked out.
> [...]
> After the subplan scan is complete, we see that the eof_underlying variable becomes true, and this part of the code will no longer be accessible. tuplestorestate also becomes Null.
>
> I also noticed that tuplestorestate=NULL is an indicator that the scan is complete, so if this section of code is called, something is wrong earlier in the code.
It appears to me that prior to the earlier commit [1], tuplestorestate was
always initialized if it was NULL. In that commit, flags were introduced,
allowing for the possibility that tuplestorestate could remain NULL further
along in the code. This is why I believe that checks for its nullness were
added before calling dereferencing functions, such as the check in line 146,
which is still present.

However, the tuplestore_getheaptuple() function, which is no longer present,
remained unchanged until commit [2], where it was replaced with
tuplestore_gettupleslot() and tuplestore_advance(). While it was acceptable in
case of tuplestore_gettupleslot() that can handle a NULL tuplestorestate,
tuplestore_advance() requires tuplestorestate to not be NULL when it is called.

This leads to my confusion as to why there is no check for the nullness of
tuplestorestate before calling tuplestore_advance().

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d2c555ee538f34be7aff744b994df4d2369a9140
[2]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3f50ba27cf417eb57fd310c2a88f76a6ea6b966e

--
Best regards,
Alexander Kuznetsov

  5. Re: Check for tuplestorestate nullness before dereferencing

    David Rowley <dgrowleyml@gmail.com> — 2024-10-17T23:28:22Z

    On Tue, 15 Oct 2024 at 05:21, Alena Rybakina <a.rybakina@postgrespro.ru> wrote:
> As I see it, node->eflags can be zero if it passes the output of a
> subquery, during the initialization of the Material node execution, and
> when the subquery is rescanned.

Do you have a test case that calls Material with zero eflags?  I tried
adding Assert(node->eflags != 0) to ExecMaterial() and nothing failed.

It would be good to know if the optimisation added in d2c555ee5 ever
applies with today's code. If it does apply, we should likely add a
test case for it and if it never does, then we should just remove the
optimisation and always create the tuplestore when it's NULL.

David

  6. Re: Check for tuplestorestate nullness before dereferencing

    Nikolay Shaplov <dhyan@nataraj.su> — 2025-04-22T11:16:27Z

    В письме от пятница, 18 октября 2024 г. 02:28:22 MSK пользователь David Rowley 
написал:

> It would be good to know if the optimisation added in d2c555ee5 ever
> applies with today's code. If it does apply, we should likely add a
> test case for it and if it never does, then we should just remove the
> optimisation and always create the tuplestore when it's NULL.

That's sounds reasonable. It looks like that removing "node->eflags != 0" check 
is more logical then adding not null check.

> If it does apply, we should likely add a test case for it 

Do you have any idea how to test it?


-- 
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

  7. Re: Check for tuplestorestate nullness before dereferencing

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-22T15:50:49Z

    Nikolay Shaplov <dhyan@nataraj.su> writes:
> В письме от пятница, 18 октября 2024 г. 02:28:22 MSK пользователь David Rowley 
> написал:

>> It would be good to know if the optimisation added in d2c555ee5 ever
>> applies with today's code. If it does apply, we should likely add a
>> test case for it and if it never does, then we should just remove the
>> optimisation and always create the tuplestore when it's NULL.

> That's sounds reasonable. It looks like that removing "node->eflags != 0" check 
> is more logical then adding not null check.

I don't believe that the patch as-proposed is necessary or reasonable.

The case of node->eflags == 0 is reachable; I don't know why David's
test didn't see this, but when I try asserting the contrary I get
a regression crash on

(gdb) p debug_query_string 
$1 = 0x1d03160 "explain (costs off) declare c1 scroll cursor for select (select 42) as x;"

The reason that the subsequent bit of code is safe is that !forward
should not possibly be true unless EXEC_FLAG_BACKWARD was given,
which'd cause us to create a tuplestore.  So if we were going
to change anything, I'd propose adding something more like

	if (!forward && eof_tuplestore)
	{
+		Assert(node->eflags & EXEC_FLAG_BACKWARD);
		if (!node->eof_underlying)
		{
			/*

or perhaps more directly, Assert that tuplestore is not null.
But I don't like the proposed patch because if tuplestore is
null here, something's wrong, and we should complain not
silently mask the problem.

			regards, tom lane

  8. Re: Check for tuplestorestate nullness before dereferencing

    Nikolay Shaplov <dhyan@nataraj.su> — 2025-04-22T16:11:51Z

    В письме от вторник, 22 апреля 2025 г. 18:50:49 MSK пользователь Tom Lane 
написал:

> The reason that the subsequent bit of code is safe is that !forward
> should not possibly be true unless EXEC_FLAG_BACKWARD was given,
> which'd cause us to create a tuplestore.  So if we were going
> to change anything, I'd propose adding something more like
> 
>         if (!forward && eof_tuplestore)
>         {
> +               Assert(node->eflags & EXEC_FLAG_BACKWARD);
>                 if (!node->eof_underlying)
>                 {
>                         /*
> 
> or perhaps more directly, Assert that tuplestore is not null.

I like  Assert(node->eflags & EXEC_FLAG_BACKWARD);
solution, it gives more information: "here we expect that BACKWARD eflag is 
set". (I am not quite familiar with this part of code, this knowledge in not 
obvious for me)
While Assert(tuplestorestate != NULL)  gives much less information about what 
is happening here.

And I think it is better to add this Assert there. People will continue using 
static analyzers on postgres code, finding this place again and again. Better 
to close this issue once and for all :-)

-- 
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su