This is an archive of the discontinued LLVM Phabricator instance.

[Support] Report EISDIR when opening a directory
Needs ReviewPublic

Authored by azhan92 on May 26 2023, 8:25 AM.

Details

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.

Diff Detail

Event Timeline

azhan92 created this revision.May 26 2023, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 8:25 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
azhan92 requested review of this revision.May 26 2023, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 8:25 AM
azhan92 retitled this revision from Report EISDIR when opening a directory on AIX to [LLVM][Support] Report EISDIR when opening a directory on AIX.May 26 2023, 8:28 AM
azhan92 edited the summary of this revision. (Show Details)
sepavloff added inline comments.May 26 2023, 10:11 PM
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 updated this revision to Diff 526445.May 29 2023, 8:13 AM

Updating error message to match expected input.

sepavloff accepted this revision.May 29 2023, 10:03 AM

LGTM.

Thanks!

This revision is now accepted and ready to land.May 29 2023, 10:03 AM
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 added inline comments.May 29 2023, 10:30 AM
llvm/lib/Support/CommandLine.cpp
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).

1311 ↗(On Diff #526073)

Yes I think you're right, thank you!

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 added inline comments.Jun 26 2023, 10:32 AM
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

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 updated this revision to Diff 535097.Jun 27 2023, 12:28 PM

Updating to use message() from the error_code.

azhan92 updated this revision to Diff 535385.Jun 28 2023, 7:05 AM

Previous revision failed premerge checks.

Herald added a reviewer: rriddle. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a reviewer: kuhar. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
azhan92 updated this revision to Diff 535389.Jun 28 2023, 7:13 AM

Remove unrelated changes.

@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, 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 marked 3 inline comments as done.Jun 29 2023, 8:59 AM

@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 updated this revision to Diff 546051.Aug 1 2023, 7:24 AM

Check if directory before opening as a file.

azhan92 updated this revision to Diff 546053.Aug 1 2023, 7:26 AM
This comment was removed by azhan92.
azhan92 updated this revision to Diff 546064.Aug 1 2023, 7:53 AM

Check if file is directory before opening.

foad removed a subscriber: foad.Aug 1 2023, 7:55 AM
azhan92 changed the visibility from "Public (No Login Required)" to "azhan92 (Alison Zhang)".Aug 1 2023, 8:00 AM
azhan92 updated this revision to Diff 546076.Aug 1 2023, 8:17 AM

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

azhan92 changed the visibility from "azhan92 (Alison Zhang)" to "Public (No Login Required)".Aug 1 2023, 8:17 AM
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.

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 updated this revision to Diff 550402.Aug 15 2023, 11:07 AM

Address TOCTOU condition (will add updated test later).

MaskRay requested changes to this revision.EditedAug 15 2023, 11:45 AM

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.

This revision now requires changes to proceed.Aug 15 2023, 11:45 AM
azhan92 retitled this revision from [LLVM][Support] Report EISDIR when opening a directory on AIX to [Support] Report EISDIR when opening a directory .Aug 22 2023, 7:20 AM
azhan92 updated this revision to Diff 552353.Aug 22 2023, 7:22 AM
azhan92 retitled this revision from [Support] Report EISDIR when opening a directory to [Support] Report EISDIR when opening a directory.

Add unit test to verify patch.