Possible pointer dereference
I'm playing with a static analyzer and it's giving out some real error
analyzing postgresql code base like the following one
src/backend/access/transam/commit_ts.c
return *ts != 0 // line 321
but a few line up (line 315) ts is checked for null, so either is not
needed to check for null or *ts can lead to a null pointer dereference.
Same happens a few line later lines 333 and 339
Regards
Gaetano
On Thu, May 28, 2015 at 6:07 AM, Gaetano Mendola <mendola@gmail.com> wrote:
I'm playing with a static analyzer and it's giving out some real error
analyzing postgresql code base like the following onesrc/backend/access/transam/commit_ts.c
return *ts != 0 // line 321
but a few line up (line 315) ts is checked for null, so either is not needed
to check for null or *ts can lead to a null pointer dereference. Same
happens a few line later lines 333 and 339
Thanks for providing detailed information.
The function "TransactionIdGetCommitTsData" is currently used only at
one place. The caller
always passes an valid pointer to this function. So there shouldn't be
a problem. But in future
if the same function is used at somewhere by passing the NULL pointer
then it leads to a crash.
By correcting the following way will solve the problem.
return ts ? (*ts != 0) : false; instead of retun *ts != 0;
Attached a patch for it.
Regards,
Hari Babu
Fujitsu Australia
Attachments:
commit_ts_fix.patchapplication/octet-stream; name=commit_ts_fix.patchDownload
*** a/src/backend/access/transam/commit_ts.c
--- b/src/backend/access/transam/commit_ts.c
***************
*** 318,324 **** TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
*nodeid = commitTsShared->dataLastCommit.nodeid;
LWLockRelease(CommitTsLock);
! return *ts != 0;
}
LWLockRelease(CommitTsLock);
}
--- 318,324 ----
*nodeid = commitTsShared->dataLastCommit.nodeid;
LWLockRelease(CommitTsLock);
! return ts ? (*ts != 0) : false;
}
LWLockRelease(CommitTsLock);
}
***************
*** 336,342 **** TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
*nodeid = entry.nodeid;
LWLockRelease(CommitTsControlLock);
! return *ts != 0;
}
/*
--- 336,342 ----
*nodeid = entry.nodeid;
LWLockRelease(CommitTsControlLock);
! return ts ? (*ts != 0) : false;
}
/*
On Wed, May 27, 2015 at 8:57 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
On Thu, May 28, 2015 at 6:07 AM, Gaetano Mendola <mendola@gmail.com> wrote:
I'm playing with a static analyzer and it's giving out some real error
analyzing postgresql code base like the following onesrc/backend/access/transam/commit_ts.c
return *ts != 0 // line 321
but a few line up (line 315) ts is checked for null, so either is not needed
to check for null or *ts can lead to a null pointer dereference. Same
happens a few line later lines 333 and 339Thanks for providing detailed information.
The function "TransactionIdGetCommitTsData" is currently used only at
one place. The caller
always passes an valid pointer to this function. So there shouldn't be
a problem. But in future
if the same function is used at somewhere by passing the NULL pointer
then it leads to a crash.By correcting the following way will solve the problem.
return ts ? (*ts != 0) : false; instead of retun *ts != 0;
Attached a patch for it.
If the only caller always passes a valid pointer, there's no point in
adding this check. We have many functions in our source base that
assume that the caller will pass a valid pointer, and changing them
all would make the code bigger, harder to read, and possibly slower,
without any real benefit.
--
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
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, May 27, 2015 at 8:57 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:By correcting the following way will solve the problem.
return ts ? (*ts != 0) : false; instead of retun *ts != 0;
Attached a patch for it.
If the only caller always passes a valid pointer, there's no point in
adding this check. We have many functions in our source base that
assume that the caller will pass a valid pointer, and changing them
all would make the code bigger, harder to read, and possibly slower,
without any real benefit.
Well, we should either install something like Haribabu's patch, or else
remove the existing tests in the function that allow "ts" to be NULL.
And the function's API contract comment needs to be clarified in either
case; the real bug here is lack of a specification.
I don't particularly have an opinion on whether it's valuable to allow
this function to be called without receiving a timestamp back. Perhaps
the authors of the patch can comment on that.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
While at it the assert(cnfa != NULL && cnfa->nstates != 0); at
src/backend/regex/rege_dfa.c:282
is issued too late indeed at line 278 and 279 cnfa was already
dereferenced.
Same for assert(t != NULL) in src/backend/regex/regexec.c:821 is issued way
too late.
On Thu, 28 May 2015 at 15:59 Tom Lane <tgl@sss.pgh.pa.us> wrote:
Show quoted text
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, May 27, 2015 at 8:57 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:By correcting the following way will solve the problem.
return ts ? (*ts != 0) : false; instead of retun *ts != 0;
Attached a patch for it.If the only caller always passes a valid pointer, there's no point in
adding this check. We have many functions in our source base that
assume that the caller will pass a valid pointer, and changing them
all would make the code bigger, harder to read, and possibly slower,
without any real benefit.Well, we should either install something like Haribabu's patch, or else
remove the existing tests in the function that allow "ts" to be NULL.
And the function's API contract comment needs to be clarified in either
case; the real bug here is lack of a specification.I don't particularly have an opinion on whether it's valuable to allow
this function to be called without receiving a timestamp back. Perhaps
the authors of the patch can comment on that.regards, tom lane