Possible uninitialized use of the variables (src/backend/access/transam/twophase.c)

Started by Ranier Vilelaalmost 4 years ago6 messages

ranier.vf@gmail.com

almost 4 years ago

1 attachment(s)

Hi,

Commit
https://github.com/postgres/postgres/commit/1eb6d6527aae264b3e0b9c95aa70bb7a594ad1cf,
modified
data struct TwoPhaseFileHeader and added two new fields:

XLogRecPtr origin_lsn; /* lsn of this record at origin node */
TimestampTz origin_timestamp; /* time of prepare at origin node */

I think thay forgot initialize these fields in the function StartPrepare
because,
when calling function save_state_data(&hdr, sizeof(TwoPhaseFileHeader));

I have one report about possible uninitialized usage of the variables.

Best regards,

Ranier Vilela

Attachments:

v1_possible_unitialized_variables_twophase.patchapplication/octet-stream; name=v1_possible_unitialized_variables_twophase.patchDownload

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 271a3146db..88c63ba0f0 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1073,6 +1073,8 @@ StartPrepare(GlobalTransaction gxact)
 	hdr.ninvalmsgs = xactGetCommittedInvalidationMessages(&invalmsgs,
 														  &hdr.initfileinval);
 	hdr.gidlen = strlen(gxact->gid) + 1;	/* Include '\0' */
+	hdr.origin_lsn = InvalidXLogRecPtr;
+	hdr.origin_timestamp = 0;
 
 	save_state_data(&hdr, sizeof(TwoPhaseFileHeader));
 	save_state_data(gxact->gid, hdr.gidlen);

Kyotaro Horiguchi

horikyota.ntt@gmail.com

almost 4 years ago

