[cfe-dev] Clang produces corrupt pch leading to crash

Sun Jul 6 23:52:09 PDT 2014

You even came up with a test, nice. I'll close the bug.

On Mon, Jul 7, 2014 at 4:47 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Thu, Jul 3, 2014 at 12:42 PM, Tobias Hahn <tobias.hahn at ableton.com>
> wrote:
>
>> Hey Richard and Nikola,
>>
>> sorry for taking so long to respond.
>>
>> I checked, and both Richard's patch and mine fix the reduced test-case I
>> reported.
>>
>> However, I tried running them on the original code that caused the crash,
>> and unfortunately, it turns out that this is not the only situation that
>> can cause the DenseMap to grow (and in turn make Entry stale). If the
>> LazyOffsetPtr is in offset-format and needs to be deserialized using
>> GetExternalDecl, this deserialization in turn can also cause a reallocation
>> of the KeyFunctions DenseMap (see the attached stack trace):
>>
>> #1 llvm::DenseMap<clang::CXXRecordDecl const*,
>> clang::LazyOffsetPtr<clang::Decl, unsigned int,
>> &(clang::ExternalASTSource::GetExternalDecl(unsigned int))>,
>> llvm::DenseMapInfo<clang::CXXRecordDecl const*> >::grow(unsigned int)
>> <...>
>> #7 clang::ASTDeclReader::VisitCXXRecordDeclImpl(clang::CXXRecordDecl*)
>> <...>
>> #19 clang::ASTReader::loadPendingDeclChain(unsigned int)
>> #20 clang::ASTReader::finishPendingActions()
>> #21 clang::ASTReader::FinishedDeserializing()
>> #22 non-virtual thunk to clang::ASTReader::FinishedDeserializing()
>> #23 clang::ExternalASTSource::Deserializing::~Deserializing()
>> #24 clang::ExternalASTSource::Deserializing::~Deserializing()
>> #25 clang::ASTReader::ReadDeclRecord(unsigned int)
>> #26 clang::ASTReader::GetDecl(unsigned int)
>> #27 clang::ASTReader::GetExternalDecl(unsigned int)
>> #28 non-virtual thunk to clang::ASTReader::GetExternalDecl(unsigned int)
>> #29 clang::LazyOffsetPtr<clang::Decl, unsigned int,
>> &(clang::ExternalASTSource::GetExternalDecl(unsigned
>> int))>::get(clang::ExternalASTSource*) const
>> #30 clang::ASTContext::getCurrentKeyFunction(clang::CXXRecordDecl const*)
>>
>> I've failed so far to get a minimal case to reproduce this issue. The new
>> patch I've attached fixes both crashes, but to me it seems more like a
>> workaround than a proper fix. There seems to be a conceptual problem:
>> Anytime you try to deref a lazy pointer in KeyFunctions that happens to be
>> in offset format can cause this pointer to be destructed while
>> deserializing it. Right now, I can't seem to think of any suggestion how to
>> fix this issue that I really like. The safest would fix would probably be
>> to change the type of KeyFunctions from
>>
>>    DenseMap<const CXXRecordDecl*, LazyDeclPtr>
>>
>> to something like a
>>
>>   DenseMap<const CXXRecordDecl*, std::shared_ptr<LazyDeclPtr>>
>>
>> (ugly) or using a map that doesn't relocate its contents (like the
>> persistent maps from Clojure).
>>
>> Thoughts?
>>
>
> Fixed in r212437.
>
>
>> Best,
>> Tobias
>>
>> Am 30.06.2014 um 12:06 schrieb Nikola Smiljanic <popizdeh at gmail.com>:
>>
>> > Yep, that one fixes the issue as well and passes regression tests.
>> Disregard my comment about Entry.get. Are you ok with me committing his
>> version Richard? With ampersand aligned with variable of course :P
>> >
>> >
>> > On Mon, Jun 30, 2014 at 6:39 PM, Nikola Smiljanic <popizdeh at gmail.com>
>> wrote:
>> > Ignore my last email.
>> >
>> > Richard's change (plus const_cast and changing Entry. to Entry->) fixes
>> the issue. The patch Tobias proposed should also work and might be a little
>> cleaner? It definitely needs a comment justifying the second lookup.
>> There's also no need to call Entry.get in this case as we know the pointer
>> is null?
>> >
>> >
>> > On Mon, Jun 30, 2014 at 6:22 PM, Nikola Smiljanic <popizdeh at gmail.com>
>> wrote:
>> > I was about to do the same :) But one thing I wanted to ask is, why
>> change Entry into a pointer? Why not just call operator[] twice? As Tobias
>> said we'll need to cast to CXXRecordDecl. And we'll have to change
>> Entry.get to Entry->get if Entry needs to be a pointer.
>> >
>> >
>> > On Mon, Jun 30, 2014 at 5:42 PM, Tobias Hahn <tobias.hahn at ableton.com>
>> wrote:
>> > Hey Richard,
>> >
>> > I'll check your patch in a minute, but except for a missing const_cast
>> I believe it should be fine just by comparing it with the patch I came up
>> with (attached below).
>> >
>> > Best,
>> > Tobias
>> > Am 27.06.2014 um 18:24 schrieb Richard Smith <richard at metafoo.co.uk>:
>> >
>> >
>> > > I think this may be one of those changes for which we need to accept
>> that a unit test would not be useful. Does the change I suggested upthread
>> fix the issue for you?
>> > >
>> > > On Fri, Jun 27, 2014 at 9:15 AM, Tobias Hahn <tobias.hahn at ableton.com>
>> wrote:
>> > > The last test case I sent around is as minimal as I could get it, but
>> still over a hundred loc. It only crashes reliably when clang is built with
>> asan (valgrind would probably be another option). I've tried a patch very
>> similar to yours; and it fixes the issue.
>> > >
>> > > Maybe one could construct a simpler test case with the knowledge that
>> it has to force the KeyFunctions DenseMap to re-allocate?
>> > >
>> > > I've also thought about how to unit-test this, but it seems
>> impossible to write a unit test without exposing the KeyFunctions cache in
>> ASTContext. At this point I stopped because I have no idea what your policy
>> is for such changes and in what general directions this code is meant to
>> evolve.
>> > >
>> > > Best,
>> > > Tobias
>> > >
>> > > Am 27.06.2014 um 17:24 schrieb "Richard Smith" <richard at metafoo.co.uk
>> >:
>> > >
>> > >> On Sun, Jun 22, 2014 at 5:45 PM, Nikola Smiljanic <
>> popizdeh at gmail.com> wrote:
>> > >> Awesome work Tobias, very interesting sequence of events. Adding
>> Richard as he'll know how to best reshuffle this code :D
>> > >>
>> > >> The precompiled header is good, the trouble starts in
>> ASTContext::getCurrentKeyFunctions:
>> > >>
>> > >> LazyDeclPtr &Entry = KeyFunctions[RD];
>> > >>   if (!Entry)
>> > >>     Entry = const_cast<CXXMethodDecl*>(computeKeyFunction(*this,
>> RD));
>> > >>
>> > >> That's a classic :)
>> > >>
>> > >> It should be straightforward to fix this, something like:
>> > >>
>> > >> LazyDeclPtr *Entry = &KeyFunctions[RD];
>> > >> if (!*Entry) {
>> > >>   auto *KeyFunction = computeKeyFunction(*this, RD);
>> > >>   // Entry might have been invalidated by computeKeyFunction.
>> > >>   Entry = &KeyFunctions[RD];
>> > >>   *Entry = KeyFunction;
>> > >> }
>> > >> // do stuff with *Entry
>> > >>
>> > >> Do we have a reduced testcase? I suspect that such a test may be
>> fragile to the point of being useless, so maybe we should go without =(
>> > >>
>> > >> KeyFunctions DenseMap will undergo reallocation inside
>> computeKeyFunctions (see two markers in the attached callstack). Upon
>> return Entry will point to freed memory. Good lord it took me a while to
>> figure this out, I suck at debugging on Linux...
>> > >>
>> > >>
>> > >> On Sat, Jun 21, 2014 at 6:12 AM, Tobias Hahn <
>> tobias.hahn at ableton.com> wrote:
>> > >> Hi Nikola,
>> > >>
>> > >> I can reproduce the bug on linux if I tell clang to cross-compile
>> for osx. Attached is a minimal example. You need to build clang with asan
>> to reproduce the crash:
>> > >>
>> > >> ../llvm/configure --enable-libcpp CFLAGS="-fsanitize=address"
>> CXXFLAGS="-fsanitize=address"
>> > >>
>> > >> Thanks again for looking into this crash!
>> > >>
>> > >> Best,
>> > >> Tobias
>> > >>
>> > >> Am 17.06.2014 um 00:16 schrieb Nikola Smiljanic <popizdeh at gmail.com
>> >:
>> > >>
>> > >>
>> > >> > It might make sense to file this with apple if you're using clang
>> shipped with XCode as they ship their own releases. What version of clang
>> are you using? Is the isystem flag important? What about #include <list>?
>> I've tried to reproduce this but it's not so straightforward because I'm on
>> linux and some of the stuff in that bash script assumes mac os... I was
>> wondering if it's possible to reduce this to something that's reproducible
>> everywhere or if this is a mac specific issue.
>> > >> >
>> > >> >
>> > >> > On Mon, Jun 16, 2014 at 11:55 PM, Nikola Smiljanic <
>> popizdeh at gmail.com> wrote:
>> > >> > Thanks for the detailed report! The only thing you can do more is
>> try and debug this yourself ;)
>> > >> >
>> > >> >
>> > >> > On Mon, Jun 16, 2014 at 8:33 PM, Tobias Hahn <
>> tobias.hahn at ableton.com> wrote:
>> > >> > Hi all,
>> > >> >
>> > >> > I've run into (what I believe is) a memory bug with clang while
>> producing a precompiled header. In short, under certain circumstances,
>> clang will write a pch that causes a crash when trying to use this pch in a
>> later compilation unit.
>> > >> >
>> > >> > Occasionally, while clang is compiling the pch, malloc complains
>> that one of its checksums has been overwritten; while at other times, clang
>> throws an error that a definition has a different exception specification
>> than the declaration two lines above it (when both have no exception
>> specification). Both these symptoms lead me to believe that somewhere clang
>> overwrites memory.
>> > >> >
>> > >> > I have stripped the code that reliably causes this crash down to a
>> few hundred lines and have created a little script to reproduce the bug
>> (details at http://llvm.org/bugs/show_bug.cgi?id=20026). I'm not sure,
>> however, about your process for handling such bugs, which is why I'm
>> cross-posting here. My main question is if there is anything else I could
>> provide you with to help fixing this issue.
>> > >> >
>> > >> > Thank you very much in advance!
>> > >> >
>> > >> > Best,
>> > >> > Tobias
>> > >> >
>> > >> >
>> > >> > Ableton AG, Schoenhauser Allee 6-7, 10119 Berlin, Germany
>> > >> > Sitz (Registered Office) Berlin, Amtsgericht
>> Berlin-Charlottenburg, HRB 72838
>> > >> > Vorstand (Management Board): Gerhard Behles, Jan Bohl
>> > >> > Vorsitzender des Aufsichtsrats (Chair of the Supervisory Board):
>> Uwe Struck
>> > >> >
>> > >> >
>> > >> >
>> > >> > _______________________________________________
>> > >> > cfe-dev mailing list
>> > >> > cfe-dev at cs.uiuc.edu
>> > >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>> > >> >
>> > >> >
>> > >>
>> > >> Ableton AG, Schoenhauser Allee 6-7, 10119 Berlin, Germany
>> > >> Sitz (Registered Office) Berlin, Amtsgericht Berlin-Charlottenburg,
>> HRB 72838
>> > >> Vorstand (Management Board): Gerhard Behles, Jan Bohl
>> > >> Vorsitzender des Aufsichtsrats (Chair of the Supervisory Board): Uwe
>> Struck
>> > >> > >>
>> > >>
>> > >>
>> > >>
>> > >
>> >
>> > Ableton AG, Schoenhauser Allee 6-7, 10119 Berlin, Germany
>> > Sitz (Registered Office) Berlin, Amtsgericht Berlin-Charlottenburg, HRB
>> 72838
>> > Vorstand (Management Board): Gerhard Behles, Jan Bohl
>> > Vorsitzender des Aufsichtsrats (Chair of the Supervisory Board): Uwe
>> Struck
>> > >
>> >
>> >
>> >
>> >
>>
>> Ableton AG, Schoenhauser Allee 6-7, 10119 Berlin, Germany
>> Sitz (Registered Office) Berlin, Amtsgericht Berlin-Charlottenburg, HRB
>> 72838
>> Vorstand (Management Board): Gerhard Behles, Jan Bohl
>> Vorsitzender des Aufsichtsrats (Chair of the Supervisory Board): Uwe
>> Struck
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140707/689aa26f/attachment.html>