[Support] Report EISDIR when opening a directory #79880
Conversation
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-llvm-binary-utilities Author: None (azhan92) ChangesThe test Full diff: https://github.com/llvm/llvm-project/pull/79880.diff 4 Files Affected:
diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc
index 9f89d63bb0fda84..fe8af76bd4d6ada 100644
--- a/llvm/lib/Support/Unix/Path.inc
+++ b/llvm/lib/Support/Unix/Path.inc
@@ -1024,6 +1024,13 @@ std::error_code openFile(const Twine &Name, int &ResultFD,
auto Open = [&]() { return ::open(P.begin(), OpenFlags, Mode); };
if ((ResultFD = sys::RetryAfterSignal(-1, Open)) < 0)
return std::error_code(errno, std::generic_category());
+ if (Access == FA_Read) {
+ struct stat Status;
+ if (fstat(ResultFD, &Status) == -1)
+ return std::error_code(errno, std::generic_category());
+ if (S_ISDIR(Status.st_mode))
+ return make_error_code(errc::is_a_directory);
+ }
#ifndef O_CLOEXEC
if (!(Flags & OF_ChildInherit)) {
int r = fcntl(ResultFD, F_SETFD, FD_CLOEXEC);
diff --git a/llvm/test/tools/llvm-symbolizer/input-file-err.test b/llvm/test/tools/llvm-symbolizer/input-file-err.test
index df14da2f433c012..76115b513470b9c 100644
--- a/llvm/test/tools/llvm-symbolizer/input-file-err.test
+++ b/llvm/test/tools/llvm-symbolizer/input-file-err.test
@@ -1,5 +1,3 @@
-Failing on AIX due to D153595. The test expects a different error message from the one given on AIX.
-XFAIL: target={{.*}}-aix{{.*}}
RUN: not llvm-addr2line -e %p/Inputs/nonexistent 0x12 2>&1 | FileCheck %s --check-prefix=CHECK-NONEXISTENT-A2L -DMSG=%errc_ENOENT
RUN: not llvm-addr2line -e %p/Inputs/nonexistent 2>&1 | FileCheck %s --check-prefix=CHECK-NONEXISTENT-A2L -DMSG=%errc_ENOENT
CHECK-NONEXISTENT-A2L: llvm-addr2line{{.*}}: error: '{{.*}}Inputs/nonexistent': [[MSG]]
diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp
index bbeb9d5dc19bda1..23f6081cd32a455 100644
--- a/llvm/unittests/Support/CommandLineTest.cpp
+++ b/llvm/unittests/Support/CommandLineTest.cpp
@@ -1117,7 +1117,6 @@ TEST(CommandLineTest, BadResponseFile) {
ASSERT_STREQ(Argv[0], "clang");
ASSERT_STREQ(Argv[1], AFileExp.c_str());
-#if !defined(_AIX) && !defined(__MVS__)
std::string ADirExp = std::string("@") + std::string(ADir.path());
Argv = {"clang", ADirExp.c_str()};
Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
@@ -1125,7 +1124,6 @@ TEST(CommandLineTest, BadResponseFile) {
ASSERT_EQ(2U, Argv.size());
ASSERT_STREQ(Argv[0], "clang");
ASSERT_STREQ(Argv[1], ADirExp.c_str());
-#endif
}
TEST(CommandLineTest, SetDefaultValue) {
diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp
index 837ca03216f87ab..604a8803f60e691 100644
--- a/llvm/unittests/Support/Path.cpp
+++ b/llvm/unittests/Support/Path.cpp
@@ -1296,6 +1296,22 @@ TEST_F(FileSystemTest, UTF8ToUTF16DirectoryIteration) {
}
#endif
+TEST_F(FileSystemTest, OpenDirectoryAsFile) {
+ ASSERT_NO_ERROR(fs::create_directory(Twine(TestDirectory)));
+ ASSERT_EQ(fs::create_directory(Twine(TestDirectory), false),
+ errc::file_exists);
+
+ int FD;
+ std::error_code EC;
+ EC = fs::openFileForRead(Twine(TestDirectory), FD);
+ ASSERT_EQ(EC, errc::is_a_directory);
+ EC = fs::openFileForWrite(Twine(TestDirectory), FD);
+ ASSERT_EQ(EC, errc::is_a_directory);
+
+ ASSERT_NO_ERROR(fs::remove_directories(Twine(TestDirectory)));
+ ::close(FD);
+}
+
TEST_F(FileSystemTest, Remove) {
SmallString<64> BaseDir;
SmallString<64> Paths[4];
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
While I can understand the motivation behind this change (it can't possibly make sense to open a directory for reading, right?) I'd be concerned about collateral damage resulting from this patch, where somebody might be expecting to be able to open directories using this interface on specific systems.
There's also a minor time of check to time of use issue with this patch, since it could be possible to replace a file with a directory being the new check and the read, although I'm less concerned about that.
Thoughts?
|
@jh7370 The changes are meant to change behaviour on AIX to comply better with what happens on Linux/other platforms. I assume these other platforms should expect non-POSIX-defined behaviour so it shouldn't cause any collateral damage. |
|
I'd like opinions from other AIX developers before giving this the thumbs up. I don't know much about AIX or its usage, so I don't think I'm really the best person to agree to this PR. @diggerlin / @EsmeYi / @stephenpeckham ? |
To be clear: There are likely other platforms that do allow "file" reading of a directory; however, the expectation of at least some tests in the LLVM monorepo project is that such reading of a directory is not allowed. A change in the Support library interface to provide a more consistent behaviour across platforms helps to avoid maintenance issues with these tests (or proliferation of directory checking code in the wider codebase). Thus I think the statement should be that the "EISDIR" behaviour should be expected from the Support library because it is in a position to abstract away some of the differences between platforms. |
|
On Linux, I am concerned that this change would cause two |
llvm/lib/Support/Unix/Path.inc
Outdated
| if (Access == FA_Read) { | ||
| struct stat Status; | ||
| if (fstat(ResultFD, &Status) == -1) | ||
| return std::error_code(errno, std::generic_category()); | ||
| if (S_ISDIR(Status.st_mode)) | ||
| return make_error_code(errc::is_a_directory); | ||
| } |
|
Hi @MaskRay, do the changes look reasonable? |
|
@azhan92, you need to rebase your patch. |
azhan92
force-pushed
the
alisonz/report-eisdir-aix
branch
from
8cf8d97
to
fe7209d
Compare
|
@azhan92, this test failure in the Linux CI results is probably real and looks related to the change: |
|
@azhan92, the failure on Windows looks real too: I think we may just want to find a way to not run the test on Windows (e.g., use a macro guard to leave it out of the Windows build). |
The "ForWrite" test is still failing on Windows. Could we not just fix both the tests for Windows rather than disabling it? I'm pretty sure the behaviour should be the same, just possibly a different error code. If you need me to look into it because you don't have access to a Windows machine, I might be able to, assuming I can get current LLVM to build. |
That would help a lot. We don't have Windows build environments handy. |
llvm/unittests/Support/Path.cpp
Outdated
| Expected<fs::file_t> FD = fs::openNativeFileForRead(TestDirectory); | ||
| ASSERT_NO_ERROR(errorToErrorCode(FD.takeError())); |
There was a problem hiding this comment.
This test fails on Windows here, because on Windows even attempting to opene the directory for read fails, so the ASSERT_NO_ERROR fires.
Side note: ASSERT_NO_ERROR(errorToErrorCode(FD.takeError())); can be simplified to ASSERT_THAT_EXPECTED(FD, Succeeded());. There are similar ones if you just want to check whether it failed with a particular message, for example (but in that case you wouldn't be able to check the actual value).
To fix this on Windows simply replace the ASSERT_NO_ERROR check with a check to make sure the returned error is in a failed state with the is_directory value (then skip the rest of the test).
llvm/unittests/Support/Path.cpp
Outdated
| ASSERT_EQ(EC, errc::is_a_directory); | ||
|
|
||
| ASSERT_NO_ERROR(fs::remove_directories(Twine(TestDirectory))); | ||
| ::close(FD); |
There was a problem hiding this comment.
This test crashes on Windows, because you're attempting to close a file handle that hasn't been opened (more precisely, FD is unitialized). I'm surprised this doesn't provoke an assertion or similar on Linux (maybe it does and the pre-merge checks don't have assertions enabled). Either way, this close here is incorrect. You probably should have something to make sure FD is closed if openFileForWrite succeeds (i.e. the test is failing), to prevent a race condition, but the "normal" flow of the test shouldn't attempt to call close at all.
llvm/unittests/Support/Path.cpp
Outdated
| @@ -1296,6 +1296,39 @@ TEST_F(FileSystemTest, UTF8ToUTF16DirectoryIteration) { | |||
| } | |||
| #endif | |||
|
|
|||
| TEST_F(FileSystemTest, OpenDirectoryAsFileForRead) { | |||
| ASSERT_NO_ERROR(fs::create_directory(Twine(TestDirectory))); | |||
There was a problem hiding this comment.
The code here does not include code to remove this directory. Perhaps it should?
There was a problem hiding this comment.
The use of TestDirectory here does not seem consistent with the other uses of the TestDirectory that is shared between tests.
llvm-project/llvm/unittests/Support/Path.cpp
Lines 665 to 667 in b667596
It seems that TestDirectory is created (and removed) by the test harness.
It seems we don't need to create/remove a directory here?
llvm/unittests/Support/Path.cpp
Outdated
| ::close(FD); | ||
| ASSERT_EQ(EC, errc::is_a_directory); | ||
|
|
||
| ASSERT_NO_ERROR(fs::remove_directories(Twine(TestDirectory))); |
There was a problem hiding this comment.
It is better to use make_scope_exit right after the directory is created.
llvm/unittests/Support/Path.cpp
Outdated
| std::string Buf(5, '?'); | ||
| Expected<fs::file_t> FD = fs::openNativeFileForRead(TestDirectory); | ||
| #ifdef _WIN32 | ||
| ASSERT_EQ(errorToErrorCode(BytesRead.takeError()), errc::is_a_directory); |
There was a problem hiding this comment.
Windows build is failing here. It's quite hard to check the state of an Expected when it's not been declared yet...
llvm/unittests/Support/Path.cpp
Outdated
| @@ -1296,6 +1296,29 @@ TEST_F(FileSystemTest, UTF8ToUTF16DirectoryIteration) { | |||
| } | |||
| #endif | |||
|
|
|||
| TEST_F(FileSystemTest, OpenDirectoryAsFileForRead) { | |||
There was a problem hiding this comment.
Nit: looking at other tests in this file, I believe these two would be better grouped in the file with other tests that attempt to open files for read/write. WDYT?
There was a problem hiding this comment.
Sure, which other file would you suggest?
There was a problem hiding this comment.
Sure, which other file would you suggest?
@azhan92, I think the idea is to leave the tests in this file but to move them within the file to be adjacent with tests that open files for read/write.
llvm/unittests/Support/Path.cpp
Outdated
| EC = fs::openFileForWrite(Twine(TestDirectory), FD); | ||
| if (!EC) | ||
| ::close(FD); | ||
| ASSERT_EQ(EC, errc::is_a_directory); |
There was a problem hiding this comment.
ASSERT_* should be used if the condition failing prevents the rest of the test from running. Otherwise, you should prefer the EXPECT_* macros (so EXPECT_EQ here).
Same goes for your other test too.
Co-authored-by: James Henderson <46713263+jh7370@users.noreply.github.com>
The test
llvm/unittests/Support/CommandLineTest.cppthat handles errors in expansion of response files was previously disabled for AIX. Originally the code was dependent onreadreturningEISDIRwhich occurs on platforms such as Linux. However, other platforms such as AIX allow use ofreadon file descriptors for directories. This change updatesreadNativeFileto produceEISDIRon AIX and z/OS when used on a directory (instead of relying on the call toreadto do so).