NULL-pointer check and incorrect comment for pstate in addRangeTableEntry
Hi all,
Coverity is pointing out that addRangeTableEntry contains the
following code path that does a NULL-pointer check on pstate:
if (pstate != NULL)
pstate->p_rtable = lappend(pstate->p_rtable, rte);
But pstate is dereferenced before in isLockedRefname when grabbing the
lock mode:
lockmode = isLockedRefname(pstate, refname) ? RowShareLock : AccessShareLock;
Note that there is as well the following comment that is confusing on
top of addRangeTableEntry:
* If pstate is NULL, we just build an RTE and return it without adding it
* to an rtable list.
So I have looked at all the code paths calling this function and found
first that the only potential point where pstate could be NULL is
transformTopLevelStmt in analyze.c. One level higher there are
parse_analyze_varparams and parse_analyze that may use pstate as NULL,
and even one level more higher in the stack there is
pg_analyze_and_rewrite. But well, looking at each case individually,
in all cases we never pass NULL for the parse tree node, so I think
that we should remove the comment on top of addRangeTableEntry, remove
the NULL-pointer check and add an assertion as in the patch attached.
Thoughts?
Regards,
--
Michael
Attachments:
20150225_parse_state_null_ptrs.patchtext/x-patch; charset=US-ASCII; name=20150225_parse_state_null_ptrs.patchDownload+3-5
On Wed, Feb 25, 2015 at 7:27 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Coverity is pointing out that addRangeTableEntry contains the
following code path that does a NULL-pointer check on pstate:
if (pstate != NULL)
pstate->p_rtable = lappend(pstate->p_rtable, rte);
But pstate is dereferenced before in isLockedRefname when grabbing the
lock mode:
lockmode = isLockedRefname(pstate, refname) ? RowShareLock : AccessShareLock;Note that there is as well the following comment that is confusing on
top of addRangeTableEntry:
* If pstate is NULL, we just build an RTE and return it without adding it
* to an rtable list.So I have looked at all the code paths calling this function and found
first that the only potential point where pstate could be NULL is
transformTopLevelStmt in analyze.c. One level higher there are
parse_analyze_varparams and parse_analyze that may use pstate as NULL,
and even one level more higher in the stack there is
pg_analyze_and_rewrite. But well, looking at each case individually,
in all cases we never pass NULL for the parse tree node, so I think
that we should remove the comment on top of addRangeTableEntry, remove
the NULL-pointer check and add an assertion as in the patch attached.Thoughts?
That seems to make sense to me. Committed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Mar 4, 2015 at 6:43 AM, Robert Haas wrote:
That seems to make sense to me. Committed.
Thanks.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
This sounded familiar... I pointed out the same thing a while back and Tom
had some feedback on what to do about it:
/messages/by-id/23294.1384142407@sss.pgh.pa.us
On 4 Mar 2015 00:37, "Michael Paquier" <michael.paquier@gmail.com> wrote:
Show quoted text
On Wed, Mar 4, 2015 at 6:43 AM, Robert Haas wrote:
That seems to make sense to me. Committed.
Thanks.
--
Michael--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Mar 4, 2015 at 9:12 PM, Greg Stark <stark@mit.edu> wrote:
This sounded familiar... I pointed out the same thing a while back and Tom
had some feedback on what to do about it:
Translated into code I guess that this gives the attached patch.
--
Michael
Attachments:
20150305_dummy_pstate_fixes.patchtext/x-patch; charset=US-ASCII; name=20150305_dummy_pstate_fixes.patchDownload+29-15
On Wed, Mar 4, 2015 at 12:38 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Translated into code I guess that this gives the attached patch.
Probably want to update a comment somewhere but yes, those are the
same three call sites I had found back then. I don't see any patch
lying around so I don't think I got any further than searching for
call sites.
--
greg
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Mar 4, 2015 at 7:38 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Mar 4, 2015 at 9:12 PM, Greg Stark <stark@mit.edu> wrote:
This sounded familiar... I pointed out the same thing a while back and Tom
had some feedback on what to do about it:Translated into code I guess that this gives the attached patch.
Looks fine to me, so committed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Mar 12, 2015 at 4:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Mar 4, 2015 at 7:38 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Mar 4, 2015 at 9:12 PM, Greg Stark <stark@mit.edu> wrote:
This sounded familiar... I pointed out the same thing a while back and Tom
had some feedback on what to do about it:Translated into code I guess that this gives the attached patch.
Looks fine to me, so committed.
Thanks for picking this up.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers