forgotten initalization of a variable

Started by Kyotaro Horiguchialmost 6 years ago5 messageshackers
Jump to latest
#1Kyotaro Horiguchi
horikyota.ntt@gmail.com

Hello.

The commit a7e8ece41c adds a new member restoreCommand to
XLogPageReadPrivate. readOneRecord doesn't make use of it but forgets
to set NULL. That can lead to illegal pointer access.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachments:

0001-Properly-initalize-a-variable.patchtext/x-patch; charset=us-asciiDownload+1-1
#2Michael Paquier
michael@paquier.xyz
In reply to: Kyotaro Horiguchi (#1)
Re: forgotten initalization of a variable

On Tue, Apr 21, 2020 at 03:08:30PM +0900, Kyotaro Horiguchi wrote:

The commit a7e8ece41c adds a new member restoreCommand to
XLogPageReadPrivate. readOneRecord doesn't make use of it but forgets
to set NULL. That can lead to illegal pointer access.

That's an oversight of the original commit. Now, instead of failing
even if there is a restore command, wouldn't it be better to pass down
the restore_command to readOneRecord() so as we can optionally
improve the stability of a single record lookup? This only applies to
a checkpoint record now, but this routine could be called elsewhere in
the future. Please see the attached.
--
Michael

Attachments:

pgrewind-restore-fix-v2.patchtext/x-diff; charset=us-asciiDownload+6-3
#3Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Michael Paquier (#2)
Re: forgotten initalization of a variable

At Tue, 21 Apr 2020 17:34:26 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Tue, Apr 21, 2020 at 03:08:30PM +0900, Kyotaro Horiguchi wrote:

The commit a7e8ece41c adds a new member restoreCommand to
XLogPageReadPrivate. readOneRecord doesn't make use of it but forgets
to set NULL. That can lead to illegal pointer access.

That's an oversight of the original commit. Now, instead of failing
even if there is a restore command, wouldn't it be better to pass down
the restore_command to readOneRecord() so as we can optionally
improve the stability of a single record lookup? This only applies to

Oops! You're right.

a checkpoint record now, but this routine could be called elsewhere in
the future. Please see the attached.

It looks fine to me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#4Michael Paquier
michael@paquier.xyz
In reply to: Kyotaro Horiguchi (#3)
Re: forgotten initalization of a variable

On Tue, Apr 21, 2020 at 06:09:30PM +0900, Kyotaro Horiguchi wrote:

At Tue, 21 Apr 2020 17:34:26 +0900, Michael Paquier <michael@paquier.xyz> wrote in

a checkpoint record now, but this routine could be called elsewhere in
the future. Please see the attached.

It looks fine to me.

Fixed this way, then. Thanks for the report!
--
Michael

#5Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Michael Paquier (#4)
Re: forgotten initalization of a variable

At Wed, 22 Apr 2020 08:13:02 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Tue, Apr 21, 2020 at 06:09:30PM +0900, Kyotaro Horiguchi wrote:

At Tue, 21 Apr 2020 17:34:26 +0900, Michael Paquier <michael@paquier.xyz> wrote in

a checkpoint record now, but this routine could be called elsewhere in
the future. Please see the attached.

It looks fine to me.

Fixed this way, then. Thanks for the report!

Thans for fixing this!

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center