pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
Fix another instance of unsafe coding for shm_toc_lookup failure.
One or another author of commit 5bcf389ec seems to have thought that
computing an offset from a NULL pointer would yield another NULL pointer.
There may possibly be architectures where that works, but common machines
don't work like that. Per a quick code review of places calling
shm_toc_lookup and not using noError = false.
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/d59ff4ab3111f20054425d82dab1393101dcfe8e
Modified Files
--------------
src/backend/executor/nodeHash.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
On Sat, Feb 3, 2018 at 12:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Fix another instance of unsafe coding for shm_toc_lookup failure.
One or another author of commit 5bcf389ec seems to have thought that
computing an offset from a NULL pointer would yield another NULL pointer.
There may possibly be architectures where that works, but common machines
don't work like that. Per a quick code review of places calling
shm_toc_lookup and not using noError = false.
Hmm... That was me. FWIW I certainly didn't think that about pointer
arithmetic... I think I must have got confused about the sense of that
flag. ExecHashInitializeWorker() should always find a TOC entry using
plan_node_id, because ExecHashInitializeDSM() always inserts one, so
my mistake was actually to put noError = true there when noError =
false was called for. However, it's not surprising that you drew the
opposite conclusion (ie that it might in fact not be in the TOC),
since the shm space is really only necessary for EXPLAIN ANALYZE.
Perhaps I should make it skip setting up this shm stuff if
!node->ss.ps.instrument, just like the equivalent Sort node code. I
will look into that on Monday.
--
Thomas Munro
http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Sat, Feb 3, 2018 at 12:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Fix another instance of unsafe coding for shm_toc_lookup failure.
my mistake was actually to put noError = true there when noError =
false was called for.
Ah.
However, it's not surprising that you drew the
opposite conclusion (ie that it might in fact not be in the TOC),
since the shm space is really only necessary for EXPLAIN ANALYZE.
Perhaps I should make it skip setting up this shm stuff if
!node->ss.ps.instrument, just like the equivalent Sort node code. I
will look into that on Monday.
OK. Please send in a patch to either do that or switch this call to use
noError = false. Or possibly both: shouldn't there be some other signal
path that tells the worker whether instrumentation is needed? I'll
leave it alone pending your investigation.
regards, tom lane
On Sat, Feb 3, 2018 at 3:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
However, it's not surprising that you drew the
opposite conclusion (ie that it might in fact not be in the TOC),
since the shm space is really only necessary for EXPLAIN ANALYZE.
Perhaps I should make it skip setting up this shm stuff if
!node->ss.ps.instrument, just like the equivalent Sort node code. I
will look into that on Monday.OK. Please send in a patch to either do that or switch this call to use
noError = false. Or possibly both: shouldn't there be some other signal
path that tells the worker whether instrumentation is needed? I'll
leave it alone pending your investigation.
Here's a patch do to both.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
skip-hash-instrumentation-if-not-needed.patchapplication/octet-stream; name=skip-hash-instrumentation-if-not-needed.patchDownload+17-7
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Sat, Feb 3, 2018 at 3:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
OK. Please send in a patch to either do that or switch this call to use
noError = false. Or possibly both: shouldn't there be some other signal
path that tells the worker whether instrumentation is needed? I'll
leave it alone pending your investigation.
Here's a patch do to both.
LGTM, pushed.
regards, tom lane