From cfac4b6329e5854010160896ba8e7a1bc34f3b31 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Mon, 5 Jun 2023 15:42:00 -0400
Subject: [PATCH v1 3/8] Change struct tablespaceinfo's oid member from 'char
 *' to 'Oid'

This shouldn't change behavior except in the unusual case where
there are file in the tablespace directory that have entirely
numeric names but are nevertheless not possible names for a
tablespace directory, either because their names has leading zeroes
that shouldn't be there, or the value is actually zero, or because
the value is too large to represent as an OID.

In those cases, the directory would previously have made it into
the list of tablespaceinfo objects and no longer will. Thus, base
backups will now ignore such directories, instead of treating them
as legitimate tablespace directories. Similarly, if entries for
such tablespaces occur in a tablespace_map file, they will now
be rejected as erroneous, instead of being honored.

This is infrastructure for future work that wants to be able to
know the tablespace of each relation that is part of a backup
*as an OID*. By strengthening the up-front validation, we don't
have to worry about weird cases later, and can more easily avoid
repeated string->integer conversions.
---
 src/backend/access/transam/xlog.c         | 19 ++++++++++--
 src/backend/access/transam/xlogrecovery.c | 12 ++++++--
 src/backend/backup/backup_manifest.c      |  6 ++--
 src/backend/backup/basebackup.c           | 35 ++++++++++++-----------
 src/backend/backup/basebackup_copy.c      |  2 +-
 src/include/backup/backup_manifest.h      |  2 +-
 src/include/backup/basebackup.h           |  2 +-
 7 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b2430f617c..664d4ba598 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8456,9 +8456,22 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
 			char	   *relpath = NULL;
 			char	   *s;
 			PGFileType	de_type;
+			char	   *badp;
+			Oid			tsoid;
 
-			/* Skip anything that doesn't look like a tablespace */
-			if (strspn(de->d_name, "0123456789") != strlen(de->d_name))
+			/*
+			 * Try to parse the directory name as an unsigned integer.
+			 *
+			 * Tablespace directories should be positive integers that can
+			 * be represented in 32 bits, with no leading zeroes or trailing
+			 * garbage. If we come across a name that doesn't meet those
+			 * criteria, skip it.
+			 */
+			if (de->d_name[0] < '1' || de->d_name[1] > '9')
+				continue;
+			errno = 0;
+			tsoid = strtoul(de->d_name, &badp, 10);
+			if (*badp != '\0' || errno == EINVAL || errno == ERANGE)
 				continue;
 
 			snprintf(fullpath, sizeof(fullpath), "pg_tblspc/%s", de->d_name);
@@ -8533,7 +8546,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
 			}
 
 			ti = palloc(sizeof(tablespaceinfo));
-			ti->oid = pstrdup(de->d_name);
+			ti->oid = tsoid;
 			ti->path = pstrdup(linkpath);
 			ti->rpath = relpath;
 			ti->size = -1;
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index becc2bda62..4ff4430006 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -678,7 +678,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
 				tablespaceinfo *ti = lfirst(lc);
 				char	   *linkloc;
 
-				linkloc = psprintf("pg_tblspc/%s", ti->oid);
+				linkloc = psprintf("pg_tblspc/%u", ti->oid);
 
 				/*
 				 * Remove the existing symlink if any and Create the symlink
@@ -692,7 +692,6 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
 							 errmsg("could not create symbolic link \"%s\": %m",
 									linkloc)));
 
-				pfree(ti->oid);
 				pfree(ti->path);
 				pfree(ti);
 			}
@@ -1341,6 +1340,8 @@ read_tablespace_map(List **tablespaces)
 	{
 		if (!was_backslash && (ch == '\n' || ch == '\r'))
 		{
+			char   *endp;
+
 			if (i == 0)
 				continue;		/* \r immediately followed by \n */
 
@@ -1360,7 +1361,12 @@ read_tablespace_map(List **tablespaces)
 			str[n++] = '\0';
 
 			ti = palloc0(sizeof(tablespaceinfo));
-			ti->oid = pstrdup(str);
+			errno = 0;
+			ti->oid = strtoul(str, &endp, 10);
+			if (*endp != '\0' ||  errno == EINVAL || errno == ERANGE)
+				ereport(FATAL,
+						(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+						 errmsg("invalid data in file \"%s\"", TABLESPACE_MAP)));
 			ti->path = pstrdup(str + n);
 			*tablespaces = lappend(*tablespaces, ti);
 
