Subject: Re: Fix use of possible uninitialized variable retval (src/pl/plpgsql/src/pl_handler.c)

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 05 Jun 2024 13:12:41 +0900 (JST)

At Mon, 27 May 2024 11:31:24 -0300, Ranier Vilela <[email-masked]> 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

---

From: Michael Paquier <[email-masked]>
Date: Wed, 5 Jun 2024 14:04:39 +0900

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

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

> 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

---

From: Ranier Vilela <[email-masked]>
Date: Wed, 5 Jun 2024 09:28:05 -0300

Em qua., 5 de jun. de 2024 às 01:12, Kyotaro Horiguchi <
[email-masked]> escreveu:

> exists.

You say it for this call
PG_RE_THROW();

> Commit 7292fd8f1c changed plpgsql_call_handler() to the

> compiler actually issues a false warning for it.
Yeah, there is a warning, but not from the compiler.

best regards,
Ranier Vilela

---

From: Ranier Vilela <[email-masked]>
Date: Wed, 5 Jun 2024 09:34:12 -0300

> justified makes everybody waste time.  That's not productive.
Of course, this is very bad when it happens.

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

> volatile in this case.  :)
I don't see any motivation, since there are no reports.

best regards,
Ranier Vilela

---

From: Julien Rouhaud <[email-masked]>
Date: Wed, 5 Jun 2024 22:27:51 +0800

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

---

From: Michael Paquier <[email-masked]>
Date: Thu, 6 Jun 2024 13:29:26 +0900

> least this animal shouldn't lead to hackish fixes for false positive

That's good to know.  Thanks for the update.
--
Michael