Thread: Re: Check for tuplestorestate nullness before dereferencing
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.
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
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
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
В письме от пятница, 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
Attachment
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
В письме от вторник, 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