Page MenuHomePhabricator

[analyzer] Fix crash caused by accessing empty map

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



This change attempts to address

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)


#11 std::__throw_out_of_range(char const*)


#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

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

  clang::SourceLocation, clang::Preprocessor const&)


Diff Detail

Unit TestsFailed

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.


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

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 @

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


Abandoning this change request in favor of @steakhal 's more comprehensive change @

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.