From: Stefan Sperling Subject: Re: -portable: add a wrapper for open() on FreeBSD To: Thomas Adam , "Todd C. Miller" , Christian Weisgerber , gameoftrees@openbsd.org Date: Sun, 26 Sep 2021 14:42:05 +0200 On Sun, Sep 26, 2021 at 02:35:07PM +0200, Stefan Sperling wrote: > On Sun, Sep 26, 2021 at 02:17:59PM +0200, Stefan Sperling wrote: > > On Sun, Sep 26, 2021 at 02:05:33PM +0200, Stefan Sperling wrote: > > > On Sun, Sep 26, 2021 at 01:57:49PM +0200, Stefan Sperling wrote: > > > > I suspect it is not intentional. > > > > > > > > It looks like EMLINK from open(symlink, O_NOFOLLOW) comes from this change: > > > > https://reviews.freebsd.org/D29323 > > > > (I don't have a freebsd system to test with, so I might be wrong.) > > > > > > I made a mistake and did no go back far enough in history. > > > > > > Turns out this behaviour was introduced in FreeBSD way back in 1998: > > > https://cgit.freebsd.org/src/commit/?id=100ceca222a193e5d374a3dc9782287785dd9b61 > > > > > > No rationale given for the switch from ELOOP to EMLINK. > > > > It does not look like there is any hope for this to change: > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214633 > > I think we should just give up and admit that this aspect of UNIX > was never unified. It's ugly but reflects reality rather than > hiding the problem under a not-really-portable POSIX rug. > > ok? Oops, I forgot to run regression tests before sending this out. I got one case wrong where the check was for == ELOOP instead of != ELOOP. Fixed here. diff 5267b9e4960076d7de62633b9f5f1dcdb6594b33 /home/stsp/src/got blob - 66b864fee620072787f5c6149732f3c409f4f0cf file + got/got.c --- got/got.c +++ got/got.c @@ -4394,7 +4394,7 @@ print_diff(void *arg, unsigned char status, unsigned c if (dirfd != -1) { fd = openat(dirfd, de_name, O_RDONLY | O_NOFOLLOW); if (fd == -1) { - if (errno != ELOOP) { + if (!got_err_open_nofollow_on_symlink()) { err = got_error_from_errno2("openat", abspath); goto done; @@ -4407,7 +4407,7 @@ print_diff(void *arg, unsigned char status, unsigned c } else { fd = open(abspath, O_RDONLY | O_NOFOLLOW); if (fd == -1) { - if (errno != ELOOP) { + if (!got_err_open_nofollow_on_symlink()) { err = got_error_from_errno2("open", abspath); goto done; blob - f87c06cad9b95c07cc3e6ce8f0e0f733de70a1a6 file + include/got_error.h --- include/got_error.h +++ include/got_error.h @@ -427,3 +427,9 @@ const struct got_error *got_error_path(const char *, i * additional arguments. */ const struct got_error *got_error_fmt(int, const char *, ...); + +/* + * Check whether open(2) with O_NOFOLLOW failed on a symlink. + * This must be called directly after open(2) because it uses errno! + */ +int got_err_open_nofollow_on_symlink(void); blob - 72e00488bedc8ea49b3e20af7eb76ec82839bfa3 file + lib/error.c --- lib/error.c +++ lib/error.c @@ -258,3 +258,15 @@ got_error_fmt(int code, const char *fmt, ...) abort(); } + +int +got_err_open_nofollow_on_symlink(void) +{ + /* + * Check whether open(2) with O_NOFOLLOW failed on a symlink. + * Posix mandates ELOOP and OpenBSD follows it. Others return + * different error codes. We carry this workaround to help the + * portable version a little. + */ + return (errno == ELOOP || errno == EMLINK || errno == EFTYPE); +} blob - d2c6246c748ba52478619a448ec02bf61c9e9729 file + lib/object_create.c --- lib/object_create.c +++ lib/object_create.c @@ -129,7 +129,7 @@ got_object_blob_file_create(struct got_object_id **id, fd = open(ondisk_path, O_RDONLY | O_NOFOLLOW); if (fd == -1) { - if (errno != ELOOP) + if (!got_err_open_nofollow_on_symlink()) return got_error_from_errno2("open", ondisk_path); if (lstat(ondisk_path, &sb) == -1) { blob - 904fb23ceb80ef07dc958d9908801822c83de3f3 file + lib/worktree.c --- lib/worktree.c +++ lib/worktree.c @@ -1284,7 +1284,7 @@ replace_existing_symlink(int *did_something, const cha */ fd = open(ondisk_path, O_RDWR | O_EXCL | O_NOFOLLOW); if (fd == -1) { - if (errno != ELOOP) + if (!got_err_open_nofollow_on_symlink()) return got_error_from_errno2("open", ondisk_path); /* We are updating an existing on-disk symlink. */ @@ -1781,9 +1781,10 @@ get_file_status(unsigned char *status, struct stat *sb } } else { fd = open(abspath, O_RDONLY | O_NOFOLLOW); - if (fd == -1 && errno != ENOENT && errno != ELOOP) + if (fd == -1 && errno != ENOENT && + !got_err_open_nofollow_on_symlink()) return got_error_from_errno2("open", abspath); - else if (fd == -1 && errno == ELOOP) { + else if (fd == -1 && got_err_open_nofollow_on_symlink()) { if (lstat(abspath, sb) == -1) return got_error_from_errno2("lstat", abspath); } else if (fd == -1 || fstat(fd, sb) == -1) { @@ -3767,7 +3768,7 @@ worktree_status(struct got_worktree *worktree, const c fd = open(ondisk_path, O_RDONLY | O_NOFOLLOW | O_DIRECTORY); if (fd == -1) { if (errno != ENOTDIR && errno != ENOENT && errno != EACCES && - errno != ELOOP) + !got_err_open_nofollow_on_symlink()) err = got_error_from_errno2("open", ondisk_path); else { if (!no_ignores) { @@ -4473,7 +4474,7 @@ create_patched_content(char **path_outfile, int revers if (dirfd2 != -1) { fd2 = openat(dirfd2, de_name2, O_RDONLY | O_NOFOLLOW); if (fd2 == -1) { - if (errno != ELOOP) { + if (!got_err_open_nofollow_on_symlink()) { err = got_error_from_errno2("openat", path2); goto done; } @@ -4487,7 +4488,7 @@ create_patched_content(char **path_outfile, int revers } else { fd2 = open(path2, O_RDONLY | O_NOFOLLOW); if (fd2 == -1) { - if (errno != ELOOP) { + if (!got_err_open_nofollow_on_symlink()) { err = got_error_from_errno2("open", path2); goto done; }