Fix use of possible uninitialized variable retval (src/pl/plpgsql/src/pl_handler.c)
Hi.
The function *plpgsql_inline_handler* can use uninitialized
variable retval, if PG_TRY fails.
Fix like function*plpgsql_call_handler* wich declare retval as
volatile and initialize to (Datum 0).
best regards,
Ranier Vilela
Attachments:
fix-use-uninitialized-retval-variable-pl_handler.patchapplication/octet-stream; name=fix-use-uninitialized-retval-variable-pl_handler.patchDownload
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index fce459ade0..3f7548cb9d 100644
--- a/src/pl/plpgsql/src/pl_handler.c
+++ b/src/pl/plpgsql/src/pl_handler.c
@@ -320,7 +320,7 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS)
FmgrInfo flinfo;
EState *simple_eval_estate;
ResourceOwner simple_eval_resowner;
- Datum retval;
+ volatile Datum retval = (Datum) 0;
int rc;
/*
At Mon, 27 May 2024 11:31:24 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
Hi.
The function *plpgsql_inline_handler* can use uninitialized
variable retval, if PG_TRY fails.
Fix like function*plpgsql_call_handler* wich declare retval as
volatile and initialize to (Datum 0).
If PG_TRY fails, retval is not actually accessed, so no real issue
exists. Commit 7292fd8f1c changed plpgsql_call_handler() to the
current form, but as stated in its commit message, it did not fix a
real issue and was solely to silence compiler.
I believe we do not need to modify plpgsql_inline_handler() unless
compiler actually issues a false warning for it.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, Jun 05, 2024 at 01:12:41PM +0900, Kyotaro Horiguchi wrote:
At Mon, 27 May 2024 11:31:24 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
The function *plpgsql_inline_handler* can use uninitialized
variable retval, if PG_TRY fails.
Fix like function*plpgsql_call_handler* wich declare retval as
volatile and initialize to (Datum 0).
You forgot to read elog.h, explaining under which circumstances
variables related to TRY/CATCH block should be marked as volatile.
There is a big "Note:" paragraph.
It is not the first time that this is mentioned on this list: but
sending a report without looking at the reason why a change is
justified makes everybody waste time. That's not productive.
If PG_TRY fails, retval is not actually accessed, so no real issue
exists. Commit 7292fd8f1c changed plpgsql_call_handler() to the
current form, but as stated in its commit message, it did not fix a
real issue and was solely to silence compiler.
This complain was from lapwing, that uses a version of gcc which
produces a lot of noise with incorrect issues. It is one of the only
32b buildfarm members, so it still has a lot of value.
I believe we do not need to modify plpgsql_inline_handler() unless
compiler actually issues a false warning for it.
If we were to do something, that would be to remove the volatile from
plpgsql_call_handler() at the end once we don't have in the buildfarm
compilers that complain about it, because there is no reason to use a
volatile in this case. :)
--
Michael
Em qua., 5 de jun. de 2024 às 01:12, Kyotaro Horiguchi <
horikyota.ntt@gmail.com> escreveu:
At Mon, 27 May 2024 11:31:24 -0300, Ranier Vilela <ranier.vf@gmail.com>
wrote inHi.
The function *plpgsql_inline_handler* can use uninitialized
variable retval, if PG_TRY fails.
Fix like function*plpgsql_call_handler* wich declare retval as
volatile and initialize to (Datum 0).If PG_TRY fails, retval is not actually accessed, so no real issue
exists.
You say it for this call
PG_RE_THROW();
Commit 7292fd8f1c changed plpgsql_call_handler() to the
current form, but as stated in its commit message, it did not fix a
real issue and was solely to silence compiler.
I believe we do not need to modify plpgsql_inline_handler() unless
compiler actually issues a false warning for it.
Yeah, there is a warning, but not from the compiler.
best regards,
Ranier Vilela
Em qua., 5 de jun. de 2024 às 02:04, Michael Paquier <michael@paquier.xyz>
escreveu:
On Wed, Jun 05, 2024 at 01:12:41PM +0900, Kyotaro Horiguchi wrote:
At Mon, 27 May 2024 11:31:24 -0300, Ranier Vilela <ranier.vf@gmail.com>
wrote in
The function *plpgsql_inline_handler* can use uninitialized
variable retval, if PG_TRY fails.
Fix like function*plpgsql_call_handler* wich declare retval as
volatile and initialize to (Datum 0).You forgot to read elog.h, explaining under which circumstances
variables related to TRY/CATCH block should be marked as volatile.
There is a big "Note:" paragraph.It is not the first time that this is mentioned on this list: but
sending a report without looking at the reason why a change is
justified makes everybody waste time. That's not productive.
Of course, this is very bad when it happens.
If PG_TRY fails, retval is not actually accessed, so no real issue
exists. Commit 7292fd8f1c changed plpgsql_call_handler() to the
current form, but as stated in its commit message, it did not fix a
real issue and was solely to silence compiler.This complain was from lapwing, that uses a version of gcc which
produces a lot of noise with incorrect issues. It is one of the only
32b buildfarm members, so it still has a lot of value.
I posted the report, because of an uninitialized variable warning.
Which is one of the most problematic situations, when it *actually exists*.
I believe we do not need to modify plpgsql_inline_handler() unless
compiler actually issues a false warning for it.If we were to do something, that would be to remove the volatile from
plpgsql_call_handler() at the end once we don't have in the buildfarm
compilers that complain about it, because there is no reason to use a
volatile in this case. :)
I don't see any motivation, since there are no reports.
best regards,
Ranier Vilela
On Wed, Jun 5, 2024 at 1:05 PM Michael Paquier <michael@paquier.xyz> wrote:
This complain was from lapwing, that uses a version of gcc which
produces a lot of noise with incorrect issues. It is one of the only
32b buildfarm members, so it still has a lot of value.
Note that I removed the -Werror from lapwing a long time ago, so at
least this animal shouldn't lead to hackish fixes for false positive
anymore.
On Wed, Jun 05, 2024 at 10:27:51PM +0800, Julien Rouhaud wrote:
Note that I removed the -Werror from lapwing a long time ago, so at
least this animal shouldn't lead to hackish fixes for false positive
anymore.
That's good to know. Thanks for the update.
--
Michael