r336219 - Fix crash in clang.

[David Blaikie via cfe-commits]

On Mon, Jul 9, 2018 at 1:52 PM Zachary Turner <zturner at google.com> wrote:

> makeArrayRef() isn't necessary, but when I was first looking at this I had
> to stare at the code for a bit to see that there was an implicit conversion
> happening.  So I put the makeArrayRef() just for the benefit of the person
> reading the code.  But no, it's not necessary.
>

Fair enough. Given that we do StringRef S; S = x; all the time without an
explicit conversion function, etc, I'm not sure this case merits any
different handling - but I don't mind too much if you prefer to keep it.


> This did not fail on existing tests, it failed when a user reported a
> crash.  I'm not sure of a way to write a test to force the original crash
> though.  Every invocation of the clang cc1 driver from the cl driver
> already goes through this code path, and it wasn't crashing on any bots.
> The crash was due to freeing stack memory and then immediately accessing
> that memory.  This will accidentally work a lot of the time.
>

Yeah - UB is UB unfortunately. Given that any execution of this code (well,
one that actually has a non-zero sized result & uses that result later on -
I assume that's the case in at least some of the existing tests in Clang?)
does tickle the bug, but whether or not it actually crashes is unknown -
I'm OK leaving that as-is.


> (Also not sure why our sanitizer bots didn't catch it).
>

Might be interesting to look into/reduce a test case, but may also be a bit
arduous - not sure.


>
> On Mon, Jul 9, 2018 at 1:42 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>> Did this fail on an existing regression test, or is there a need for more
>> test coverage? (guessing it failed on existing tests)
>>
>> Also, is the makeArrayRef necessary? Looks like if the original code
>> compiled (implicitly converting from vector to ArrayRef) then the new code
>> wouldn't need a makeArrayRef either?
>>
>> On Tue, Jul 3, 2018 at 11:17 AM Zachary Turner via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>>
>>> Author: zturner
>>> Date: Tue Jul  3 11:12:39 2018
>>> New Revision: 336219
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=336219&view=rev
>>> Log:
>>> Fix crash in clang.
>>>
>>> This happened during a recent refactor.  toStringRefArray() returns
>>> a vector<StringRef>, which was being implicitly converted to an
>>> ArrayRef<StringRef>, and then the vector was immediately being
>>> destroyed, so the ArrayRef<> was losing its backing storage.
>>> Fix this by making sure the vector gets permanent storage.
>>>
>>> Modified:
>>>     cfe/trunk/lib/Driver/Job.cpp
>>>
>>> Modified: cfe/trunk/lib/Driver/Job.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Job.cpp?rev=336219&r1=336218&r2=336219&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Driver/Job.cpp (original)
>>> +++ cfe/trunk/lib/Driver/Job.cpp Tue Jul  3 11:12:39 2018
>>> @@ -318,10 +318,12 @@ int Command::Execute(ArrayRef<llvm::Opti
>>>    SmallVector<const char*, 128> Argv;
>>>
>>>    Optional<ArrayRef<StringRef>> Env;
>>> +  std::vector<StringRef> ArgvVectorStorage;
>>>    if (!Environment.empty()) {
>>>      assert(Environment.back() == nullptr &&
>>>             "Environment vector should be null-terminated by now");
>>> -    Env = llvm::toStringRefArray(Environment.data());
>>> +    ArgvVectorStorage = llvm::toStringRefArray(Environment.data());
>>> +    Env = makeArrayRef(ArgvVectorStorage);
>>>    }
>>>
>>>    if (ResponseFile == nullptr) {
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180709/7e3a7032/attachment.html>