Re: BUG #17858: ExecEvalArrayExpr() leaves uninitialised memory for multidim array with nulls

22.03.2023 06:07, Richard Guo wrote:
>
> On Tue, Mar 21, 2023 at 10:24 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Alexander Lakhin <exclusion(at)gmail(dot)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