Обсуждение: pgsql: Fix another instance of unsafe coding for shm_toc_lookupfailure
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
Вложения
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