Обсуждение: Fix misuse use of window_gettupleslot function (src/backend/executor/nodeWindowAgg.c)

Поиск
Список
Период
Сортировка

Fix misuse use of window_gettupleslot function (src/backend/executor/nodeWindowAgg.c)

От
Ranier Vilela
Дата:
Hi.

Per Coverity.

CID 1635309: (#1 of 1): Unchecked return value (CHECKED_RETURN)
7. check_return: Calling window_gettupleslot without checking return value (as is done elsewhere 8 out of 9 times).

The function "window_gettupleslot" can fail.

All other calls check the return, In this case it could not be different.

Fix by checking the return and reporting a message to the user,
in case of failure.
The error message, however, I'm not sure if it's ideal.

best regards,
Ranier Vilela

Вложения

Re: Fix misuse use of window_gettupleslot function (src/backend/executor/nodeWindowAgg.c)

От
Tom Lane
Дата:
Ranier Vilela <ranier.vf@gmail.com> writes:
> Per Coverity.
> CID 1635309: (#1 of 1): Unchecked return value (CHECKED_RETURN)
> 7. check_return: Calling window_gettupleslot without checking return value
> (as is done elsewhere 8 out of 9 times).

Yeah, the security team's Coverity instance just whined about that
too.  But isn't the correct behavior simply "return -1"?  It looks
to me like a failure should be interpreted as "row doesn't exist,
therefore it's not in frame".

What would be really useful is a test case that reaches this
condition.  That would make it plain what to do.

            regards, tom lane

Re: Fix misuse use of window_gettupleslot function (src/backend/executor/nodeWindowAgg.c)

От
Ranier Vilela
Дата:

Em dom., 5 de out. de 2025 às 13:05, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
> Per Coverity.
> CID 1635309: (#1 of 1): Unchecked return value (CHECKED_RETURN)
> 7. check_return: Calling window_gettupleslot without checking return value
> (as is done elsewhere 8 out of 9 times).

Yeah, the security team's Coverity instance just whined about that
too.  But isn't the correct behavior simply "return -1"?
It seems to me a better option.
 
  It looks
to me like a failure should be interpreted as "row doesn't exist,
therefore it's not in frame".
I also believe that the original author did not expect a failure here.


What would be really useful is a test case that reaches this
condition.  That would make it plain what to do.
There is a comment above that indicates that possibly a failure could also be the end of the partition.

v1 patch attached.

best regards,
Ranier Vilela
Вложения

Re: Fix misuse use of window_gettupleslot function (src/backend/executor/nodeWindowAgg.c)

От
Ranier Vilela
Дата:


Em seg., 6 de out. de 2025 às 08:33, Ranier Vilela <ranier.vf@gmail.com> escreveu:

Em dom., 5 de out. de 2025 às 13:05, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
> Per Coverity.
> CID 1635309: (#1 of 1): Unchecked return value (CHECKED_RETURN)
> 7. check_return: Calling window_gettupleslot without checking return value
> (as is done elsewhere 8 out of 9 times).

Yeah, the security team's Coverity instance just whined about that
too.  But isn't the correct behavior simply "return -1"?
It seems to me a better option.
 
  It looks
to me like a failure should be interpreted as "row doesn't exist,
therefore it's not in frame".
I also believe that the original author did not expect a failure here.


What would be really useful is a test case that reaches this
condition.  That would make it plain what to do.
There is a comment above that indicates that possibly a failure could also be the end of the partition.

v1 patch attached.
It seems to me that this issue is being addressed in another thread. [1]
I'll withdraw these patch.

best regards,
Ranier Vilela