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
Attachments:
0001-Properly-initalize-a-variable.patchtext/x-patch; charset=us-asciiDownload
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
Attachments:
pgrewind-restore-fix-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 14a5db5433..c51b5db315 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -106,7 +106,8 @@ extractPageMap(const char *datadir, XLogRecPtr startpoint, int tliIndex,
* doing anything with the record itself.
*/
XLogRecPtr
-readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex)
+readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex,
+ const char *restoreCommand)
{
XLogRecord *record;
XLogReaderState *xlogreader;
@@ -115,6 +116,7 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex)
XLogRecPtr endptr;
private.tliIndex = tliIndex;
+ private.restoreCommand = restoreCommand;
xlogreader = XLogReaderAllocate(WalSegSz, datadir, &SimpleXLogPageRead,
&private);
if (xlogreader == NULL)
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 101f0911be..633955f7be 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -339,7 +339,8 @@ main(int argc, char **argv)
/* Read the checkpoint record on the target to see where it ends. */
chkptendrec = readOneRecord(datadir_target,
ControlFile_target.checkPoint,
- targetNentries - 1);
+ targetNentries - 1,
+ restore_command);
/*
* If the histories diverged exactly at the end of the shutdown
diff --git a/src/bin/pg_rewind/pg_rewind.h b/src/bin/pg_rewind/pg_rewind.h
index b122ae43e5..5cf5f17bb5 100644
--- a/src/bin/pg_rewind/pg_rewind.h
+++ b/src/bin/pg_rewind/pg_rewind.h
@@ -50,7 +50,7 @@ extern void findLastCheckpoint(const char *datadir, XLogRecPtr searchptr,
XLogRecPtr *lastchkptredo,
const char *restoreCommand);
extern XLogRecPtr readOneRecord(const char *datadir, XLogRecPtr ptr,
- int tliIndex);
+ int tliIndex, const char *restoreCommand);
/* in pg_rewind.c */
extern void progress_report(bool force);
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