Content-Type: multipart/alternative;
 boundary="------------0HnbQH2IxeoqrEfLaNAln0c8"
Message-ID: <3c2ca9f5-a69a-71e2-1d70-6c33702009b2@gmail.com>
Date: Wed, 22 Mar 2023 10:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #17858: ExecEvalArrayExpr() leaves uninitialised memory for
 multidim array with nulls
Content-Language: en-US
To: Richard Guo <guofenglinux@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
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>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: 
 <CAMbWs4_=kd4-u3Mh3tV+NcHuSGY-9Fax+Ck1ZVzOV0HzJ6dXow@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/3c2ca9f5-a69a-71e2-1d70-6c33702009b2%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------0HnbQH2IxeoqrEfLaNAln0c8
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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
--------------0HnbQH2IxeoqrEfLaNAln0c8
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <div class="moz-cite-prefix">22.03.2023 06:07, Richard Guo wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:CAMbWs4_=kd4-u3Mh3tV+NcHuSGY-9Fax+Ck1ZVzOV0HzJ6dXow@mail.gmail.com">
      <meta http-equiv="content-type" content="text/html; charset=UTF-8">
      <div dir="ltr">
        <div dir="ltr"><br>
        </div>
        <div class="gmail_quote">
          <div dir="ltr" class="gmail_attr">On Tue, Mar 21, 2023 at
            10:24 PM Tom Lane &lt;<a href="mailto:tgl@sss.pgh.pa.us"
              moz-do-not-send="true" class="moz-txt-link-freetext">tgl@sss.pgh.pa.us</a>&gt;
            wrote:<br>
          </div>
          <blockquote class="gmail_quote" style="margin:0px 0px 0px
            0.8ex;border-left:1px solid
            rgb(204,204,204);padding-left:1ex">Alexander Lakhin &lt;<a
              href="mailto:exclusion@gmail.com" target="_blank"
              moz-do-not-send="true" class="moz-txt-link-freetext">exclusion@gmail.com</a>&gt;
            writes:<br>
            &gt; I'm afraid that zeroing only bytes behind nitems bits
            is not enough, as outDatum() doesn't bother to calculate the
            exact <br>
            &gt; size of nulls bitmap, it just outputs all bytes of a
            datum (40 bytes in that case):<br>
            <br>
            In that case, won't padding bytes between array elements
            also create<br>
            issues?  Seems like we have to just zero the whole array
            area, like<br>
            those other functions do.</blockquote>
          <div><br>
          </div>
          <div>Yeah, this should be the right fix, to use palloc0
            instead here.  FWIW<br>
            currently in the codes there are 14 places that explicitly
            allocate<br>
            ArrayType, 13 of them using palloc0, the only exception is
            the one<br>
            discussed here.<br>
            <br>
            BTW, in array_set_slice() and array_set_element() we
            explicitly zero the<br>
            null bitmap although the whole array area is allocated with
            palloc0.  Is<br>
            this necessary?<br>
            <br>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    I had also wondered, how come this call of array_bitmap_copy() shown
    as covered here:<br>
<a class="moz-txt-link-freetext" href="https://coverage.postgresql.org/src/backend/executor/execExprInterp.c.gcov.html">https://coverage.postgresql.org/src/backend/executor/execExprInterp.c.gcov.html</a><br>
    but valgrind doesn't complain during `make check` (with
    WRITE_READ_PARSE_PLAN_TREES).<br>
    I've found the query, which is reaching that call:<br>
    select percentile_disc(array[[null,1,0.5],[0.75,0.25,null]]) within
    group (order by thousand)<br>
    from tenk1;<br>
    and the absence of the valgrind complaint here is explained by the
    array coercion performed implicitly.<br>
    The query modification:<br>
    select
    percentile_disc(array[[null,1,0.5],[0.75,0.25,null]]::float8[])
    within group (order by thousand)<br>
    from tenk1;<br>
    can confirm the fix (if needed).<br>
    <br>
    With the following check:<br>
    if (result) VALGRIND_CHECK_MEM_IS_DEFINED(result, VARSIZE(result));<br>
    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:<br>
     * The actual data starts on a MAXALIGN boundary.  Individual items
    in the<br>
     * array are aligned as specified by the array element type. ...<br>
    <br>
    I'll try to find cases where the alignment padding is possible...<br>
    <br>
    Though if palloc0() used in 13 places, maybe the minor optimization
    (with zeroing only the null bitmap) doesn't justify the
    inconsistency.<br>
    <br>
    Best regards,<br>
    Alexander<br>
    <span style="font-family:monospace"></span>
  </body>
</html>

--------------0HnbQH2IxeoqrEfLaNAln0c8--