Re: forgotten initalization of a variable
At Wed, 22 Apr 2020 08:13:02 +0900, Michael Paquier 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 > > 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
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 > 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 signature.asc Description: PGP signature
Re: forgotten initalization of a variable
At Tue, 21 Apr 2020 17:34:26 +0900, Michael Paquier 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
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 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, , ); 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); signature.asc Description: PGP signature
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 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, , ); if (xlogreader == NULL) -- 2.18.2