[PATCH] Fix clang-tidy delete of stack object
Aaron Wishnick
aaron.s.wishnick at gmail.com
Wed Nov 12 12:14:05 PST 2014
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/20141112/cef1b020/attachment.html>
More information about the cfe-commits
mailing list