Обсуждение: forgotten initalization of a variable

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
From 2260cf859ffa570639fd0b04cc94540a937e6042 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Tue, 21 Apr 2020 14:15:39 +0900
Subject: [PATCH] Properly initalize a variable.

Commit a7e8ece41c adds new member restoreCommand to
XLogPageReadPrivate, but forgot to initialize it with NULL in
readOneRecord. That leads to illegal pointer access in
SimpleXLogPageRead.
---
 src/bin/pg_rewind/parsexlog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 14a5db5433..542160c493 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -115,6 +115,7 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex)
     XLogRecPtr    endptr;
 
     private.tliIndex = tliIndex;
+    private.restoreCommand = NULL;
     xlogreader = XLogReaderAllocate(WalSegSz, datadir, &SimpleXLogPageRead,
                                     &private);
     if (xlogreader == NULL)
-- 
2.18.2

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

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

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

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