From: Alexander Lakhin <exclusion@gmail.com>
To: Richard Guo <guofenglinux@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17858: ExecEvalArrayExpr() leaves uninitialised memory for multidim array with nulls
Date: Sun, 26 Mar 2023 07:00:00 +0300
Message-ID: <87461d66-e96b-24f8-3e66-1daf937fff98@gmail.com> (raw)
In-Reply-To: <CAMbWs4_=kd4-u3Mh3tV+NcHuSGY-9Fax+Ck1ZVzOV0HzJ6dXow@mail.gmail.com>
References: <17858-8fd287fd3663d051@postgresql.org>
<CAMbWs48CyDghUckGQbkbAB6uPdfzYS4X4Zkdfooausr5vaV9Bg@mail.gmail.com>
<4240d86f-7e9f-388b-fd96-f8944860c0f5@gmail.com>
<1216375.1679408668@sss.pgh.pa.us>
<CAMbWs4_=kd4-u3Mh3tV+NcHuSGY-9Fax+Ck1ZVzOV0HzJ6dXow@mail.gmail.com>
22.03.2023 06:07, Richard Guo wrote:
>
> On Tue, Mar 21, 2023 at 10:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alexander Lakhin <exclusion@gmail.com> writes:
> > I'm afraid that zeroing only bytes behind nitems bits is not enough, as outDatum() doesn't bother to calculate
> the exact
> > size of nulls bitmap, it just outputs all bytes of a datum (40 bytes in that case):
>
> In that case, won't padding bytes between array elements also create
> issues? Seems like we have to just zero the whole array area, like
> those other functions do.
>
I came to a conclusion that the padding is not affected by
palloc/palloc0 there.
There we have:
subdata[outer_nelems] = ARR_DATA_PTR(array);
subbytes[outer_nelems] = ARR_SIZE(array) - ARR_DATA_OFFSET(array);
...
memcpy(dat, subdata[i], subbytes[i]);
dat += subbytes[i];
So the result array elements come packed one after another from the input
array and are not padded here.
>
> Yeah, this should be the right fix, to use palloc0 instead here. FWIW
> currently in the codes there are 14 places that explicitly allocate
> ArrayType, 13 of them using palloc0, the only exception is the one
> discussed here.
>
> BTW, in array_set_slice() and array_set_element() we explicitly zero the
> null bitmap although the whole array area is allocated with palloc0. Is
> this necessary?
Yeah, I see two instances of MemSet(newnullbitmap, 0, ...):
1) In array_set_element():
/* Zero the bitmap to take care of marking inserted positions null */
MemSet(newnullbitmap, 0, (newnitems + 7) / 8);
It came with cecb60755 (2005-11-17).
2) In array_set_slice():
/* Zero the bitmap to handle marking inserted positions null */
MemSet(newnullbitmap, 0, (nitems + 7) / 8);
It came with 352a56ba6 (2006-09-29).
These MemSets were meaningful before 18c0b4ecc (2011-04-27).
The commit 18c0b4ecc changed palloc() to palloc0() almost everywhere, so
that the MemSets became redundant, but at the same time the discussed
palloc() (residing in execQual.c:ExecEvalArray() back then) somehow evaded
the change.
So maybe the fix for the bug can be seen as a supplement for 18c0b4ecc.
Best regards,
Alexander
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: exclusion@gmail.com, guofenglinux@gmail.com, tgl@sss.pgh.pa.us, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17858: ExecEvalArrayExpr() leaves uninitialised memory for multidim array with nulls
In-Reply-To: <87461d66-e96b-24f8-3e66-1daf937fff98@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox