Content-Type: multipart/alternative;
 boundary="------------AxAjPcR3VZCg7La7QzCDOtrm"
Message-ID: <87461d66-e96b-24f8-3e66-1daf937fff98@gmail.com>
Date: Sun, 26 Mar 2023 07: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/87461d66-e96b-24f8-3e66-1daf937fff98%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------AxAjPcR3VZCg7La7QzCDOtrm
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.
>

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
--------------AxAjPcR3VZCg7La7QzCDOtrm
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>
      </div>
    </blockquote>
    <br>
    I came to a conclusion that the padding is not affected by<br>
    palloc/palloc0 there.<br>
    There we have:<br>
                subdata[outer_nelems] = ARR_DATA_PTR(array);<br>
                subbytes[outer_nelems] = ARR_SIZE(array) -
    ARR_DATA_OFFSET(array);<br>
    ...<br>
                memcpy(dat, subdata[i], subbytes[i]);<br>
                dat += subbytes[i];<br>
    <br>
    So the result array elements come packed one after another from the
    input<br>
    array and are not padded here.<br>
    <br>
    <blockquote type="cite"
cite="mid:CAMbWs4_=kd4-u3Mh3tV+NcHuSGY-9Fax+Ck1ZVzOV0HzJ6dXow@mail.gmail.com">
      <div dir="ltr">
        <div class="gmail_quote">
          <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>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    Yeah, I see two instances of MemSet(newnullbitmap, 0, ...):<br>
    1) In array_set_element():<br>
            /* Zero the bitmap to take care of marking inserted
    positions null */<br>
            MemSet(newnullbitmap, 0, (newnitems + 7) / 8);<br>
    <br>
    It came with cecb60755 (2005-11-17).<br>
    <br>
    2) In array_set_slice():<br>
                /* Zero the bitmap to handle marking inserted positions
    null */<br>
                MemSet(newnullbitmap, 0, (nitems + 7) / 8);<br>
    <br>
    It came with 352a56ba6 (2006-09-29).<br>
    <br>
    These MemSets were meaningful before 18c0b4ecc (2011-04-27).<br>
    The commit 18c0b4ecc changed palloc() to palloc0() almost
    everywhere, so<br>
    that the MemSets became redundant, but at the same time the
    discussed<br>
    palloc() (residing in execQual.c:ExecEvalArray() back then) somehow
    evaded<br>
    the change.<br>
    <br>
    So maybe the fix for the bug can be seen as a supplement for
    18c0b4ecc.<br>
    <br>
    Best regards,<br>
    Alexander<br>
  </body>
</html>

--------------AxAjPcR3VZCg7La7QzCDOtrm--