[llvm] [llvm][Support] Stop using PWD in current_path (PR #94544)
Iñaki Amatria Barral via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 5 16:05:25 PDT 2024
https://github.com/inaki-amatria created https://github.com/llvm/llvm-project/pull/94544
In a filesystem structured as follows:
```
r/
|- a/
`- b -> a/
```
If current directory is set to `r/b` (a symbolic link to `r/a`) and then changed to `r/a` using `set_current_path()`, subsequent calls to `current_path()` incorrectly return `r/b`. This is because `current_path()` reads the `PWD` environment variable, which still points to the symbolic link `r/b` instead of the actual directory `r/a`.
This issue leads to inaccurate path reporting, causing confusion and potential errors in applications that rely on `current_path()`.
This issue is reproducible with bind mounts too.
>From ed167da84dd9ad59a5b2c61bd237d041efa9d4fc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?I=C3=B1aki=20Amatria=20Barral?= <inaki.amatria at appentra.com>
Date: Thu, 6 Jun 2024 00:52:30 +0200
Subject: [PATCH] [llvm][Support] Stop using PWD in current_path
In a filesystem structured as follows:
```
r/
|- a/
`- b -> a/
```
If current directory is set to `r/b` (a symbolic link to `r/a`) and then
changed to `r/a` using `set_current_path()`, subsequent calls to
`current_path()` incorrectly return `r/b`. This is because
`current_path()` reads the `PWD` environment variable, which still
points to the symbolic link `r/b` instead of the actual directory `r/a`.
This issue leads to inaccurate path reporting, causing confusion and
potential errors in applications that rely on `current_path()`.
This issue is reproducible with bind mounts too.
---
.../llvm/Testing/Support/SupportHelpers.h | 24 ++++++++++++++
llvm/lib/Support/Unix/Path.inc | 10 ------
llvm/test/MC/ELF/comp-dir.s | 8 -----
llvm/unittests/Support/Path.cpp | 32 +++----------------
.../Support/VirtualFileSystemTest.cpp | 17 ++++++++++
5 files changed, 46 insertions(+), 45 deletions(-)
diff --git a/llvm/include/llvm/Testing/Support/SupportHelpers.h b/llvm/include/llvm/Testing/Support/SupportHelpers.h
index 95c2765027ba0..49e44e42a1dc1 100644
--- a/llvm/include/llvm/Testing/Support/SupportHelpers.h
+++ b/llvm/include/llvm/Testing/Support/SupportHelpers.h
@@ -248,6 +248,30 @@ class TempFile {
StringRef path() const { return Path; }
};
+#ifndef _WIN32
+// RAII helper to set and restore an environment variable.
+class WithEnv {
+ const char *Var;
+ std::optional<std::string> OriginalValue;
+
+public:
+ WithEnv(const char *Var, const char *Value) : Var(Var) {
+ if (const char *V = ::getenv(Var))
+ OriginalValue.emplace(V);
+ if (Value)
+ ::setenv(Var, Value, 1);
+ else
+ ::unsetenv(Var);
+ }
+ ~WithEnv() {
+ if (OriginalValue)
+ ::setenv(Var, OriginalValue->c_str(), 1);
+ else
+ ::unsetenv(Var);
+ }
+};
+#endif
+
} // namespace unittest
} // namespace llvm
diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc
index 6e679f74869f0..01af15ebfa52b 100644
--- a/llvm/lib/Support/Unix/Path.inc
+++ b/llvm/lib/Support/Unix/Path.inc
@@ -369,16 +369,6 @@ ErrorOr<space_info> disk_space(const Twine &Path) {
std::error_code current_path(SmallVectorImpl<char> &result) {
result.clear();
- const char *pwd = ::getenv("PWD");
- llvm::sys::fs::file_status PWDStatus, DotStatus;
- if (pwd && llvm::sys::path::is_absolute(pwd) &&
- !llvm::sys::fs::status(pwd, PWDStatus) &&
- !llvm::sys::fs::status(".", DotStatus) &&
- PWDStatus.getUniqueID() == DotStatus.getUniqueID()) {
- result.append(pwd, pwd + strlen(pwd));
- return std::error_code();
- }
-
result.resize_for_overwrite(PATH_MAX);
while (true) {
diff --git a/llvm/test/MC/ELF/comp-dir.s b/llvm/test/MC/ELF/comp-dir.s
index 59edce1ca5452..729f41e872781 100644
--- a/llvm/test/MC/ELF/comp-dir.s
+++ b/llvm/test/MC/ELF/comp-dir.s
@@ -6,13 +6,5 @@
// CHECK: DW_AT_comp_dir [DW_FORM_string] ("{{([A-Za-z]:.*)?}}/test/comp/dir")
-// RUN: mkdir -p %t.foo
-// RUN: ln -sf %t.foo %t.bar
-// RUN: cd %t.foo
-// RUN: env PWD=%t.bar llvm-mc -triple=x86_64-linux-unknown -g %s -filetype=obj -o %t.o
-// RUN: llvm-dwarfdump -v -debug-info %t.o | FileCheck --check-prefix=PWD %s
-// PWD: DW_AT_comp_dir [DW_FORM_string] ("{{.*}}.bar")
-
-
f:
nop
diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp
index bdae7a8ee4b55..02c34ea632aa4 100644
--- a/llvm/unittests/Support/Path.cpp
+++ b/llvm/unittests/Support/Path.cpp
@@ -451,28 +451,6 @@ std::string getEnvWin(const wchar_t *Var) {
}
return expected;
}
-#else
-// RAII helper to set and restore an environment variable.
-class WithEnv {
- const char *Var;
- std::optional<std::string> OriginalValue;
-
-public:
- WithEnv(const char *Var, const char *Value) : Var(Var) {
- if (const char *V = ::getenv(Var))
- OriginalValue.emplace(V);
- if (Value)
- ::setenv(Var, Value, 1);
- else
- ::unsetenv(Var);
- }
- ~WithEnv() {
- if (OriginalValue)
- ::setenv(Var, OriginalValue->c_str(), 1);
- else
- ::unsetenv(Var);
- }
-};
#endif
TEST(Support, HomeDirectory) {
@@ -496,7 +474,7 @@ TEST(Support, HomeDirectory) {
// Apple has their own solution for this.
#if defined(LLVM_ON_UNIX) && !defined(__APPLE__)
TEST(Support, HomeDirectoryWithNoEnv) {
- WithEnv Env("HOME", nullptr);
+ unittest::WithEnv Env("HOME", nullptr);
// Don't run the test if we have nothing to compare against.
struct passwd *pw = getpwuid(getuid());
@@ -510,7 +488,7 @@ TEST(Support, HomeDirectoryWithNoEnv) {
}
TEST(Support, ConfigDirectoryWithEnv) {
- WithEnv Env("XDG_CONFIG_HOME", "/xdg/config");
+ unittest::WithEnv Env("XDG_CONFIG_HOME", "/xdg/config");
SmallString<128> ConfigDir;
EXPECT_TRUE(path::user_config_directory(ConfigDir));
@@ -518,7 +496,7 @@ TEST(Support, ConfigDirectoryWithEnv) {
}
TEST(Support, ConfigDirectoryNoEnv) {
- WithEnv Env("XDG_CONFIG_HOME", nullptr);
+ unittest::WithEnv Env("XDG_CONFIG_HOME", nullptr);
SmallString<128> Fallback;
ASSERT_TRUE(path::home_directory(Fallback));
@@ -530,7 +508,7 @@ TEST(Support, ConfigDirectoryNoEnv) {
}
TEST(Support, CacheDirectoryWithEnv) {
- WithEnv Env("XDG_CACHE_HOME", "/xdg/cache");
+ unittest::WithEnv Env("XDG_CACHE_HOME", "/xdg/cache");
SmallString<128> CacheDir;
EXPECT_TRUE(path::cache_directory(CacheDir));
@@ -538,7 +516,7 @@ TEST(Support, CacheDirectoryWithEnv) {
}
TEST(Support, CacheDirectoryNoEnv) {
- WithEnv Env("XDG_CACHE_HOME", nullptr);
+ unittest::WithEnv Env("XDG_CACHE_HOME", nullptr);
SmallString<128> Fallback;
ASSERT_TRUE(path::home_directory(Fallback));
diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp
index e9fd9671ea6ab..5d9386ba347dd 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -479,6 +479,23 @@ TEST(VirtualFileSystemTest, BasicRealFSIteration) {
}
#ifdef LLVM_ON_UNIX
+TEST(VirtualFileSystemTest, ChangeCWDToDirFromASymbolicLinkToDir) {
+ // r/
+ // |- a/
+ // `- b -> a/
+ TempDir Root("r", /*Unique=*/true);
+ TempDir ADir(Root.path("a"));
+ TempLink B(ADir.path(), Root.path("b"));
+
+ // PWD points to `/r/b`
+ unittest::WithEnv Env("PWD", B.path().str().c_str());
+
+ // Navigate to `/r/a` from `/r/b`
+ IntrusiveRefCntPtr<vfs::FileSystem> FS = vfs::getRealFileSystem();
+ ASSERT_FALSE(FS->setCurrentWorkingDirectory(ADir.path()));
+ EXPECT_EQ(ADir.path(), *FS->getCurrentWorkingDirectory());
+}
+
TEST(VirtualFileSystemTest, MultipleWorkingDirs) {
// Our root contains a/aa, b/bb, c, where c is a link to a/.
// Run tests both in root/b/ and root/c/ (to test "normal" and symlink dirs).
More information about the llvm-commits
mailing list