[PATCH] Fix a crash that occurs when PWD is invalid.

Andrew Trick atrick at apple.com
Mon Dec 9 18:01:50 PST 2013


On Dec 9, 2013, at 3:54 PM, Eric Christopher <echristo at gmail.com> wrote:

> On Mon, Dec 9, 2013 at 3:48 PM, Andrew Trick <atrick at apple.com> wrote:
>> 
>> On Dec 9, 2013, at 3:09 PM, Eric Christopher <echristo at gmail.com> wrote:
>> 
>>> On Mon, Dec 9, 2013 at 3:02 PM, Andrew Trick <atrick at apple.com> wrote:
>>>> 
>>>> On Dec 9, 2013, at 2:50 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>> 
>>>> On Mon, Dec 9, 2013 at 1:58 PM, Filip Pizlo <fpizlo at apple.com> wrote:
>>>> 
>>>> 
>>>> On Dec 9, 2013, at 12:42 PM, Andrew Trick <atrick at apple.com> wrote:
>>>> 
>>>> 
>>>> On Dec 9, 2013, at 12:01 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>> 
>>>> Out of curiosity why isn't it able to get a compilation dir?
>>>> 
>>>> 
>>>> Probably one of these.
>>>> 
>>>>  The getcwd() function will fail if:
>>>> 
>>>>  [EACCES]           Read or search permission was denied for a component
>>>> of the pathname.
>>>> 
>>>>  [ENOENT]           A component of the pathname no longer exists.
>>>> 
>>>> Phil might know for sure.
>>>> 
>>>> 
>>>> I'm not sure, actually. ;-)  When I hit the bug it was probably ENOENT
>>>> caused by weirdness in our JSC test harness.
>>>> 
>>>> But, broadly, there is no requirement for a client of JavaScriptCore to run
>>>> with $PWD set to anything remotely sensible.  Moreover, it's my
>>>> understanding that libraries and frameworks in general never assume anything
>>>> about the sanity of $PWD unless they are asked to do relative path IO.  This
>>>> isn't just a JSC bug: if the claim is that LLVM is a JIT library then it's
>>>> probably best if LLVM gracefully handles not having a sane $PWD.
>>>> 
>>>> 
>>>> That's not quite what I'm talking about, FWIW I agree that you
>>>> shouldn't have to have an environment at all :)
>>>> 
>>>> Basically the lines of code in MCDwarf.cpp are supposed to be when
>>>> generating dwarf for assembly files and I wasn't under the impression
>>>> you were compiling .s files. :)
>>>> 
>>>> That code should probably have some decent fallback in it, but I don't
>>>> think a null DW_AT_compilation_dir is the correct one. It should omit
>>>> the attribute if it can't get a valid value.
>>>> 
>>>> 
>>>> You think we should omit the entire compile_unit DIE if we don’t find the
>>>> absolute path? If we simply omit the DIE, do you think any tools could be
>>>> confused? Is it worth adding a special case for a situation that cannot
>>>> conceivably happen in the context of compiling .s files? I’d be happy to
>>>> simply assert that CompilationDir is non-empty if that’s preferable to
>>>> emitting a DIE with an empty path.
>>>> 
>>> 
>>> No, I think you should omit the DW_AT_compilation_dir attribute on the
>>> DIE if it's empty. I still don't understand how you're getting down
>>> this path, but…
>> 
>> Maybe the problem was my commit comment:
>> 
>> ---
>> MCJIT needs to be able to run in hostile environments, even when PWD
>> is invalid. There's no need to crash MCJIT in this case.
>> 
>> The obvious fix is to simply leave MCContext's CompilationDir empty
>> when PWD can't be determined. This way, MCJIT clients,
>> and other clients that link with LLVM don’t need a valid working directory.
>> 
>> If we do want to guarantee valid CompilationDir, that should be done
>> only for clients of getCompilationDir(). This is as simple as checking
>> for an empty string.
>> 
>> The only current use of getCompilationDir is EmitGenDwarfInfo. This case won’t
>> conceivably run with an invalid working dir, and if it did (hypothetically), skipping
>> the AT_comp_dir attribute is a reasonable way to handle it.
> 
> I understood all of this.
> 
>> ---
>> 
>> I’m not sure how to “omit the attribute” other than emitting a NULL byte in its place.
>> 
> 
> You don't emit it at all. You'll probably need to adjust the
> abbreviation as well.
> 
> An alternate workaround is to set it via the "command line" in the
> case that you're running into.

We’re not running into this case, nor do I think it is possible. However, I did hack an LLVM build to test this code path, and I think this is the patch you’re looking for:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131209/00cd0a43/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-a-crash-that-occurs-when-PWD-is-invalid.patch
Type: application/octet-stream
Size: 3765 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131209/00cd0a43/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131209/00cd0a43/attachment-0001.html>


More information about the llvm-commits mailing list