On Tue, Aug 9, 2022 at 10:59 PM <r.zhar...@postgrespro.ru> wrote:
> On 2022-08-09 05:44, Thomas Munro wrote:
> > On Tue, Aug 9, 2022 at 8:30 AM Thomas Munro <thomas.mu...@gmail.com>
> > wrote:
> >> On Mon, Aug 8, 2022 at 8:23 PM <r.zhar...@postgrespro.ru> wrote:
> >> > "C:/HOME" is the junction point to the second volume on my hard drive -
> >> > "\??\Volume{GUID}\" which name pgreadlink() erroneously strips here:
> >> > https://github.com/postgres/postgres/blob/7e29a79a46d30dc236d097825ab849158929d977/src/port/dirmod.c#L357.
> >
> >> ... Would it
> >> be better to say: if it doesn't begin with "\??\X:", where X could be
> >> any letter, then don't modify it?
> >
> > Concretely, I wonder if this is a good fix at least in the short term.
> > Does this work for you, and do the logic and explanation make sense?
>
> Yes, this patch works well with my junction points.
Thanks for testing! I did a bit more reading on this stuff, so that I
could update the comments with the correct terminology from Windows
APIs. I also realised that the pattern we could accept to symlink()
and expect to work is not just "C:..." (could be
RtlPathTypeDriveRelative, which wouldn't actually work in a junction
point) but "C:\..." (RtlPathTypeDriveAbsolute). I tweaked it a bit to
test for that.
> I checked a few variants:
>
> 21.07.2022 15:11 <JUNCTION> HOME [\??\Volume{GUID}\]
> 09.08.2022 15:06 <JUNCTION> Test1 [\\?\Volume{GUID}\]
> 09.08.2022 15:06 <JUNCTION> Test2 [\\.\Volume{GUID}\]
> 09.08.2022 15:17 <JUNCTION> Test3 [\??\Volume{GUID}\]
> 09.08.2022 15:27 <JUNCTION> Test4 [C:\temp\1]
> 09.08.2022 15:28 <JUNCTION> Test5 [C:\HOME\Temp\1]
One more thing I wondered about, now that we're following junctions
outside PGDATA: can a junction point to another junction? If so, I
didn't allow for that: stat() gives up after one hop, because I
figured that was enough for the stuff we expect inside PGDATA and I
couldn't find any evidence in the man pages that referred to chains.
But if you *are* allowed to create a junction "c:\huey" that points to
junction "c:\dewey" that points to "c:\louie", and then you do initdb
-D c:\huey\pgdata, then I guess it would fail. Would you mind
checking if that is a real possibility, and if so, testing this
chain-following patch to see if it fixes it?
> After hours of reading the documentation and debugging, it seems to me
> we can use REPARSE_GUID_DATA_BUFFER structure instead of our
> REPARSE_JUNCTION_DATA_BUFFER [1]. DataBuffer doesn't contain any
> prefixes,
> so we don't need to strip them. But we still need to construct a correct
> volume name if a junction point is a volume mount point. Is it worth to
> check this idea?
I don't know. I think I prefer our current approach, because it can
handle anything (raw/full NT paths) and doesn't try to be very clever,
and I don't want to change to a different scheme for no real
benefit...
From 6ed3edc77275aa51d1d0b1012c0a36db65735d39 Mon Sep 17 00:00:00 2001
From: Thomas Munro <tmu...@postgresql.org>
Date: Thu, 11 Aug 2022 10:42:13 +1200
Subject: [PATCH v2 1/2] Fix readlink() for general Windows 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.zhar...@postgrespro.ru>
Discussion: https://postgr.es/m/4590c37927d7b8ee84f9855d83229018%40postgrespro.ru
---
src/port/dirmod.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/port/dirmod.c b/src/port/dirmod.c
index 2818bfd2e9..53310baafb 100644
--- a/src/port/dirmod.c
+++ b/src/port/dirmod.c
@@ -199,7 +199,11 @@ pgsymlink(const char *oldpath, const char *newpath)
if (dirhandle == INVALID_HANDLE_VALUE)
return -1;
- /* make sure we have an unparsed native win32 path */
+ /*
+ * We expect either an NT path or a "drive absolute" path like "C:\..."
+ * that we convert to an NT path by bolting on a prefix and converting any
+ * Unix-style path delimiters to NT-style.
+ */
if (memcmp("\\??\\", oldpath, 4) != 0)
snprintf(nativeTarget, sizeof(nativeTarget), "\\??\\%s", oldpath);
else
@@ -351,10 +355,21 @@ pgreadlink(const char *path, char *buf, size_t size)
}
/*
- * 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;
--
2.35.1
From 81f203af67bb0dfed1a55656496fd55211d98972 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Thu, 11 Aug 2022 12:30:48 +1200
Subject: [PATCH v2 2/2] Follow junction point chains in our stat() emulation.
Commit c5cb8f3b supposed that we'd only ever have to follow junction
points for one hop, because we don't construct longer chains of them
ourselves. But when stat()ing a parent directory supplied by the user,
we should really be able to cope with longer chains. Choose an
arbitrary cap of 8, to match the minimum acceptable value of SYMLOOP_MAX
from POSIX.
---
src/port/win32stat.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/port/win32stat.c b/src/port/win32stat.c
index 26443293d7..2ff42b9a90 100644
--- a/src/port/win32stat.c
+++ b/src/port/win32stat.c
@@ -184,16 +184,27 @@ _pglstat64(const char *name, struct stat *buf)
int
_pgstat64(const char *name, struct stat *buf)
{
+ int loops = 0;
int ret;
ret = _pglstat64(name, buf);
/* Do we need to follow a symlink (junction point)? */
- if (ret == 0 && S_ISLNK(buf->st_mode))
+ while (ret == 0 && S_ISLNK(buf->st_mode))
{
char next[MAXPGPATH];
ssize_t size;
+ if (++loops > 8)
+ {
+ /*
+ * Give up. The error for too many symlinks is supposed to be
+ * ELOOP, but Windows hasn't got it.
+ */
+ errno = EIO;
+ return -1;
+ }
+
/*
* _pglstat64() already called readlink() once to be able to fill in
* st_size, and now we need to do it again to get the path to follow.
@@ -219,17 +230,6 @@ _pgstat64(const char *name, struct stat *buf)
next[size] = 0;
ret = _pglstat64(next, buf);
- if (ret == 0 && S_ISLNK(buf->st_mode))
- {
- /*
- * We're only prepared to go one hop, because we only expect to
- * deal with the simple cases that we create. The error for too
- * many symlinks is supposed to be ELOOP, but Windows hasn't got
- * it.
- */
- errno = EIO;
- return -1;
- }
}
return ret;
--
2.35.1
