Page MenuHomePhabricator

[analyzer] Fix crash caused by accessing empty map
AbandonedPublic

Authored by vabridgers on Dec 23 2020, 5:29 PM.

Details

Summary

This change attempts to address
https://bugs.llvm.org/show_bug.cgi?id=48588.

In certain cases, it appears that VA_ARGS is not in PrevParamMap
before it's accessed using at(). This change simply skips injectRange()
if VA_ARGS is not found in PrevParamMap.

The crash seen is described below, the case this was discovered in was
distilled into a reproducer inserted into the LIT tests for this module.

terminate called after throwing an instance of 'std::out_of_range'

what():  map::at

#0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int)
    <base>/llvm/lib/Support/Unix/Signals.inc:563:22

...

#11 std::__throw_out_of_range(char const*)

<base>/libstdc++-v3/src/c++11/functexcept.cc:82:5

#12 std::map<clang::IdentifierInfo const*,

  llvm::SmallVector<clang::Token, 2u>, std::less<clang::IdentifierInfo
  const*>, std::allocator<std::pair<clang::IdentifierInfo const* const,
  llvm::SmallVector<clang::Token, 2u> > > >::at(clang::IdentifierInfo
  const* const&) const
<base>/gcc/9.3.0/include/c++/9.3.0/bits/stl_map.h:549:10

#13 getMacroExpansionInfo((anonymous namespace)::MacroParamMap const&,

  clang::SourceLocation, clang::Preprocessor const&)
<base>/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1242:66

...

Diff Detail

Unit TestsFailed

TimeTest
60 msx64 debian > Clang.Analysis::plist-macros-with-expansion.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -std=c++14 -analyzer-checker=core /mnt/disks/ssd0/agent/llvm-project/clang/test/Analysis/plist-macros-with-expansion.cpp -analyzer-output=plist -o /mnt/disks/ssd0/agent/llvm-project/build/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist -analyzer-config expand-macros=true
360 msx64 windows > Clang.Analysis::plist-macros-with-expansion.cpp
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w16-1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -std=c++14 -analyzer-checker=core C:\ws\w16-1\llvm-project\premerge-checks\clang\test\Analysis\plist-macros-with-expansion.cpp -analyzer-output=plist -o C:\ws\w16-1\llvm-project\premerge-checks\build\tools\clang\test\Analysis\Output\plist-macros-with-expansion.cpp.tmp.plist -analyzer-config expand-macros=true

Event Timeline

vabridgers created this revision.Dec 23 2020, 5:29 PM
vabridgers requested review of this revision.Dec 23 2020, 5:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2020, 5:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steakhal requested changes to this revision.Dec 25 2020, 2:40 PM
steakhal added a reviewer: steakhal.

This form of macro expansion is obsolete - I hope that the community agrees on this.
Crashes for many more cases then just the one you mentioned. Its a really hard and bugrone to mimic the preprocessor, thus we seek to substitute this with the clang's preprocessor implementation.
I recommend pushing an XFAIL lit test demonstrating the current limitation (without your proposed fix), while we can show that this will pass after we accept my patches.
For the record those are in an early phase at D93222.

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
1241

Btw this lookup supposed to be successful. Always. Which suggests me that there are even more logic bug lurking there.
Without using 'at' here we wouldn't notice it, which is lucky.

This revision now requires changes to proceed.Dec 25 2020, 2:40 PM

Thanks @steakhal , I found your Bugzilla bug only after I submitted this patch. I'll revise based on your comments and resubmit. Best!

vabridgers added inline comments.Dec 26 2020, 1:59 PM
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
1241

If we keep the at(), maybe it's worthwhile adding an assert for the key present?

Abandoning this change request in favor of @steakhal 's more comprehensive change @ https://reviews.llvm.org/D93222

vabridgers abandoned this revision.Feb 3 2021, 6:01 AM

Abandoning.

Abandoning this change request in favor of @steakhal 's more comprehensive change @ https://reviews.llvm.org/D93222

Let me know if that patch-stack resolves your crash.
I don't mind extending the macro expansion test cases, it's a tricky area.