diff --git a/src/backend/backup/backup_manifest.c b/src/backend/backup/backup_manifest.c
index cee6216524..aeed362a9a 100644
--- a/src/backend/backup/backup_manifest.c
+++ b/src/backend/backup/backup_manifest.c
@@ -97,7 +97,7 @@ FreeBackupManifest(backup_manifest_info *manifest)
  * Add an entry to the backup manifest for a file.
  */
 void
-AddFileToBackupManifest(backup_manifest_info *manifest, const char *spcoid,
+AddFileToBackupManifest(backup_manifest_info *manifest, Oid spcoid,
 						const char *pathname, size_t size, pg_time_t mtime,
 						pg_checksum_context *checksum_ctx)
 {
@@ -114,9 +114,9 @@ AddFileToBackupManifest(backup_manifest_info *manifest, const char *spcoid,
 	 * pathname relative to the data directory (ignoring the intermediate
 	 * symlink traversal).
 	 */
-	if (spcoid != NULL)
+	if (OidIsValid(spcoid))
 	{
-		snprintf(pathbuf, sizeof(pathbuf), "pg_tblspc/%s/%s", spcoid,
+		snprintf(pathbuf, sizeof(pathbuf), "pg_tblspc/%u/%s", spcoid,
 				 pathname);
 		pathname = pathbuf;
 	}
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index f46f930329..cc3d2e0c41 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -75,14 +75,15 @@ typedef struct
 	pg_checksum_type manifest_checksum_type;
 } basebackup_options;
 
-static int64 sendTablespace(bbsink *sink, char *path, char *spcoid, bool sizeonly,
+static int64 sendTablespace(bbsink *sink, char *path, Oid spcoid, bool sizeonly,
 							struct backup_manifest_info *manifest);
 static int64 sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
 					 List *tablespaces, bool sendtblspclinks,
-					 backup_manifest_info *manifest, const char *spcoid);
+					 backup_manifest_info *manifest, Oid spcoid);
 static bool sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
-					 struct stat *statbuf, bool missing_ok, Oid dboid,
-					 backup_manifest_info *manifest, const char *spcoid);
+					 struct stat *statbuf, bool missing_ok,
+					 Oid dboid, Oid spcoid,
+					 backup_manifest_info *manifest);
 static off_t read_file_data_into_buffer(bbsink *sink,
 										const char *readfilename, int fd,
 										off_t offset, size_t length,
@@ -305,7 +306,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 
 				if (tmp->path == NULL)
 					tmp->size = sendDir(sink, ".", 1, true, state.tablespaces,
-										true, NULL, NULL);
+										true, NULL, InvalidOid);
 				else
 					tmp->size = sendTablespace(sink, tmp->path, tmp->oid, true,
 											   NULL);
@@ -346,7 +347,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 
 				/* Then the bulk of the files... */
 				sendDir(sink, ".", 1, false, state.tablespaces,
-						sendtblspclinks, &manifest, NULL);
+						sendtblspclinks, &manifest, InvalidOid);
 
 				/* ... and pg_control after everything else. */
 				if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
@@ -355,11 +356,11 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 							 errmsg("could not stat file \"%s\": %m",
 									XLOG_CONTROL_FILE)));
 				sendFile(sink, XLOG_CONTROL_FILE, XLOG_CONTROL_FILE, &statbuf,
-						 false, InvalidOid, &manifest, NULL);
+						 false, InvalidOid, InvalidOid, &manifest);
 			}
 			else
 			{
-				char	   *archive_name = psprintf("%s.tar", ti->oid);
+				char	   *archive_name = psprintf("%u.tar", ti->oid);
 
 				bbsink_begin_archive(sink, archive_name);
 
@@ -623,8 +624,8 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 						(errcode_for_file_access(),
 						 errmsg("could not stat file \"%s\": %m", pathbuf)));
 
-			sendFile(sink, pathbuf, pathbuf, &statbuf, false, InvalidOid,
-					 &manifest, NULL);
+			sendFile(sink, pathbuf, pathbuf, &statbuf, false,
+					 InvalidOid, InvalidOid, &manifest);
 
 			/* unconditionally mark file as archived */
 			StatusFilePath(pathbuf, fname, ".done");
