Re: ARRNELEMS Out-of-bounds possible errors

[Ranier Vilela]

Em seg., 26 de dez. de 2022 às 15:45, Nikita Malakhov 
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

Re: ARRNELEMS Out-of-bounds possible errors

[Nikita Malakhov]

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  wrote:

> Nikita Malakhov  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/

Re: ARRNELEMS Out-of-bounds possible errors

[Tom Lane]

Nikita Malakhov  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

Re: ARRNELEMS Out-of-bounds possible errors

[Nikita Malakhov]

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 
wrote:

> At Fri, 23 Dec 2022 17:37:55 +0900 (JST), Kyotaro Horiguchi <
> horikyota@gmail.com> wrote in
> > At Thu, 22 Dec 2022 12:35:58 -0300, Ranier Vilela 
> 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/

Re: ARRNELEMS Out-of-bounds possible errors

[Kyotaro Horiguchi]

At Fri, 23 Dec 2022 17:37:55 +0900 (JST), Kyotaro Horiguchi 
 wrote in 
> At Thu, 22 Dec 2022 12:35:58 -0300, Ranier Vilela  wrote 
> in 
> > 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."
> 
> 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

Re: ARRNELEMS Out-of-bounds possible errors

[Kyotaro Horiguchi]

At Thu, 22 Dec 2022 12:35:58 -0300, Ranier Vilela  wrote 
in 
> 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."

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

Re: ARRNELEMS Out-of-bounds possible errors

[Nikita Malakhov]

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  wrote:

> Em qui., 22 de dez. de 2022 às 15:45, Nikita Malakhov 
> 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/

Re: ARRNELEMS Out-of-bounds possible errors

[Ranier Vilela]

Em qui., 22 de dez. de 2022 às 15:45, Nikita Malakhov 
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

Re: ARRNELEMS Out-of-bounds possible errors

[Nikita Malakhov]

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  wrote:

> 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
>


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