Fix tar xf with symlinks

[dscho]

When tar xf encounters a symbolic link, it seems to call fstatat() with a file descriptor referring to said symbolic link and a pathname that is empty. Apparently the intended behavior is to then operate on the symbolic link itself (i.e. not on its target, unlike fstat() would). This behavior was broken in Cygwin v3.4.7, and here is a fix for that bug.

[dscho]

I also sent this patch to the Cygwin project, let's see what they say: https://inbox.sourceware.org/cygwin-patches/c985ab15b28da4fe6f28da4e20236bc0feb484bd.1687898935.git.johannes.schindelin@gmx.de/T/#u.

[vdye]

Per the fstatat() man page, the error returned by the function if pathname is empty should be ENOENT:

ENOENT A component of path does not name an existing file or path is an empty string.

fchownat() is similarly documented:

ENOENT A component of path does not name an existing file or path is an empty string.

So, I think it's incorrect that gen_full_path_at() is returning ENOTDIR for an empty pathname, and the fix would be to instead check for an empty pathname right at the start of gen_full_path_at(), then return ENOENT accordingly.

[dscho]

Right, but there's also this:

AT_EMPTY_PATH (since Linux 2.6.39):
If pathname is an empty string, operate on the file referred to by dirfd (which may have been obtained using the open(2) O_PATH flag). In this case, dirfd can refer to any type of file, not just a directory, and the behavior of fstatat() is similar to that of fstat(). If dirfd is AT_FDCWD, the call operates on the current working directory. This flag is Linux-specific; define _GNU_SOURCE to obtain its definition.

And indeed, the following program will not fail when run in a git/git checkout but print "Got mode 0120777" instead:

#define _GNU_SOURCE 1
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>
#include <stdio.h>

int main(int argc, char **argv)
{
	int fd = open("RelNotes", O_PATH | O_NOFOLLOW);
	struct stat st;

	if (fd < 0)
		fprintf(stderr, "Could not open `RelNotes`\n");
	else if (fstatat(fd, "", &st, AT_EMPTY_PATH) < 0)
		fprintf(stderr, "fstatat() failed with %d (%s)\n", (int)errno, strerror(errno));
	else
		printf("Got mode 0%o\n", (int)st.st_mode);
	
	return 0;
}

[vdye]

Right - in that case, your fix behaves correctly. My point is that, if AT_EMPTY_PATH is not set, fstatat() is currently behaving incorrectly (since it returns ENOTDIR, rather than ENOENT, for an empty path).

[dscho]

if AT_EMPTY_PATH is not set, fstatat() is currently behaving incorrectly (since it returns ENOTDIR, rather than ENOENT, for an empty path

I think that is intended, as the dirfd should be used as directory unless AT_EMPTY_PATH is specified, if I read the spec correctly.

[vdye]

AFAICT, this issue is that there's no commentary in the documentation indicating what happens when both "path is empty" (where you'd return ENOENT) and "dirfd doesn't point to a directory" (where you'd return ENOTDIR) are satisfied, assuming no AT_EMPTY_PATH. Either could feasibly be correct, but the upstream change you're responding to (possibly inadvertently) changed the behavior from the former to the latter.

The reason I lean towards "ENOENT when path is empty" superceding "ENOTDIR when dirfd is not a directory" is because returning ENOENT aligns with MSYS2's historical behavior before the recent upstream change and having gen_full_path_at() return ENOENT for an empty path avoids the need to change its callers as you have here.

One way to gain more confidence in one way or the other might be to reference other implementations of fstatat(). I'm not sure where to look, though; I think I found an example in the Linux kernel that checks filename before it does anything with dfd (returning ENOENT if filename is empty & AT_EMPTY_PATH is unset), but I also don't know enough about the Linux kernel to say whether that's a relevant comparison.

[dscho]

I hear you, and agree with you that it would probably make more sense to return ENOENT in that case. I'm just out of energy to fight that particular battle on the Cygwin-patches mailing list...

[dscho]

Okay, I ended up asking with curiosity: https://inbox.sourceware.org/cygwin-patches/d983003d-b8e6-e312-2197-499cc7f29306@gmx.de/

[dscho]

I'll go ahead and merge this so that we have a fixed MSYS2 runtime. In the unlikely event that Cygwin decides to go with a different solution, we can always adapt.