@@ -1087,7 +1088,7 @@ sendFileWithContent(bbsink *sink, const char *filename, const char *content,
 
 	_tarWritePadding(sink, len);
 
-	AddFileToBackupManifest(manifest, NULL, filename, len,
+	AddFileToBackupManifest(manifest, InvalidOid, filename, len,
 							(pg_time_t) statbuf.st_mtime, &checksum_ctx);
 }
 
@@ -1099,7 +1100,7 @@ sendFileWithContent(bbsink *sink, const char *filename, const char *content,
  * Only used to send auxiliary tablespaces, not PGDATA.
  */
 static int64
-sendTablespace(bbsink *sink, char *path, char *spcoid, bool sizeonly,
+sendTablespace(bbsink *sink, char *path, Oid spcoid, bool sizeonly,
 			   backup_manifest_info *manifest)
 {
 	int64		size;
@@ -1154,7 +1155,7 @@ sendTablespace(bbsink *sink, char *path, char *spcoid, bool sizeonly,
 static int64
 sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
 		List *tablespaces, bool sendtblspclinks, backup_manifest_info *manifest,
-		const char *spcoid)
+		Oid spcoid)
 {
 	DIR		   *dir;
 	struct dirent *de;
@@ -1419,8 +1420,8 @@ sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
 
 			if (!sizeonly)
 				sent = sendFile(sink, pathbuf, pathbuf + basepathlen + 1, &statbuf,
-								true, isDbDir ? atooid(lastDir + 1) : InvalidOid,
-								manifest, spcoid);
+								true, isDbDir ? atooid(lastDir + 1) : InvalidOid, spcoid,
+								manifest);
 
 			if (sent || sizeonly)
 			{
@@ -1489,8 +1490,8 @@ is_checksummed_file(const char *fullpath, const char *filename)
  */
 static bool
 sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
-		 struct stat *statbuf, bool missing_ok, Oid dboid,
-		 backup_manifest_info *manifest, const char *spcoid)
+		 struct stat *statbuf, bool missing_ok, Oid dboid, Oid spcoid,
+		 backup_manifest_info *manifest)
 {
 	int			fd;
 	BlockNumber blkno = 0;
diff --git a/src/backend/backup/basebackup_copy.c b/src/backend/backup/basebackup_copy.c
index 1db80cde1b..2b42fd257e 100644
--- a/src/backend/backup/basebackup_copy.c
+++ b/src/backend/backup/basebackup_copy.c
@@ -407,7 +407,7 @@ SendTablespaceList(List *tablespaces)
 		}
 		else
 		{
-			values[0] = ObjectIdGetDatum(strtoul(ti->oid, NULL, 10));
+			values[0] = ObjectIdGetDatum(ti->oid);
 			values[1] = CStringGetTextDatum(ti->path);
 		}
 		if (ti->size >= 0)
diff --git a/src/include/backup/backup_manifest.h b/src/include/backup/backup_manifest.h
index d41b439980..5a481dbcf5 100644
--- a/src/include/backup/backup_manifest.h
+++ b/src/include/backup/backup_manifest.h
@@ -39,7 +39,7 @@ extern void InitializeBackupManifest(backup_manifest_info *manifest,
 									 backup_manifest_option want_manifest,
 									 pg_checksum_type manifest_checksum_type);
 extern void AddFileToBackupManifest(backup_manifest_info *manifest,
-									const char *spcoid,
+									Oid spcoid,
 									const char *pathname, size_t size,
 									pg_time_t mtime,
 									pg_checksum_context *checksum_ctx);
diff --git a/src/include/backup/basebackup.h b/src/include/backup/basebackup.h
index 3e68abc2bb..1432d9c206 100644
--- a/src/include/backup/basebackup.h
+++ b/src/include/backup/basebackup.h
@@ -27,7 +27,7 @@
  */
 typedef struct
 {
-	char	   *oid;			/* tablespace's OID, as a decimal string */
+	Oid			oid;			/* tablespace's OID */
 	char	   *path;			/* full path to tablespace's directory */
 	char	   *rpath;			/* relative path if it's within PGDATA, else
 								 * NULL */
-- 
2.37.1 (Apple Git-137.1)

