[PATCH v1] strengthen backup history filename check

Started by Junwang Zhaoover 3 years ago3 messages

Junwang Zhao

zhjwpku@gmail.com

over 3 years ago

1 attachment(s)

This patch makes the backup history filename check more tight.

--
Regards
Junwang Zhao

Attachments:

0001-strengthen-backup-history-filename-check.patchapplication/octet-stream; name=0001-strengthen-backup-history-filename-check.patchDownload

From 9d9ba49b4b6f83d39eda0724ca1349059aff8a98 Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Mon, 25 Jul 2022 19:27:34 +0800
Subject: [PATCH v1] strengthen backup history filename check

Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
---
 src/include/access/xlog_internal.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 44291b337b..be49aa87ea 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -252,9 +252,10 @@ BackupHistoryFileName(char *fname, TimeLineID tli, XLogSegNo logSegNo, XLogRecPt
 static inline bool
 IsBackupHistoryFileName(const char *fname)
 {
-	return (strlen(fname) > XLOG_FNAME_LEN &&
+	return (strlen(fname) == XLOG_FNAME_LEN + 9 + strlen(".backup") &&
 			strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN &&
-			strcmp(fname + strlen(fname) - strlen(".backup"), ".backup") == 0);
+			strspn(fname + XLOG_FNAME_LEN + 1, "0123456789ABCDEF") == 8 &&
+			strcmp(fname + XLOG_FNAME_LEN + 9, ".backup") == 0);
 }
 
 static inline void
-- 
2.33.0

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

over 3 years ago

In reply to: Junwang Zhao (#1)

Re: [PATCH v1] strengthen backup history filename check

On Mon, Jul 25, 2022 at 5:01 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

This patch makes the backup history filename check more tight.

Can you please elaborate a bit on the issue with existing
IsBackupHistoryFileName(), if there's any?

Also, the patch does have hard coded numbers [1]static inline bool IsBackupHistoryFileName(const char *fname) { - return (strlen(fname) > XLOG_FNAME_LEN && + return (strlen(fname) == XLOG_FNAME_LEN + 9 + strlen(".backup") && strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && - strcmp(fname + strlen(fname) - strlen(".backup"), ".backup") == 0); + strspn(fname + XLOG_FNAME_LEN + 1, "0123456789ABCDEF") == 8 && + strcmp(fname + XLOG_FNAME_LEN + 9, ".backup") == 0); } which isn't good from
a readability perspective, adding macros and/or comments would help
here.

[1]
 static inline bool
 IsBackupHistoryFileName(const char *fname)
 {
- return (strlen(fname) > XLOG_FNAME_LEN &&
+ return (strlen(fname) == XLOG_FNAME_LEN + 9 + strlen(".backup") &&
  strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN &&
- strcmp(fname + strlen(fname) - strlen(".backup"), ".backup") == 0);
+ strspn(fname + XLOG_FNAME_LEN + 1, "0123456789ABCDEF") == 8 &&
+ strcmp(fname + XLOG_FNAME_LEN + 9, ".backup") == 0);
 }

Regards,
Bharath Rupireddy.

Junwang Zhao

zhjwpku@gmail.com

over 3 years ago

In reply to: Bharath Rupireddy (#2)

Re: [PATCH v1] strengthen backup history filename check

On Mon, Jul 25, 2022 at 7:39 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, Jul 25, 2022 at 5:01 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

This patch makes the backup history filename check more tight.

Can you please elaborate a bit on the issue with existing
IsBackupHistoryFileName(), if there's any?

There are two call of this function, `CleanupBackupHistory` and
`SetWALFileNameForCleanup`, there
seems no issue of the existing IsBackupHistoryFileName() since the
creation of the backup history file
will make sure it is of format "%08X%08X%08X.%08X.backup".

The patch just makes `IsBackupHistoryFileName()` more match to
`BackupHistoryFileName()`, thus
more easier to understand.

Also, the patch does have hard coded numbers [1] which isn't good from
a readability perspective, adding macros and/or comments would help
here.

I'm not sure using macros is a good idea here, cause I noticed
`IsTLHistoryFileName()` also uses
some hard code numbers [1]static inline bool IsTLHistoryFileName(const char *fname) { return (strlen(fname) == 8 + strlen(".history") && strspn(fname, "0123456789ABCDEF") == 8 && strcmp(fname + 8, ".history") == 0); }.

[1]: static inline bool IsTLHistoryFileName(const char *fname) { return (strlen(fname) == 8 + strlen(".history") && strspn(fname, "0123456789ABCDEF") == 8 && strcmp(fname + 8, ".history") == 0); }
static inline bool
IsTLHistoryFileName(const char *fname)
{
return (strlen(fname) == 8 + strlen(".history") &&
strspn(fname, "0123456789ABCDEF") == 8 &&
strcmp(fname + 8, ".history") == 0);
}

[1]
static inline bool
IsBackupHistoryFileName(const char *fname)
{
- return (strlen(fname) > XLOG_FNAME_LEN &&
+ return (strlen(fname) == XLOG_FNAME_LEN + 9 + strlen(".backup") &&
strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN &&
- strcmp(fname + strlen(fname) - strlen(".backup"), ".backup") == 0);
+ strspn(fname + XLOG_FNAME_LEN + 1, "0123456789ABCDEF") == 8 &&
+ strcmp(fname + XLOG_FNAME_LEN + 9, ".backup") == 0);
}

Regards,
Bharath Rupireddy.

--
Regards
Junwang Zhao