[llvm] [Support] Report EISDIR when opening a directory (PR #79880)

James Henderson via llvm-commits llvm-commits at lists.llvm.org
Mon Apr 15 01:05:29 PDT 2024


================
@@ -1296,6 +1296,36 @@ TEST_F(FileSystemTest, UTF8ToUTF16DirectoryIteration) {
 }
 #endif
 
+#ifndef _WIN32
+TEST_F(FileSystemTest, OpenDirectoryAsFileForRead) {
+  ASSERT_NO_ERROR(fs::create_directory(Twine(TestDirectory)));
+  ASSERT_EQ(fs::create_directory(Twine(TestDirectory), false),
+            errc::file_exists);
+
+  std::string Buf(5, '?');
+  Expected<fs::file_t> FD = fs::openNativeFileForRead(TestDirectory);
+  ASSERT_NO_ERROR(errorToErrorCode(FD.takeError()));
+  auto Close = make_scope_exit([&] { fs::closeFile(*FD); });
+  Expected<size_t> BytesRead =
+      fs::readNativeFile(*FD, MutableArrayRef(&*Buf.begin(), Buf.size()));
+  ASSERT_EQ(errorToErrorCode(BytesRead.takeError()), errc::is_a_directory);
+}
+#endif
+
+TEST_F(FileSystemTest, OpenDirectoryAsFileForWrite) {
+  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::openFileForWrite(Twine(TestDirectory), FD);
+  ASSERT_EQ(EC, errc::is_a_directory);
+
+  ASSERT_NO_ERROR(fs::remove_directories(Twine(TestDirectory)));
+  ::close(FD);
----------------
jh7370 wrote:

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.

https://github.com/llvm/llvm-project/pull/79880


More information about the llvm-commits mailing list