In reply to: Ranier Vilela (#1)

Re: Possible uninitialized use of the variables (src/backend/access/transam/twophase.c)

At Wed, 9 Feb 2022 08:15:45 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in

Hi,

Commit
https://github.com/postgres/postgres/commit/1eb6d6527aae264b3e0b9c95aa70bb7a594ad1cf,
modified
data struct TwoPhaseFileHeader and added two new fields:

XLogRecPtr origin_lsn; /* lsn of this record at origin node */
TimestampTz origin_timestamp; /* time of prepare at origin node */

I think thay forgot initialize these fields in the function StartPrepare
because,
when calling function save_state_data(&hdr, sizeof(TwoPhaseFileHeader));

I have one report about possible uninitialized usage of the variables.

Who repoerted that to you?

StartPrepare and EndPrepare are virtually a single function that
accepts some additional operations in the middle. StartPrepare leaves
the "records" incomplete then EndPrepare completes it. It is not
designed so that the fields are accessed from others before the
completion. There seems to be no critical reasons for EndPrepare to
do the pointed operations, but looking that another replorigin-related
operation is needed in the same function, it is sensible that the
author intended to consolidate all replorigin related changes in
EndPrepare.

So, my humble opinion here is, that change is not needed.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Michael Paquier

michael@paquier.xyz

almost 4 years ago

In reply to: Kyotaro Horiguchi (#2)

1 attachment(s)

Re: Possible uninitialized use of the variables (src/backend/access/transam/twophase.c)

On Thu, Feb 10, 2022 at 10:18:15AM +0900, Kyotaro Horiguchi wrote:

Who repoerted that to you?

Let's bet on Coverity.

StartPrepare and EndPrepare are virtually a single function that
accepts some additional operations in the middle. StartPrepare leaves
the "records" incomplete then EndPrepare completes it. It is not
designed so that the fields are accessed from others before the
completion. There seems to be no critical reasons for EndPrepare to
do the pointed operations, but looking that another replorigin-related
operation is needed in the same function, it is sensible that the
author intended to consolidate all replorigin related changes in
EndPrepare.

Well, that's the intention I recall from reading this code a couple of
weeks ago. In the worst case, this could be considered as a bad
practice as having clean data helps at least debugging if you look at
this data between the calls of StartPrepare() and EndPrepare(), and we
do things this way for all the other fields, like total_len.

The proposed change is incomplete anyway once you consider this
argument. First, there is no need to set up those fields in
EndPrepare() anymore if there is no origin data, no? It would be good
to comment that these are filled in EndPrepare(), I guess, once you do
the initialization in StartPrepare(). I agree that those changes are
purely cosmetic.
--
Michael

Attachments:

2pc-hdr-init.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 608c5149e5..874c8ed125 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1074,6 +1074,9 @@ StartPrepare(GlobalTransaction gxact)
 	hdr.ninvalmsgs = xactGetCommittedInvalidationMessages(&invalmsgs,
 														  &hdr.initfileinval);
 	hdr.gidlen = strlen(gxact->gid) + 1;	/* Include '\0' */
+	/* EndPrepare will fill the origin data, if necessary */
+	hdr.origin_lsn = InvalidXLogRecPtr;
+	hdr.origin_timestamp = 0;
 
 	save_state_data(&hdr, sizeof(TwoPhaseFileHeader));
 	save_state_data(gxact->gid, hdr.gidlen);
@@ -1133,11 +1136,6 @@ EndPrepare(GlobalTransaction gxact)
 		hdr->origin_lsn = replorigin_session_origin_lsn;
 		hdr->origin_timestamp = replorigin_session_origin_timestamp;
 	}
-	else
-	{
-		hdr->origin_lsn = InvalidXLogRecPtr;
-		hdr->origin_timestamp = 0;
-	}
 
 	/*
 	 * If the data size exceeds MaxAllocSize, we won't be able to read it in

Michael Paquier

michael@paquier.xyz

almost 4 years ago

In reply to: Michael Paquier (#3)

Re: Possible uninitialized use of the variables (src/backend/access/transam/twophase.c)

On Thu, Feb 10, 2022 at 11:38:44AM +0900, Michael Paquier wrote:

The proposed change is incomplete anyway once you consider this
argument. First, there is no need to set up those fields in
EndPrepare() anymore if there is no origin data, no? It would be good
to comment that these are filled in EndPrepare(), I guess, once you do
the initialization in StartPrepare().

That still looked better on a fresh look in terms of consistency, so
applied this way.
--
Michael

Kyotaro Horiguchi

horikyota.ntt@gmail.com

almost 4 years ago

In reply to: Michael Paquier (#4)

Re: Possible uninitialized use of the variables (src/backend/access/transam/twophase.c)

At Mon, 14 Feb 2022 11:07:19 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Thu, Feb 10, 2022 at 11:38:44AM +0900, Michael Paquier wrote:

The proposed change is incomplete anyway once you consider this
argument. First, there is no need to set up those fields in
EndPrepare() anymore if there is no origin data, no? It would be good
to comment that these are filled in EndPrepare(), I guess, once you do
the initialization in StartPrepare().

That still looked better on a fresh look in terms of consistency, so
applied this way.

FWIW +1. That is what I had in my mind at the time of posting my
comment, but I choosed not to make a change. It is one way to keep
consistency. (And works to silence Coverity.)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Ranier Vilela

ranier.vf@gmail.com

almost 4 years ago

In reply to: Michael Paquier (#4)

Re: Possible uninitialized use of the variables (src/backend/access/transam/twophase.c)

Em dom., 13 de fev. de 2022 às 23:07, Michael Paquier <michael@paquier.xyz>
escreveu:

On Thu, Feb 10, 2022 at 11:38:44AM +0900, Michael Paquier wrote:

The proposed change is incomplete anyway once you consider this
argument. First, there is no need to set up those fields in
EndPrepare() anymore if there is no origin data, no? It would be good
to comment that these are filled in EndPrepare(), I guess, once you do
the initialization in StartPrepare().

That still looked better on a fresh look in terms of consistency, so
applied this way.

Thanks.

regards,
Ranier Vilela