[Support] Report EISDIR when opening a directory

[azhan92]

Summary

The test llvm/unittests/Support/CommandLineTest.cpp that handles errors in expansion of response files was previously disabled for AIX in https://reviews.llvm.org/rG6ca33cb925edf17fa837b773cf3cfdd922a0e88f. Originally the code was dependent on read returning EISDIR which occurs on platforms such as Linux. However, other platforms such as AIX follow POSIX in reporting EISDIR only if write access is requested when opening a directory as a file. This change introduces an implementation that checks the validity of the path in llvm/lib/Support/CommandLine.cpp instead of relying on read.

[sepavloff]

llvm/lib/Support/CommandLine.cpp

1311 ↗

(On Diff #526073)

I think putting single quotes around file name (as in the call of createStringError above) would fix failing test.

[azhan92]

Updating error message to match expected input.

[sepavloff]

LGTM.

Thanks!

[hubert.reinterpretcast]

llvm/lib/Support/CommandLine.cpp

1312 ↗

(On Diff #526445)

@azhan92, how was the string ("': Is a directory") determined in terms of the text and capitalization? I ask because strerror on different platforms may return different text (for example, may include message catalogue numbers or use different capitalization).

[azhan92]

llvm/lib/Support/CommandLine.cpp

1311 ↗

(On Diff #526073)

Yes I think you're right, thank you!

1312 ↗

(On Diff #526445)

I used that string because that's the message used on Linux, and also what this lit test expects (https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/response-file-errs.c).

[hubert.reinterpretcast]

llvm/lib/Support/CommandLine.cpp

1312 ↗

(On Diff #526445)

@azhan92, for consistency, please use the message() from the error_code.
For example, z/OS generates: EDC5123I Is a directory.

[azhan92]

llvm/lib/Support/CommandLine.cpp

1312 ↗

(On Diff #526445)

Sorry, I replied to your comment awhile ago but I forgot to submit it. I determined this string based off this lit test (https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/response-file-errs.c) as well as the failing external bot failure (https://lab.llvm.org/buildbot/#/builders/214/builds/8134/steps/6/logs/FAIL__LLVM__input-file-err_test) since they both expect '{{.*}}Inputs': {{[Ii]}}s a directory

[Jake-Egan]

The test LLVM :: tools/llvm-symbolizer/input-file-err.test fails because it expects "Is a directory" as an error message, but we currently get "The file was not recognized as a valid object file" on AIX.
https://lab.llvm.org/buildbot/#/builders/214/builds/8134/steps/6/logs/FAIL__LLVM__input-file-err_test

I've added an XFAIL to the test for now. https://reviews.llvm.org/rG64ca650cf9f180cc0b68c0005639028084066e10. This patch should fix the test, so please verify and update the patch to revert the XFAIL commit.

[azhan92]

Updating to use message() from the error_code.

[azhan92]

Previous revision failed premerge checks.

[azhan92]

Remove unrelated changes.

[hubert.reinterpretcast]

@azhan92, please incorporate a revert of https://reviews.llvm.org/rG64ca650cf9f180cc0b68c0005639028084066e10. Since it is an XFAIL. once the problem is fixed, the test will end up being an "unexpected success" unless we remove the XFAIL.

[hubert.reinterpretcast]

@azhan92, rG6ace52e5e49cff6664fc301fa4985fc28c88f26f and rGc14df228ff3ca73e3c5c00c495216bba56665fd5 should also be reverted. Same reason: XFAIL.

[hubert.reinterpretcast]

In D151567#4458232, @hubert.reinterpretcast wrote:

@azhan92, please incorporate a revert of https://reviews.llvm.org/rG64ca650cf9f180cc0b68c0005639028084066e10. Since it is an XFAIL. once the problem is fixed, the test will end up being an "unexpected success" unless we remove the XFAIL.

@azhan92, I believe that the fix needs to be done at a lower level. Currently, we are addressing one case (in expandResponseFiles). That does not address other cases where the ability to open directories for reading on AIX causes unexpected behaviour for LLVM.

We should go deeper to see where the EISDIR error originates (likely "below" the expandResponseFile call) on other platforms.

[azhan92]

@hubert.reinterpretcast I have the call stack on AIX:

#0  0x090000000004304c in read () from /usr/lib/libc.a(shr_64.o)
#1  0x0000000100d5f044 in llvm::sys::RetryAfterSignal<int, long (int, void*, unsigned long), int, char*, unsigned long>(int const&, long ( const&)(int, void*, unsigned long), int const&, char* const&, unsigned long const&) (
    Fail=@0xfffffffffff92e4: -1, F=@0x9001000a0083ec0: {long (int, void *, unsigned long)} 0x9001000a0083ec0 <_$STATIC+22000>, As=@0xfffffffffff92f0: 16384, As=@0xfffffffffff92f0: 16384, As=@0xfffffffffff92f0: 16384)
    at /home/alisonz/llvm/dev/llvm-project/llvm/include/llvm/Support/Errno.h:37
#2  0x0000000100d5973c in llvm::sys::fs::readNativeFile (FD=3, Buf=...) at /home/alisonz/llvm/dev/llvm-project/llvm/lib/Support/Unix/Path.inc:1186
#3  0x0000000100d59404 in llvm::sys::fs::readNativeFileToEOF (FileHandle=3, Buffer=..., ChunkSize=16384) at /home/alisonz/llvm/dev/llvm-project/llvm/lib/Support/Path.cpp:1183
#4  0x00000001010973e0 in getMemoryBufferForStream (FD=3, BufferName=...) at /home/alisonz/llvm/dev/llvm-project/llvm/lib/Support/MemoryBuffer.cpp:247
#5  0x0000000101096df0 in getOpenFileImpl<llvm::MemoryBuffer> (FD=3, Filename=..., FileSize=18446744073709551615, MapSize=18446744073709551615, Offset=0, RequiresNullTerminator=true, IsVolatile=false, Alignment=...)
    at /home/alisonz/llvm/dev/llvm-project/llvm/lib/Support/MemoryBuffer.cpp:474
#6  0x0000000101096be8 in llvm::MemoryBuffer::getOpenFile (FD=3, Filename=..., FileSize=18446744073709551615, RequiresNullTerminator=true, IsVolatile=false, Alignment=...)
    at /home/alisonz/llvm/dev/llvm-project/llvm/lib/Support/MemoryBuffer.cpp:527
#7  0x00000001015940a0 in (anonymous namespace)::RealFile::getBuffer (this=0x1105f6610, Name=..., FileSize=-1, RequiresNullTerminator=true, IsVolatile=false) at /home/alisonz/llvm/dev/llvm-project/llvm/lib/Support/VirtualFileSystem.cpp:229
#8  0x0000000101570cb8 in llvm::vfs::FileSystem::getBufferForFile (this=0x1105f6210, Name=..., FileSize=-1, RequiresNullTerminator=true, IsVolatile=false) at /home/alisonz/llvm/dev/llvm-project/llvm/lib/Support/VirtualFileSystem.cpp:124
#9  0x000000010020b058 in llvm::cl::ExpansionContext::expandResponseFile (this=0xfffffffffffea58, FName=..., NewArgv=...) at /home/alisonz/llvm/dev/llvm-project/llvm/lib/Support/CommandLine.cpp:1157
#10 0x000000010020dd44 in llvm::cl::ExpansionContext::expandResponseFiles (this=0xfffffffffffea58, Argv=...) at /home/alisonz/llvm/dev/llvm-project/llvm/lib/Support/CommandLine.cpp:1332
#11 0x0000000100210d78 in llvm::cl::ExpandResponseFiles (Saver=..., Tokenizer=@0x1104acfe0: 0x100208890 <llvm::cl::TokenizeGNUCommandLine(llvm::StringRef, llvm::StringSaver&, llvm::SmallVectorImpl<char const*>&, bool)>, Argv=...)
    at /home/alisonz/llvm/dev/llvm-project/llvm/lib/Support/CommandLine.cpp:1381
#12 0x000000010017b7cc in (anonymous namespace)::CommandLineTest_BadResponseFile_Test::TestBody (this=0x1105da090) at /home/alisonz/llvm/dev/llvm-project/llvm/unittests/Support/CommandLineTest.cpp:1066

By lower level do you mean in llvm/include/llvm/Support/Errno.h?

[azhan92]

Check if directory before opening as a file.

[azhan92]

This comment was removed by azhan92.

[azhan92]

Check if file is directory before opening.

[azhan92]

Check if file is directory before opening for read (and undo previous mistake).

[hubert.reinterpretcast]

llvm/lib/Support/Unix/Path.inc

1020

Please try using fstat on the result of open to address the comments from https://reviews.llvm.org/D156798.

[hubert.reinterpretcast]

I believe the unit tests should be updated to ensure that we get EISDIR when opening directories for reading and for writing: llvm/unittests/Support/Path.cpp.

[azhan92]

Address TOCTOU condition (will add updated test later).

[MaskRay]

Thanks for the patch, but there are two issues that should be fixed:

(1) stat => fstat
(2) change the subject to mean that this is not AIX specific ([LLVM][Support] Report EISDIR when opening a directory on AIX). Other OSes including Linux are changed as well. I think keeping [Support] and dropping [LLVM] is fine. [Support] is sufficient to describe the scope of the patch.

[azhan92]

Add unit test to verify patch.

llvm/lib/Support/Unix/Path.inc

std::error_code openFile(const Twine &Name, int &ResultFD,
                          OpenFlags Flags, unsigned Mode) {
   int OpenFlags = nativeOpenFlags(Disp, Flags, Access);
 
   SmallString<128> Storage;
   StringRef P = Name.toNullTerminatedStringRef(Storage);
   // Call ::open in a lambda to avoid overload resolution in RetryAfterSignal
   // when open is overloaded, such as in Bionic.
   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);
     (void)r;
     assert(r == 0 && "fcntl(F_SETFD, FD_CLOEXEC) failed");
   }
 #endif
 

llvm/test/tools/llvm-symbolizer/input-file-err.test

-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]]
 
 RUN: not llvm-symbolizer -e %p/Inputs/nonexistent 0x12 2>&1 | FileCheck %s --check-prefix=CHECK-NONEXISTENT -DMSG=%errc_ENOENT
 RUN: not llvm-symbolizer -e %p/Inputs/nonexistent 2>&1 | FileCheck %s --check-prefix=CHECK-NONEXISTENT -DMSG=%errc_ENOENT
 CHECK-NONEXISTENT: llvm-symbolizer{{.*}}: error: '{{.*}}Inputs/nonexistent': [[MSG]]
 
 RUN: not llvm-addr2line -e %p/Inputs 0x12 2>&1 | FileCheck %s --check-prefix=CHECK-DIRECTORY-A2L -DMSG=%errc_EISDIR
 RUN: not llvm-addr2line -e %p/Inputs 2>&1 | FileCheck %s --check-prefix=CHECK-DIRECTORY-A2L -DMSG=%errc_EISDIR
 CHECK-DIRECTORY-A2L: llvm-addr2line{{.*}}: error: '{{.*}}Inputs': [[MSG]]
 
 RUN: not llvm-symbolizer -e %p/Inputs 0x12 2>&1 | FileCheck %s --check-prefix=CHECK-DIRECTORY -DMSG=%errc_EISDIR
 RUN: not llvm-symbolizer -e %p/Inputs 2>&1 | FileCheck %s --check-prefix=CHECK-DIRECTORY -DMSG=%errc_EISDIR
 CHECK-DIRECTORY: llvm-symbolizer{{.*}}: error: '{{.*}}Inputs': [[MSG]]

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, BadResponseFile) {
   SmallVector<const char *, 2> Argv = {"clang", AFileExp.c_str()};
 
   bool Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
   ASSERT_TRUE(Res);
   ASSERT_EQ(2U, Argv.size());
   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);
   ASSERT_FALSE(Res);
   ASSERT_EQ(2U, Argv.size());
   ASSERT_STREQ(Argv[0], "clang");
   ASSERT_STREQ(Argv[1], ADirExp.c_str());
-#endif
 }
 
 TEST(CommandLineTest, SetDefaultValue) {
   cl::ResetCommandLineParser();
 
   StackOption<std::string> Opt1("opt1", cl::init("true"));
   StackOption<bool> Opt2("opt2", cl::init(true));
   cl::alias Alias("alias", llvm::cl::aliasopt(Opt2));

llvm/unittests/Support/Path.cpp

TEST_F(FileSystemTest, UTF8ToUTF16DirectoryIteration) {
   EXPECT_EQ(Count, 2U);
 
   ASSERT_NO_ERROR(fs::remove(Twine(RootDir) + "/a"));
   ASSERT_NO_ERROR(fs::remove(Twine(RootDir) + "/b"));
   ASSERT_NO_ERROR(fs::remove(Twine(RootDir)));
 }
 #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];
   int fds[4];
   ASSERT_NO_ERROR(fs::createUniqueDirectory("fs_remove", BaseDir));
 
   ASSERT_NO_ERROR(fs::create_directories(Twine(BaseDir) + "/foo/bar/baz"));
   ASSERT_NO_ERROR(fs::create_directories(Twine(BaseDir) + "/foo/bar/buzz"));