From 435848cef12d065b209c82c96ce3a8bfa5e6a051 Mon Sep 17 00:00:00 2001 From: aszlig Date: Fri, 1 Apr 2022 09:23:43 -0700 Subject: [PATCH 1/8] libutil: Fix restoring mount namespace I regularly pass around simple scripts by using nix-shell as the script interpreter, eg. like this: #!/usr/bin/env nix-shell #!nix-shell -p dd_rescue coreutils bash -i bash While this works most of the time, I recently had one occasion where it would not and the above would result in the following: $ sudo ./myscript.sh bash: ./myscript.sh: No such file or directory Note the "sudo" here, because this error only occurs if we're root. The reason for the latter is because running Nix as root means that we can directly access the store, which makes sure we use a filesystem namespace to make the store writable. XXX - REWORD! So when stracing the process, I stumbled on the following sequence: openat(AT_FDCWD, "/proc/self/ns/mnt", O_RDONLY) = 3 unshare(CLONE_NEWNS) = 0 ... later ... getcwd("/the/real/cwd", 4096) = 14 setns(3, CLONE_NEWNS) = 0 getcwd("/", 4096) = 2 In the whole strace output there are no calls to chdir() whatsoever, so I decided to look into the kernel source to see what else could change directories and found this[1]: /* Update the pwd and root */ set_fs_pwd(fs, &root); set_fs_root(fs, &root); The set_fs_pwd() call is roughly equivalent to a chdir() syscall and this is called when the setns() syscall is invoked[2]. [1]: https://github.com/torvalds/linux/blob/b14ffae378aa1db993e62b01392e70d1e585fb23/fs/namespace.c#L4659 [2]: https://github.com/torvalds/linux/blob/b14ffae378aa1db993e62b01392e70d1e585fb23/kernel/nsproxy.c#L346 --- src/libutil/util.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 59e3aad6d..e62672717 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -1688,13 +1689,17 @@ void setStackSize(size_t stackSize) #endif } +#if __linux__ static AutoCloseFD fdSavedMountNamespace; +std::optional savedCwd; +#endif void saveMountNamespace() { #if __linux__ static std::once_flag done; std::call_once(done, []() { + savedCwd.emplace(std::filesystem::current_path()); AutoCloseFD fd = open("/proc/self/ns/mnt", O_RDONLY); if (!fd) throw SysError("saving parent mount namespace"); @@ -1712,6 +1717,12 @@ void restoreMountNamespace() } catch (Error & e) { debug(e.msg()); } + try { + if (savedCwd) + std::filesystem::current_path(*savedCwd); + } catch (std::filesystem::filesystem_error const &e) { + debug(e.what()); + } #endif } From 7f5caaa7c0f151520d05d4662415ac09d4cf34b0 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Fri, 1 Apr 2022 10:24:31 -0700 Subject: [PATCH 2/8] libutil: Don't use std::filesystem Just in case making libutil depend on std::filesystem is unacceptable, here is the non-filesystem approach. --- src/libutil/util.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index e62672717..0b18f1027 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -10,7 +10,6 @@ #include #include #include -#include #include #include #include @@ -1691,7 +1690,7 @@ void setStackSize(size_t stackSize) #if __linux__ static AutoCloseFD fdSavedMountNamespace; -std::optional savedCwd; +std::optional savedCwd; #endif void saveMountNamespace() @@ -1699,7 +1698,11 @@ void saveMountNamespace() #if __linux__ static std::once_flag done; std::call_once(done, []() { - savedCwd.emplace(std::filesystem::current_path()); + char* cwd = getcwd(NULL, 0); + if (cwd == NULL) throw SysError("getting cwd"); + savedCwd.emplace(cwd); + free(cwd); + AutoCloseFD fd = open("/proc/self/ns/mnt", O_RDONLY); if (!fd) throw SysError("saving parent mount namespace"); @@ -1717,11 +1720,8 @@ void restoreMountNamespace() } catch (Error & e) { debug(e.msg()); } - try { - if (savedCwd) - std::filesystem::current_path(*savedCwd); - } catch (std::filesystem::filesystem_error const &e) { - debug(e.what()); + if (savedCwd && chdir(savedCwd->c_str()) == -1) { + throw SysError("restoring cwd"); } #endif } From 2a45cf54e4201a894254604676dcb51f4c1a471c Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Fri, 1 Apr 2022 12:20:34 -0700 Subject: [PATCH 3/8] libutil: Properly guard self-allocating getcwd on GNU It's a GNU extension, as pointed out by pennae. --- src/libutil/util.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 0b18f1027..28ab77adc 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1698,10 +1698,19 @@ void saveMountNamespace() #if __linux__ static std::once_flag done; std::call_once(done, []() { - char* cwd = getcwd(NULL, 0); - if (cwd == NULL) throw SysError("getting cwd"); +#ifdef __GNU__ + // getcwd allocating its return value is a GNU extension. + char *cwd = getcwd(NULL, 0); + if (cwd == NULL) +#else + char cwd[PATH_MAX]; + if (!getcwd(cwd, sizeof(cwd))) +#endif + throw SysError("getting cwd"); savedCwd.emplace(cwd); +#ifdef __GNU__ free(cwd); +#endif AutoCloseFD fd = open("/proc/self/ns/mnt", O_RDONLY); if (!fd) From e5b70d47aa656833e4609106f9f1ef48b67664cc Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 4 Apr 2022 08:19:49 -0700 Subject: [PATCH 4/8] libutil: cleanup savedCwd logic Co-authored-by: Eelco Dolstra --- src/libutil/util.cc | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 28ab77adc..9b34d5d72 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1698,20 +1698,7 @@ void saveMountNamespace() #if __linux__ static std::once_flag done; std::call_once(done, []() { -#ifdef __GNU__ - // getcwd allocating its return value is a GNU extension. - char *cwd = getcwd(NULL, 0); - if (cwd == NULL) -#else - char cwd[PATH_MAX]; - if (!getcwd(cwd, sizeof(cwd))) -#endif - throw SysError("getting cwd"); - savedCwd.emplace(cwd); -#ifdef __GNU__ - free(cwd); -#endif - + savedCwd = absPath("."); AutoCloseFD fd = open("/proc/self/ns/mnt", O_RDONLY); if (!fd) throw SysError("saving parent mount namespace"); From e135d223f66f77f2b03a11b979d714e582364013 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 4 Apr 2022 08:32:45 -0700 Subject: [PATCH 5/8] libutil: save fd to cwd instead of cwd itself --- src/libutil/util.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 9b34d5d72..a00a03978 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1690,7 +1690,7 @@ void setStackSize(size_t stackSize) #if __linux__ static AutoCloseFD fdSavedMountNamespace; -std::optional savedCwd; +static AutoCloseFD fdSavedCwd; #endif void saveMountNamespace() @@ -1698,11 +1698,15 @@ void saveMountNamespace() #if __linux__ static std::once_flag done; std::call_once(done, []() { - savedCwd = absPath("."); AutoCloseFD fd = open("/proc/self/ns/mnt", O_RDONLY); if (!fd) throw SysError("saving parent mount namespace"); fdSavedMountNamespace = std::move(fd); + + fd = open("/proc/self/cwd", O_RDONLY); + if (!fd) + throw SysError("saving cwd"); + fdSavedCwd = std::move(fd); }); #endif } @@ -1716,7 +1720,7 @@ void restoreMountNamespace() } catch (Error & e) { debug(e.msg()); } - if (savedCwd && chdir(savedCwd->c_str()) == -1) { + if (fdSavedCwd && fchdir(fdSavedCwd.get()) == -1) { throw SysError("restoring cwd"); } #endif From f89b0f7846f2177b65eebdbff7f24a255d66819e Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 4 Apr 2022 08:33:59 -0700 Subject: [PATCH 6/8] libutil: `try` restoring the cwd from fdSavedCwd --- src/libutil/util.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index a00a03978..1f800f3f4 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1717,12 +1717,12 @@ void restoreMountNamespace() try { if (fdSavedMountNamespace && setns(fdSavedMountNamespace.get(), CLONE_NEWNS) == -1) throw SysError("restoring parent mount namespace"); + if (fdSavedCwd && fchdir(fdSavedCwd.get()) == -1) { + throw SysError("restoring cwd"); + } } catch (Error & e) { debug(e.msg()); } - if (fdSavedCwd && fchdir(fdSavedCwd.get()) == -1) { - throw SysError("restoring cwd"); - } #endif } From 10b9c1b2b269b96dcb8b3d298491fa143d1663e8 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 4 Apr 2022 10:16:30 -0700 Subject: [PATCH 7/8] libutil: save cwd fd in restoreMountNamespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This doesn't work very well (maybe I'm misunderstanding the desired implementation): : ~/w/vc/nix ; doas outputs/out/bin/nix --experimental-features 'nix-command flakes' develop -c pwd pwd: couldn't find directory entry in ‘../../../..’ with matching i-node --- src/libutil/util.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 1f800f3f4..701545589 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1690,7 +1690,6 @@ void setStackSize(size_t stackSize) #if __linux__ static AutoCloseFD fdSavedMountNamespace; -static AutoCloseFD fdSavedCwd; #endif void saveMountNamespace() @@ -1702,11 +1701,6 @@ void saveMountNamespace() if (!fd) throw SysError("saving parent mount namespace"); fdSavedMountNamespace = std::move(fd); - - fd = open("/proc/self/cwd", O_RDONLY); - if (!fd) - throw SysError("saving cwd"); - fdSavedCwd = std::move(fd); }); #endif } @@ -1715,6 +1709,10 @@ void restoreMountNamespace() { #if __linux__ try { + AutoCloseFD fdSavedCwd = open("/proc/self/cwd", O_RDONLY); + if (!fdSavedCwd) { + throw SysError("saving cwd"); + } if (fdSavedMountNamespace && setns(fdSavedMountNamespace.get(), CLONE_NEWNS) == -1) throw SysError("restoring parent mount namespace"); if (fdSavedCwd && fchdir(fdSavedCwd.get()) == -1) { From 56009b2639dc878be11f94d096fae56ac14dcd1d Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 4 Apr 2022 10:21:56 -0700 Subject: [PATCH 8/8] libutil: don't save cwd fd, use path instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Saving the cwd fd didn't actually work well -- prior to this commit, the following would happen: : ~/w/vc/nix ; doas outputs/out/bin/nix --experimental-features 'nix-command flakes' run nixpkgs#coreutils -- --coreutils-prog=pwd pwd: couldn't find directory entry in ‘../../../..’ with matching i-node : ~/w/vc/nix ; doas outputs/out/bin/nix --experimental-features 'nix-command flakes' develop -c pwd pwd: couldn't find directory entry in ‘../../../..’ with matching i-node --- src/libutil/util.cc | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 701545589..3132e7161 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1709,13 +1709,11 @@ void restoreMountNamespace() { #if __linux__ try { - AutoCloseFD fdSavedCwd = open("/proc/self/cwd", O_RDONLY); - if (!fdSavedCwd) { - throw SysError("saving cwd"); - } + auto savedCwd = absPath("."); + if (fdSavedMountNamespace && setns(fdSavedMountNamespace.get(), CLONE_NEWNS) == -1) throw SysError("restoring parent mount namespace"); - if (fdSavedCwd && fchdir(fdSavedCwd.get()) == -1) { + if (chdir(savedCwd.c_str()) == -1) { throw SysError("restoring cwd"); } } catch (Error & e) {