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