Thread: Re: Check for tuplestorestate nullness before dereferencing

Re: Check for tuplestorestate nullness before dereferencing

From
Ilia Evdokimov
Date:
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.




Re: Check for tuplestorestate nullness before dereferencing

From
Alena Rybakina
Date:
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




Re: Check for tuplestorestate nullness before dereferencing

From
Alexander Kuznetsov
Date:
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
tohandle a tuplestorestate of NULL, and this error can be accessed if the flags are not zero, but I think these cases
havebeen worked out.
 
> [...]
> After the subplan scan is complete, we see that the eof_underlying variable becomes true, and this part of the code
willno 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



Re: Check for tuplestorestate nullness before dereferencing

From
David Rowley
Date:
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