Re: Check for tuplestorestate nullness before dereferencing

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