Thread: ARRNELEMS Out-of-bounds possible errors

Hi.

Per Coverity.

The commit ccff2d2, changed the behavior function ArrayGetNItems,

with the introduction of the function ArrayGetNItemsSafe.

Now ArrayGetNItems may return -1, according to the comment.

" instead of throwing an exception. -1 is returned after an error."

So the macro ARRNELEMS can fail entirely with -1 return,

resulting in codes failing to use without checking the function return.

Like (contrib/intarray/_int_gist.c):

{

int nel;

nel = ARRNELEMS(ent);
memcpy(ptr, ARRPTR(ent), nel * sizeof(int32));

}

Sources possibly affecteds:
contrib\cube\cube.c
contrib\intarray\_intbig_gist.c

contrib\intarray\_int_bool.c

contrib\intarray\_int_gin.c

contrib\intarray\_int_gist.c

contrib\intarray\_int_op.c
contrib\intarray\_int_tool.c:

Thoughts?

regards,

Ranier Vilela

Hi,

Actually, there would be much more sources affected, like

         nbytes += subbytes[outer_nelems];

         subnitems[outer_nelems] = ArrayGetNItems(this_ndims,

                                        ARR_DIMS(array));

         nitems += subnitems[outer_nelems];

         havenulls |= ARR_HASNULL(array);

         outer_nelems++;

      }

Maybe it is better for most calls like this to keep old behavior, by passing a flag

that says which behavior is expected by caller?

Em qui., 22 de dez. de 2022 às 15:45, Nikita Malakhov <hukutoc@gmail.com> escreveu:

Hi,

Actually, there would be much more sources affected, like

         nbytes += subbytes[outer_nelems];

         subnitems[outer_nelems] = ArrayGetNItems(this_ndims,

                                        ARR_DIMS(array));

         nitems += subnitems[outer_nelems];

         havenulls |= ARR_HASNULL(array);

         outer_nelems++;

      }

Maybe it is better for most calls like this to keep old behavior, by passing a flag

that says which behavior is expected by caller?

I agreed that it is better to keep old behavior.

Even the value 0 is problematic, with calls like this:

nel = ARRNELEMS(ent);
memcpy(ptr, ARRPTR(ent), nel * sizeof(int32));

regards,

Ranier Vilela

At Fri, 23 Dec 2022 17:37:55 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> At Thu, 22 Dec 2022 12:35:58 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> > Hi.
> >
> > Per Coverity.
> >
> > The commit ccff2d2
> > <https://github.com/postgres/postgres/commit/ccff2d20ed9622815df2a7deffce8a7b14830965>,
> > changed the behavior function ArrayGetNItems,
> > with the introduction of the function ArrayGetNItemsSafe.
> >
> > Now ArrayGetNItems may return -1, according to the comment.
> > " instead of throwing an exception. -1 is returned after an error."
>
> If I'm reading the code correctly, it's the definition of
> ArrayGetNItems*Safe*. ArrayGetNItems() calls that function with a NULL
> escontext and the NULL turns ereturn() into ereport().

> That doesn't seem to be changed by the commit.

No.. It seems to me that the commit didn't change its behavior in that
regard.

> Of course teaching Coverity not to issue the false warnings would be
> another actual issue that we should do, maybe.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Nikita Malakhov <hukutoc@gmail.com> writes:
> Even with null context it does not turn to ereport, and returns dummy value

Read the code.  ArrayGetNItems passes NULL for escontext, therefore
if there's a problem the ereturn calls in ArrayGetNItemsSafe will
throw error, *not* return -1.

Not sure how we could persuade Coverity of that, though,
if it fails to understand that for itself.

                        regards, tom lane

Hi,

My bad, I was misleaded by unconditional return in ereturn_domain.

Sorry for the noise.