pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedgcc -Wclobbered in PostgresMain
4+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2023-07-10 a8b7424684 0c32818d75 671bf1cf27 6b51fe8340 93dcdfa88f 22447db17c 48582cf9ed Be more rigorous about local variables in PostgresMain().
* gcc -Wclobbered in PostgresMain
@ 2023-07-07 12:13 Sergey Shinderuk <s.shinderuk@postgrespro.ru>
0 siblings, 1 reply; 4+ messages in thread
From: Sergey Shinderuk @ 2023-07-07 12:13 UTC (permalink / raw)
To: pgsql-hackers
Hi, hackers!
While analyzing -Wclobbered warnings from gcc we found a true one in
PostgresMain():
postgres.c: In function ‘PostgresMain’:
postgres.c:4118:25: warning: variable
‘idle_in_transaction_timeout_enabled’ might be clobbered by ‘longjmp’ or
‘vfork’ [-Wclobbered]
4118 | bool idle_in_transaction_timeout_enabled =
false;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
postgres.c:4119:25: warning: variable ‘idle_session_timeout_enabled’
might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
4119 | bool idle_session_timeout_enabled = false;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
These variables must be declared volatile, because they are read after
longjmp(). send_ready_for_query declared there is volatile.
Without volatile, these variables are kept in registers and restored by
longjump(). I think, this is harmless because the error handling code
calls disable_all_timeouts() anyway. But strictly speaking the code is
invoking undefined behavior by reading those variables after longjmp(),
so it's worth fixing. And for consistency with send_ready_for_query too.
I believe, making them volatile doesn't affect performance in any way.
I also moved firstchar's declaration inside the loop where it's used, to
make it clear that this variable needn't be volatile and is not
preserved after longjmp().
Best regards,
--
Sergey Shinderuk https://postgrespro.com/
Attachments:
missing-volatile-longjmp.diff (text/x-patch, 962B) download
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 01b6cc1f7d3..56d8b0814b5 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4111,12 +4111,11 @@ PostgresSingleUserMain(int argc, char *argv[],
void
PostgresMain(const char *dbname, const char *username)
{
- int firstchar;
StringInfoData input_message;
sigjmp_buf local_sigjmp_buf;
volatile bool send_ready_for_query = true;
- bool idle_in_transaction_timeout_enabled = false;
- bool idle_session_timeout_enabled = false;
+ volatile bool idle_in_transaction_timeout_enabled = false;
+ volatile bool idle_session_timeout_enabled = false;
Assert(dbname != NULL);
Assert(username != NULL);
@@ -4418,6 +4417,8 @@ PostgresMain(const char *dbname, const char *username)
for (;;)
{
+ int firstchar;
+
/*
* At top of loop, reset extended-query-message flag, so that any
* errors encountered in "idle" state don't provoke skip.
* Re: gcc -Wclobbered in PostgresMain
@ 2023-07-08 15:11 Tom Lane <tgl@sss.pgh.pa.us>
parent: Sergey Shinderuk <s.shinderuk@postgrespro.ru>
0 siblings, 1 reply; 4+ messages in thread
From: Tom Lane @ 2023-07-08 15:11 UTC (permalink / raw)
To: Sergey Shinderuk <s.shinderuk@postgrespro.ru>; +Cc: pgsql-hackers
Sergey Shinderuk <s.shinderuk@postgrespro.ru> writes:
> While analyzing -Wclobbered warnings from gcc we found a true one in
> PostgresMain():
> ...
> These variables must be declared volatile, because they are read after
> longjmp(). send_ready_for_query declared there is volatile.
Yeah, you're on to something there.
> Without volatile, these variables are kept in registers and restored by
> longjump(). I think, this is harmless because the error handling code
> calls disable_all_timeouts() anyway.
Hmm. So what could happen (if these *aren't* in registers) is that we
might later uselessly call disable_timeout to get rid of timeouts that
are long gone anyway. While that's not terribly expensive, it's not
great either. What we ought to be doing is resetting these two flags
after the disable_all_timeouts call.
Having done that, it wouldn't really be necessary to mark these
as volatile. I kept that marking anyway for consistency with
send_ready_for_query, but perhaps we shouldn't?
> I also moved firstchar's declaration inside the loop where it's used, to
> make it clear that this variable needn't be volatile and is not
> preserved after longjmp().
Good idea, but then why not the same for input_message? It's fully
reinitialized each time through the loop, too.
In short, something like the attached, except I'm not totally sold
on changing the volatility of the timeout flags.
regards, tom lane
Attachments:
v2-fix-PostgresMain-local-vars.patch (text/x-diff, 1.4K) download
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 01b6cc1f7d..d18018671d 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4111,12 +4111,12 @@ PostgresSingleUserMain(int argc, char *argv[],
void
PostgresMain(const char *dbname, const char *username)
{
- int firstchar;
- StringInfoData input_message;
sigjmp_buf local_sigjmp_buf;
+
+ /* these must be volatile to ensure state is preserved across longjmp: */
volatile bool send_ready_for_query = true;
- bool idle_in_transaction_timeout_enabled = false;
- bool idle_session_timeout_enabled = false;
+ volatile bool idle_in_transaction_timeout_enabled = false;
+ volatile bool idle_session_timeout_enabled = false;
Assert(dbname != NULL);
Assert(username != NULL);
@@ -4324,6 +4324,8 @@ PostgresMain(const char *dbname, const char *username)
*/
disable_all_timeouts(false);
QueryCancelPending = false; /* second to avoid race condition */
+ idle_in_transaction_timeout_enabled = false;
+ idle_session_timeout_enabled = false;
/* Not reading from the client anymore. */
DoingCommandRead = false;
@@ -4418,6 +4420,9 @@ PostgresMain(const char *dbname, const char *username)
for (;;)
{
+ int firstchar;
+ StringInfoData input_message;
+
/*
* At top of loop, reset extended-query-message flag, so that any
* errors encountered in "idle" state don't provoke skip.
* Re: gcc -Wclobbered in PostgresMain
@ 2023-07-10 09:39 Sergey Shinderuk <s.shinderuk@postgrespro.ru>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 4+ messages in thread
From: Sergey Shinderuk @ 2023-07-10 09:39 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
Hello, Tom,
On 08.07.2023 18:11, Tom Lane wrote:
> What we ought to be doing is resetting these two flags
> after the disable_all_timeouts call.
Oops, I missed that.
> Having done that, it wouldn't really be necessary to mark these
> as volatile. I kept that marking anyway for consistency with
> send_ready_for_query, but perhaps we shouldn't?
I don't know. Maybe marking them volatile is more future proof. Not sure.
>> I also moved firstchar's declaration inside the loop where it's used, to
>> make it clear that this variable needn't be volatile and is not
>> preserved after longjmp().
>
> Good idea, but then why not the same for input_message? It's fully
> reinitialized each time through the loop, too.
Yeah, that's better.
> In short, something like the attached, except I'm not totally sold
> on changing the volatility of the timeout flags.
Looks good to me.
Thank you.
--
Sergey Shinderuk https://postgrespro.com/
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: gcc -Wclobbered in PostgresMain
@ 2023-07-10 16:17 Tom Lane <tgl@sss.pgh.pa.us>
parent: Sergey Shinderuk <s.shinderuk@postgrespro.ru>
0 siblings, 0 replies; 4+ messages in thread
From: Tom Lane @ 2023-07-10 16:17 UTC (permalink / raw)
To: Sergey Shinderuk <s.shinderuk@postgrespro.ru>; +Cc: pgsql-hackers
Sergey Shinderuk <s.shinderuk@postgrespro.ru> writes:
> On 08.07.2023 18:11, Tom Lane wrote:
>> Having done that, it wouldn't really be necessary to mark these
>> as volatile. I kept that marking anyway for consistency with
>> send_ready_for_query, but perhaps we shouldn't?
> I don't know. Maybe marking them volatile is more future proof. Not sure.
Yeah, after sleeping on it, it seems best to have a policy that all
variables declared in that place are volatile. Even if there's no bug
now, not having volatile creates a risk of surprising behavior after
future changes. Pushed that way.
regards, tom lane
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-07-10 16:17 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-07-07 12:13 gcc -Wclobbered in PostgresMain Sergey Shinderuk <s.shinderuk@postgrespro.ru>
2023-07-08 15:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-07-10 09:39 ` Sergey Shinderuk <s.shinderuk@postgrespro.ru>
2023-07-10 16:17 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox