0007-Fix-readlink-for-non-PostgreSQL-created-junction-poi.patch

text/x-patch

Filename: 0007-Fix-readlink-for-non-PostgreSQL-created-junction-poi.patch
Type: text/x-patch
Part: 6
Message: Understanding, testing and improving our Windows filesystem code

Patch

Format: format-patch
Series: patch 0007
Subject: Fix readlink() for non-PostgreSQL-created junction points.
File+
src/port/dirmod.c 14 3
src/port/t/001_filesystem.c 51 0
From 664fa8d965151b4cbf1e0408b2944cb8b7d5bc22 Mon Sep 17 00:00:00 2001
From: Thomas Munro <tmunro@postgresql.org>
Date: Thu, 11 Aug 2022 10:42:13 +1200
Subject: [PATCH 07/10] Fix readlink() for non-PostgreSQL-created junction
 points.

Our symlink() and readlink() replacements perform a naive transformation
from DOS paths to NT paths and back, as required by the junction point
APIs.  This was enough for the "drive absolute" paths we expect users to
provide for tablespaces, for example "d:\my\fast\storage".

Since commit c5cb8f3b taught stat() to follow symlinks, and since initdb
uses pg_mkdir_p(), and that examines parent directories, our humble
readlink() implementation can now be exposed to junction points not of
PostgreSQL origin.  Those might be corrupted by our naive path mangling,
which doesn't really understand NT paths in general.

Simply decline to transform paths that don't look like a drive absolute
path.  That means that readlink() returns the NT path directly when
checking a parent directory of PGDATA that happen to point to a drive
using "rooted" format.  That  works for the purposes of our stat()
emulation.

Reported-by: Roman Zharkov <r.zharkov@postgrespro.ru>
Reviewed-by: Roman Zharkov <r.zharkov@postgrespro.ru>
Discussion: https://postgr.es/m/4590c37927d7b8ee84f9855d83229018%40postgrespro.ru
---
 src/port/dirmod.c           | 17 ++++++++++---
 src/port/t/001_filesystem.c | 51 +++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/src/port/dirmod.c b/src/port/dirmod.c
index 3e1c78fae6..f7fd4c15ad 100644
--- a/src/port/dirmod.c
+++ b/src/port/dirmod.c
@@ -366,10 +366,21 @@ pgreadlink(const char *path, char *buf, size_t size)
 	r -= 1;
 
 	/*
-	 * If the path starts with "\??\", which it will do in most (all?) cases,
-	 * strip those out.
+	 * If the path starts with "\??\" followed by a "drive absolute" path
+	 * (known to Windows APIs as RtlPathTypeDriveAbsolute), then strip that
+	 * prefix.  This undoes some of the transformation performed by
+	 * pqsymlink(), to get back to a format that users are used to seeing.  We
+	 * don't know how to transform other path types that might be encountered
+	 * outside PGDATA, so we just return them directly.
 	 */
-	if (r > 4 && strncmp(buf, "\\??\\", 4) == 0)
+	if (r >= 7 &&
+		buf[0] == '\\' &&
+		buf[1] == '?' &&
+		buf[2] == '?' &&
+		buf[3] == '\\' &&
+		isalpha(buf[4]) &&
+		buf[5] == ':' &&
+		buf[6] == '\\')
 	{
 		memmove(buf, buf + 4, strlen(buf + 4) + 1);
 		r -= 4;
diff --git a/src/port/t/001_filesystem.c b/src/port/t/001_filesystem.c
index 28ef069ee0..4178b8e1f2 100644
--- a/src/port/t/001_filesystem.c
+++ b/src/port/t/001_filesystem.c
@@ -216,6 +216,57 @@ filesystem_metadata_tests(void)
 	PG_EXPECT(readlink("does-not-exist", path3, sizeof(path3)) == -1, "readlink fails on missing path");
 	PG_EXPECT_EQ(errno, ENOENT);
 
+	/*
+	 * Checks that we don't corrupt non-drive-absolute paths when peforming
+	 * internal conversions.
+	 */
+
+	/*
+	 * Typical case: Windows drive absolute.  This should also be accepted on
+	 * POSIX systems, because they are required not to validate the target
+	 * string as a path.
+	 */
+	make_path(path2, "my_symlink");
+	PG_EXPECT_SYS(symlink("c:\\foo", path2) == 0);
+	size = readlink(path2, path3, sizeof(path3));
+	PG_EXPECT_SYS(size != -1, "readlink succeeds");
+	PG_EXPECT_EQ(size, 6);
+	path3[size] = '\0';
+	PG_EXPECT_EQ_STR(path3, "c:\\foo");
+	PG_EXPECT_SYS(unlink(path2) == 0);
+
+	/*
+	 * Drive absolute given in full NT format will be stripped on round-trip
+	 * through our Windows emulations.
+	 */
+	make_path(path2, "my_symlink");
+	PG_EXPECT_SYS(symlink("\\??\\c:\\foo", path2) == 0);
+	size = readlink(path2, path3, sizeof(path3));
+	PG_EXPECT_SYS(size != -1, "readlink succeeds");
+	path3[size] = '\0';
+#ifdef WIN32
+	PG_EXPECT_EQ(size, 6);
+	PG_EXPECT_EQ_STR(path3, "c:\\foo");
+#else
+	PG_EXPECT_EQ(size, 10);
+	PG_EXPECT_EQ_STR(path3, "\\??\\c:\\foo");
+#endif
+	PG_EXPECT_SYS(unlink(path2) == 0);
+
+	/*
+	 * Anything that doesn't look like the NT pattern that symlink() creates
+	 * will be returned verbatim.  This will allow our stat() to handle paths
+	 * that were not created by symlink().
+	 */
+	make_path(path2, "my_symlink");
+	PG_EXPECT_SYS(symlink("\\??\\Volume1234", path2) == 0);
+	size = readlink(path2, path3, sizeof(path3));
+	PG_EXPECT_SYS(size != -1, "readlink succeeds");
+	PG_EXPECT_EQ(size, 14);
+	path3[size] = '\0';
+	PG_EXPECT_EQ_STR(path3, "\\??\\Volume1234");
+	PG_EXPECT_SYS(unlink(path2) == 0);
+
 	/* Tests for fstat(). */
 
 	make_path(path, "dir1/test.txt");
-- 
2.37.3