[PATCH] Fix clang-tidy delete of stack object

Manuel Klimek klimek at google.com
Thu Nov 13 01:20:27 PST 2014 

+dblakie

On Thu Nov 13 2014 at 1:53:39 AM Alexander Kornienko <alexfh at google.com>
wrote:

> Thank you for the analysis and the proposed solution!
>
> I can reproduce the issue (with any q.cpp that is not clang-tidy clean):
>
> $ clang-tidy q.cpp -- --serialize-diagnostics test.dia
> *** Error in `clang_tidy': free(): invalid pointer: 0x00007fffa65bb4d8 ***
> Aborted (core dumped)
>
>
> The patch seems correct to me and the way to distinguish between owning
> and non-owning constructors seems also fine. I'll commit the patch tomorrow
> if nobody offers a better solution.
>

I don't really see anything better under the current restrictions. Perhaps
David has an idea, he has done a lot of the unique_ptr migrations in llvm.


>
> -- Alex
>
> On Wed, Nov 12, 2014 at 9:32 PM, Aaron Wishnick <
> aaron.s.wishnick at gmail.com> wrote:
>
>> Understanding the bug better, I've attached a patch that more correctly
>> fixes this bug, by teaching ChainedDiagnosticConsumer how to not take
>> ownership of one of its arguments, and having SetupSerializedDiagnostics()
>> use it. Is there a more idiomatic way, in the LLVM project, of a "maybe"
>> owning pointer? I see that some related functions take a "ShouldOwnClient"
>> argument, but this seems a little more kludgy for two arguments with
>> separate ownership.
>>
>> Thanks,
>> Aaron
>>
>> On Wed, Nov 12, 2014 at 3:14 PM, Aaron Wishnick <
>> aaron.s.wishnick at gmail.com> wrote:
>>
>>> Hi Alexander, sorry to dig up an old issue, but I've just gotten some
>>> more time to look into it. This is still reproducing for me on trunk, and I
>>> can see where the ChainedDiagnosticConsumer is created, as well as why it
>>> ends up trying to free a stack object. In short, there's a function
>>> SetupSerializedDiagnostics() in CompilerInstance.cpp that doesn't know how
>>> to handle the case where its DiagnosticsEngine doesn't own its client. This
>>> bug can be reproduced by using clang-tidy with a compilation database that
>>> uses the "--serialize-diagnostics" flag.
>>>
>>> When I run a debug build with the arguments "clang-tidy -p
>>> /path/to/compile_commands.json /path/to/source.cpp", I get a failed assert
>>> in tools/clang/lib/Frontend/CompilerInstance.cpp, line 173, in
>>> SetupSerializedDiagnostics():
>>>
>>> static void SetupSerializedDiagnostics(DiagnosticOptions *DiagOpts,
>>>                                        DiagnosticsEngine &Diags,
>>>                                        StringRef OutputFile) {
>>>   auto SerializedConsumer =
>>>       clang::serialized_diags::create(OutputFile, DiagOpts);
>>>
>>>   assert(Diags.ownsClient());
>>>   Diags.setClient(new ChainedDiagnosticConsumer(
>>>       std::unique_ptr<DiagnosticConsumer>(Diags.takeClient()),
>>>       std::move(SerializedConsumer)));
>>> }
>>>
>>> Stepping one stack frame up into createDiagnostics(), it looks like this
>>> code path is hit because the "if
>>> (!Opts->DiagnosticSerializationFile.empty())" condition on line 209 of
>>> CompilerInstance.cpp is met.
>>>
>>> If I skip that assert, and continue, I get that same "pointer being
>>> freed was not allocated" error, once the program finishes and the
>>> ChainedDiagnosticConsumer is deleted. The address is from the stack, rather
>>> than the heap, and it corresponds to the value of "Diags.Client" before
>>> that call to "Diags.takeClient()." In other words, I think the problem is
>>> that the DiagnosticsEngine passed into SetupSerializedDiagnostics doesn't
>>> own its client, and the client is stack allocated, and then the client is
>>> stored in a unique_ptr which is owned by the ChainedDiagnosticConsumer.
>>>
>>> Ultimately, I can see this comes from ClangTidy.cpp, line 470. This
>>> ClangTidyDiagnosticConsumer is created on the stack, and is the one that
>>> eventually ends up being freed, causing the bug.
>>>
>>> I am using this in conjunction with Xcode: I am using xcodebuild to
>>> build my project, and then oclint-xcodebuild to generate the
>>> compile_commands.json database. Sure enough, all of the commands in the
>>> compilation database include the argument "--serialize-diagnostics
>>> /path/to/source.dia". If I remove these arguments, this bug doesn't occur.
>>> So, I think the issue is that SetupSerializedDiagnostics doesn't know how
>>> to handle the case where the DiagnosticsEngine doesn't own its client.
>>>
>>> Hope this helps!
>>>
>>> Best,
>>> Aaron
>>>
>>>
>>> On Mon, Jun 23, 2014 at 9:51 AM, Alexander Kornienko <alexfh at google.com>
>>> wrote:
>>>
>>>>
>>>> On Mon, Jun 23, 2014 at 2:51 PM, Alexander Kornienko <alexfh at google.com
>>>> > wrote:
>>>>
>>>>>
>>>>> On Thu, Jun 19, 2014 at 5:59 PM, Aaron Wishnick <
>>>>> aaron.s.wishnick at gmail.com> wrote:
>>>>>
>>>>>> When I run clang-tidy on OS X 10.9.3, I immediately get this output:
>>>>>>
>>>>>> clang-tidy(97903,0x7fff782fb310) malloc: *** error for object
>>>>>> 0x7fff5fbfecd0: pointer being freed was not allocated
>>>>>> *** set a breakpoint in malloc_error_break to debug
>>>>>>
>>>>>> This occurs inside the destructor of ClangTidyDiagnosticConsumer.
>>>>>> Here's my callstack:
>>>>>>
>>>>>> #4 0x000000010058e3e2 in ~ClangTidyDiagnosticConsumer at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h:190
>>>>>> #5 0x0000000100656a73 in
>>>>>> std::__1::default_delete<clang::DiagnosticConsumer>::operator()(clang::DiagnosticConsumer*)
>>>>>> const [inlined] at
>>>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2426
>>>>>> #6 0x0000000100656a4b in
>>>>>> std::__1::unique_ptr<clang::DiagnosticConsumer,
>>>>>> std::__1::default_delete<clang::DiagnosticConsumer>
>>>>>> >::reset(clang::DiagnosticConsumer*) [inlined] at
>>>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2625
>>>>>> #7 0x00000001006569f5 in ~unique_ptr [inlined] at
>>>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2593
>>>>>> #8 0x00000001006569f5 in ~unique_ptr [inlined] at
>>>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2593
>>>>>> #9 0x00000001006569f5 in ~ChainedDiagnosticConsumer at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23
>>>>>> #10 0x0000000100656595 in ~ChainedDiagnosticConsumer at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23
>>>>>> #11 0x00000001006565b9 in ~ChainedDiagnosticConsumer at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23
>>>>>> #12 0x00000001015eec84 in ~DiagnosticsEngine at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Basic/Diagnostic.cpp:68
>>>>>> #13 0x00000001015eec35 in ~DiagnosticsEngine at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Basic/Diagnostic.cpp:66
>>>>>> #14 0x00000001006bd3d3 in
>>>>>> llvm::RefCountedBase<clang::DiagnosticsEngine>::Release() const at
>>>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:55
>>>>>> #15 0x00000001006bd325 in
>>>>>> llvm::IntrusiveRefCntPtrInfo<clang::DiagnosticsEngine>::release(clang::DiagnosticsEngine*)
>>>>>> at /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:90
>>>>>> #16 0x00000001006bd2fd in
>>>>>> llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine>::release() at
>>>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:199
>>>>>> #17 0x00000001006bd2c5 in ~IntrusiveRefCntPtr at
>>>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:172
>>>>>> #18 0x00000001006bbe15 in ~IntrusiveRefCntPtr at
>>>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:172
>>>>>> #19 0x000000010065cbc1 in ~CompilerInstance at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:63
>>>>>> #20 0x000000010065c505 in ~CompilerInstance at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:61
>>>>>> #21 0x00000001005d6474 in
>>>>>> clang::tooling::FrontendActionFactory::runInvocation(clang::CompilerInvocation*,
>>>>>> clang::FileManager*, clang::DiagnosticConsumer*) at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:270
>>>>>> #22 0x00000001005d614f in
>>>>>> clang::tooling::ToolInvocation::runInvocation(char const*,
>>>>>> clang::driver::Compilation*, clang::CompilerInvocation*) at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:243
>>>>>> #23 0x00000001005d5290 in clang::tooling::ToolInvocation::run() at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:229
>>>>>> #24 0x00000001005d7b29 in
>>>>>> clang::tooling::ClangTool::run(clang::tooling::ToolAction*) at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:360
>>>>>> #25 0x0000000100566cd2 in
>>>>>> clang::tidy::runClangTidy(clang::tidy::ClangTidyOptionsProvider*,
>>>>>> clang::tooling::CompilationDatabase const&,
>>>>>> llvm::ArrayRef<std::__1::basic_string<char, std::__1::char_traits<char>,
>>>>>> std::__1::allocator<char> > >,
>>>>>> std::__1::vector<clang::tidy::ClangTidyError,
>>>>>> std::__1::allocator<clang::tidy::ClangTidyError> >*) at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/ClangTidy.cpp:345
>>>>>> #26 0x0000000100002a96 in main at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/tool/ClangTidyMain.cpp:145
>>>>>>
>>>>>> In short, it appears that ClangTool takes ownership of the diagnostic
>>>>>> consumer, but it's being allocated on the stack. My fix is to allocate it
>>>>>> on the heap instead. I've attached my patch. Please let me know if this
>>>>>> assessment is incorrect, or if you'd like me to go about this differently.
>>>>>>
>>>>>
>>>>> Well, the ownership of the diagnostic consumer shouldn't be
>>>>> transferred, and I don't see any evidence ClangTool::setDiagnosticConsumer
>>>>> expects this to happen. This all looks strange, and I'm investigating this.
>>>>>
>>>>
>>>> I wasn't able to reproduce this crash. Your stack trace has
>>>> ChainedDiagnosticConsumer in it, which afaiu, it is only used twice in
>>>> Clang, and both places don't seem to be unrelated to clang-tidy. Could you
>>>> set a breakpoint in ChainedDiagnosticConsumer constructor and send me the
>>>> stack trace where it gets called in clang-tidy? (or add an "assert(false);"
>>>> there to get the stack trace on the console in the assertions-enabled build)
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks!
>>>>>> Aaron
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits at cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>
>>> On Mon, Jun 23, 2014 at 9:51 AM, Alexander Kornienko <alexfh at google.com>
>>> wrote:
>>>
>>>>
>>>> On Mon, Jun 23, 2014 at 2:51 PM, Alexander Kornienko <alexfh at google.com
>>>> > wrote:
>>>>
>>>>>
>>>>> On Thu, Jun 19, 2014 at 5:59 PM, Aaron Wishnick <
>>>>> aaron.s.wishnick at gmail.com> wrote:
>>>>>
>>>>>> When I run clang-tidy on OS X 10.9.3, I immediately get this output:
>>>>>>
>>>>>> clang-tidy(97903,0x7fff782fb310) malloc: *** error for object
>>>>>> 0x7fff5fbfecd0: pointer being freed was not allocated
>>>>>> *** set a breakpoint in malloc_error_break to debug
>>>>>>
>>>>>> This occurs inside the destructor of ClangTidyDiagnosticConsumer.
>>>>>> Here's my callstack:
>>>>>>
>>>>>> #4 0x000000010058e3e2 in ~ClangTidyDiagnosticConsumer at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h:190
>>>>>> #5 0x0000000100656a73 in
>>>>>> std::__1::default_delete<clang::DiagnosticConsumer>::operator()(clang::DiagnosticConsumer*)
>>>>>> const [inlined] at
>>>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2426
>>>>>> #6 0x0000000100656a4b in
>>>>>> std::__1::unique_ptr<clang::DiagnosticConsumer,
>>>>>> std::__1::default_delete<clang::DiagnosticConsumer>
>>>>>> >::reset(clang::DiagnosticConsumer*) [inlined] at
>>>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2625
>>>>>> #7 0x00000001006569f5 in ~unique_ptr [inlined] at
>>>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2593
>>>>>> #8 0x00000001006569f5 in ~unique_ptr [inlined] at
>>>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2593
>>>>>> #9 0x00000001006569f5 in ~ChainedDiagnosticConsumer at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23
>>>>>> #10 0x0000000100656595 in ~ChainedDiagnosticConsumer at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23
>>>>>> #11 0x00000001006565b9 in ~ChainedDiagnosticConsumer at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23
>>>>>> #12 0x00000001015eec84 in ~DiagnosticsEngine at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Basic/Diagnostic.cpp:68
>>>>>> #13 0x00000001015eec35 in ~DiagnosticsEngine at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Basic/Diagnostic.cpp:66
>>>>>> #14 0x00000001006bd3d3 in
>>>>>> llvm::RefCountedBase<clang::DiagnosticsEngine>::Release() const at
>>>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:55
>>>>>> #15 0x00000001006bd325 in
>>>>>> llvm::IntrusiveRefCntPtrInfo<clang::DiagnosticsEngine>::release(clang::DiagnosticsEngine*)
>>>>>> at /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:90
>>>>>> #16 0x00000001006bd2fd in
>>>>>> llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine>::release() at
>>>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:199
>>>>>> #17 0x00000001006bd2c5 in ~IntrusiveRefCntPtr at
>>>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:172
>>>>>> #18 0x00000001006bbe15 in ~IntrusiveRefCntPtr at
>>>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:172
>>>>>> #19 0x000000010065cbc1 in ~CompilerInstance at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:63
>>>>>> #20 0x000000010065c505 in ~CompilerInstance at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:61
>>>>>> #21 0x00000001005d6474 in
>>>>>> clang::tooling::FrontendActionFactory::runInvocation(clang::CompilerInvocation*,
>>>>>> clang::FileManager*, clang::DiagnosticConsumer*) at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:270
>>>>>> #22 0x00000001005d614f in
>>>>>> clang::tooling::ToolInvocation::runInvocation(char const*,
>>>>>> clang::driver::Compilation*, clang::CompilerInvocation*) at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:243
>>>>>> #23 0x00000001005d5290 in clang::tooling::ToolInvocation::run() at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:229
>>>>>> #24 0x00000001005d7b29 in
>>>>>> clang::tooling::ClangTool::run(clang::tooling::ToolAction*) at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:360
>>>>>> #25 0x0000000100566cd2 in
>>>>>> clang::tidy::runClangTidy(clang::tidy::ClangTidyOptionsProvider*,
>>>>>> clang::tooling::CompilationDatabase const&,
>>>>>> llvm::ArrayRef<std::__1::basic_string<char, std::__1::char_traits<char>,
>>>>>> std::__1::allocator<char> > >,
>>>>>> std::__1::vector<clang::tidy::ClangTidyError,
>>>>>> std::__1::allocator<clang::tidy::ClangTidyError> >*) at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/ClangTidy.cpp:345
>>>>>> #26 0x0000000100002a96 in main at
>>>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/tool/ClangTidyMain.cpp:145
>>>>>>
>>>>>> In short, it appears that ClangTool takes ownership of the diagnostic
>>>>>> consumer, but it's being allocated on the stack. My fix is to allocate it
>>>>>> on the heap instead. I've attached my patch. Please let me know if this
>>>>>> assessment is incorrect, or if you'd like me to go about this differently.
>>>>>>
>>>>>
>>>>> Well, the ownership of the diagnostic consumer shouldn't be
>>>>> transferred, and I don't see any evidence ClangTool::setDiagnosticConsumer
>>>>> expects this to happen. This all looks strange, and I'm investigating this.
>>>>>
>>>>
>>>> I wasn't able to reproduce this crash. Your stack trace has
>>>> ChainedDiagnosticConsumer in it, which afaiu, it is only used twice in
>>>> Clang, and both places don't seem to be unrelated to clang-tidy. Could you
>>>> set a breakpoint in ChainedDiagnosticConsumer constructor and send me the
>>>> stack trace where it gets called in clang-tidy? (or add an "assert(false);"
>>>> there to get the stack trace on the console in the assertions-enabled build)
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks!
>>>>>> Aaron
>>>>>>
>>>>>>  _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141113/d47edf5d/attachment.html>

More information about the cfe-commits mailing list