ARRNELEMS Out-of-bounds possible errors

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?

On Thu, Dec 22, 2022 at 6:36 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

Hi.

Per Coverity.

The commit ccff2d2
<https://github.com/postgres/postgres/commit/ccff2d20ed9622815df2a7deffce8a7b14830965&gt;,
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

--
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

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

Hi,

The most obvious solution I see is to check all calls and for cases like we
both mentioned
to pass a flag meaning safe or unsafe (for these cases) behavior is
expected, like

#define ARRNELEMS(x) ArrayGetNItems( ARR_NDIM(x), ARR_DIMS(x), false)

...

int
ArrayGetNItems(int ndim, const int *dims, bool issafe)
{
return ArrayGetNItemsSafe(ndim, dims, NULL, issafe);
}

int
ArrayGetNItemsSafe(int ndim, const int *dims, struct Node *escontext, bool
issafe)
{
...

Another solution is revision of wrapping code for all calls of
ArrayGetNItems.
Safe functions is a good idea overall, but a lot of code needs to be
revised.

On Fri, Dec 23, 2022 at 1:20 AM Ranier Vilela <ranier.vf@gmail.com> wrote:

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

--
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

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&gt;,
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.

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

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

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&gt;,
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

Hi,

Even with null context it does not turn to ereport, and returns dummy value
-

#define errsave_domain(context, domain, ...) \
do { \
struct Node *context_ = (context); \
pg_prevent_errno_in_scope(); \
if (errsave_start(context_, domain)) \
__VA_ARGS__, errsave_finish(context_, __FILE__, __LINE__, __func__); \
} while(0)

#define errsave(context, ...) \
errsave_domain(context, TEXTDOMAIN, __VA_ARGS__)

/*
* "ereturn(context, dummy_value, ...);" is exactly the same as
* "errsave(context, ...); return dummy_value;". This saves a bit
* of typing in the common case where a function has no cleanup
* actions to take after reporting a soft error. "dummy_value"
* can be empty if the function returns void.
*/
#define ereturn_domain(context, dummy_value, domain, ...) \
do { \
errsave_domain(context, domain, __VA_ARGS__); \
return dummy_value; \
} while(0)

#define ereturn(context, dummy_value, ...) \
ereturn_domain(context, dummy_value, TEXTDOMAIN, __VA_ARGS__)

On Fri, Dec 23, 2022 at 11:40 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

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

--
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

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.

On Sat, Dec 24, 2022 at 7:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

--
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

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

Hi,

My bad, I was misleaded by unconditional return in ereturn_domain.
Sorry for the noise.

By no means, the mistake was entirely mine, I apologize to you.

regards,
Ranier Vilela