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: Wed, 22 Mar 2023 10:00:00 +0300
Message-ID: <3c2ca9f5-a69a-71e2-1d70-6c33702009b2@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.
>
>
> 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?
>
I had also wondered, how come this call of array_bitmap_copy() shown as covered here:
https://coverage.postgresql.org/src/backend/executor/execExprInterp.c.gcov.html
but valgrind doesn't complain during `make check` (with WRITE_READ_PARSE_PLAN_TREES).
I've found the query, which is reaching that call:
select percentile_disc(array[[null,1,0.5],[0.75,0.25,null]]) within group (order by thousand)
from tenk1;
and the absence of the valgrind complaint here is explained by the array coercion performed implicitly.
The query modification:
select percentile_disc(array[[null,1,0.5],[0.75,0.25,null]]::float8[]) within group (order by thousand)
from tenk1;
can confirm the fix (if needed).
With the following check:
if (result) VALGRIND_CHECK_MEM_IS_DEFINED(result, VARSIZE(result));
added at the end of ExecEvalArrayExpr() I see no failures during `make check` (except for select percentile_disc(...))
so it seems that uninitialised padding bytes between array elements are not produced with the existing regression tests.
Though a comment in array.h says:
* The actual data starts on a MAXALIGN boundary. Individual items in the
* array are aligned as specified by the array element type. ...
I'll try to find cases where the alignment padding is possible...
Though if palloc0() used in 13 places, maybe the minor optimization (with zeroing only the null bitmap) doesn't justify
the inconsistency.
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: <3c2ca9f5-a69a-71e2-1d70-6c33702009b2@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