This is an archive of the discontinued LLVM Phabricator instance.

Differential D93787

[analyzer] Fix crash caused by accessing empty map
AbandonedPublic

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

Details

Reviewers
Szelethus
steakhal
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
360 msx64 windows > Clang.Analysis::plist-macros-with-expansion.cpp

Event Timeline

vabridgers created this revision.Dec 23 2020, 5:29 PM
Herald added subscribers: steakhal, ASDenysPetrov, martong and 9 others. · View Herald TranscriptDec 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
Harbormaster completed remote builds in B83441: Diff 313632.Dec 23 2020, 6:38 PM
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
vabridgers added a comment.Dec 26 2020, 1:58 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?

vabridgers added a comment.Feb 3 2021, 5:59 AM

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.

steakhal added a comment.Feb 3 2021, 8:08 AM
In D93787#2539111, @vabridgers wrote:

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.

Revision Contents

PathSize
clang/
lib/
StaticAnalyzer/
Core/
3 lines
test/
Analysis/
Inputs/
expected-plists/
136 lines
20 lines

Diff 313632

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

Show First 20 Lines • Show All 1,232 Lines • ▼ Show 20 Lines if (ParenthesesDepth != 0) {
// (void)(10 / x); // (void)(10 / x);
// } // }
// //
// We will stumble across this while trying to expand // We will stumble across this while trying to expand
// PARAMS_RESOLVE_TO_VA_ARGS. By this point, we already noted during // PARAMS_RESOLVE_TO_VA_ARGS. By this point, we already noted during
// the processing of DISPATCH what __VA_ARGS__ maps to, so we'll // the processing of DISPATCH what __VA_ARGS__ maps to, so we'll
// retrieve the next series of tokens from that. // retrieve the next series of tokens from that.
if (TheTok.getIdentifierInfo() == VariadicParamII) { if (TheTok.getIdentifierInfo() == VariadicParamII) {
if (PrevParamMap.find(VariadicParamII) != PrevParamMap.end())
steakhalUnsubmitted
Not Done
ReplyInline Actions

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.

steakhal: Btw this lookup supposed to be successful. Always. Which suggests me that there are even more…
vabridgersAuthorUnsubmitted
Done
ReplyInline Actions

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

vabridgers: If we keep the at(), maybe it's worthwhile adding an assert for the key present?
TStream.injectRange(PrevParamMap.at(VariadicParamII)); TStream.injectRange(PrevParamMap.at(VariadicParamII));
TStream.next(TheTok); TStream.next(TheTok);
continue; continue;
} }
} }
ArgTokens.push_back(TheTok); ArgTokens.push_back(TheTok);
TStream.next(TheTok); TStream.next(TheTok);
} }
▲ Show 20 Lines • Show All 135 Lines • Show Last 20 Lines

clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist

Show First 20 Lines • Show All 6,917 Lines • ▼ Show 20 Lines <array>
<integer>536</integer> <integer>536</integer>
<integer>537</integer> <integer>537</integer>
<integer>538</integer> <integer>538</integer>
<integer>539</integer> <integer>539</integer>
<integer>540</integer> <integer>540</integer>
</array> </array>
</dict> </dict>
</dict> </dict>
<dict>
<key>path</key>
<array>
<dict>
<key>kind</key><string>event</string>
<key>location</key>
<dict>
<key>line</key><integer>560</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<key>ranges</key>
<array>
<array>
<dict>
<key>line</key><integer>560</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
<key>line</key><integer>560</integer>
<key>col</key><integer>14</integer>
<key>file</key><integer>0</integer>
</dict>
</array>
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
<string>&apos;localvar&apos; declared without an initial value</string>
<key>message</key>
<string>&apos;localvar&apos; declared without an initial value</string>
</dict>
<dict>
<key>kind</key><string>control</string>
<key>edges</key>
<array>
<dict>
<key>start</key>
<array>
<dict>
<key>line</key><integer>560</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
<key>line</key><integer>560</integer>
<key>col</key><integer>5</integer>
<key>file</key><integer>0</integer>
</dict>
</array>
<key>end</key>
<array>
<dict>
<key>line</key><integer>561</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
<key>line</key><integer>561</integer>
<key>col</key><integer>15</integer>
<key>file</key><integer>0</integer>
</dict>
</array>
</dict>
</array>
</dict>
<dict>
<key>kind</key><string>event</string>
<key>location</key>
<dict>
<key>line</key><integer>561</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<key>ranges</key>
<array>
<array>
<dict>
<key>line</key><integer>561</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
<key>line</key><integer>564</integer>
<key>col</key><integer>12</integer>
<key>file</key><integer>0</integer>
</dict>
</array>
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
<string>4th function call argument is an uninitialized value</string>
<key>message</key>
<string>4th function call argument is an uninitialized value</string>
</dict>
</array>
<key>macro_expansions</key>
<array>
<dict>
<key>location</key>
<dict>
<key>line</key><integer>561</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<key>name</key><string>TRACE_WRAPPER</string>
<key>expansion</key><string>{ trace((0), traceid(&quot;formatstr &quot; str), 0, tracelevel,); }</string>
</dict>
</array>
<key>description</key><string>4th function call argument is an uninitialized value</string>
<key>category</key><string>Logic error</string>
<key>type</key><string>Uninitialized argument value</string>
<key>check_name</key><string>core.CallAndMessage</string>
<!-- This hash is experimental and going to change! -->
<key>issue_hash_content_of_line_in_context</key><string>7b4de363511977d731e5dfab4fd89c66</string>
<key>issue_context_kind</key><string>function</string>
<key>issue_context</key><string>funcXXX</string>
<key>issue_hash_function_offset</key><string>2</string>
<key>location</key>
<dict>
<key>line</key><integer>561</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<key>ExecutedLines</key>
<dict>
<key>0</key>
<array>
<integer>557</integer>
<integer>558</integer>
<integer>559</integer>
<integer>560</integer>
<integer>561</integer>
</array>
</dict>
</dict>
</array> </array>
<key>files</key> <key>files</key>
<array> <array>
</array> </array>
</dict> </dict>
</plist> </plist>

clang/test/Analysis/plist-macros-with-expansion.cpp

Show First 20 Lines • Show All 537 Lines • ▼ Show 20 Linesint bz44493(void) {
BZ44493_GNUVA(a); BZ44493_GNUVA(a);
BZ44493_GNUVA(a, "arg2"); BZ44493_GNUVA(a, "arg2");
(void)(10 / a); // expected-warning{{Division by zero}} (void)(10 / a); // expected-warning{{Division by zero}}
return 0; return 0;
} }
// CHECK: <key>name</key><string>BZ44493_GNUVA</string> // CHECK: <key>name</key><string>BZ44493_GNUVA</string>
// CHECK-NEXT: <key>expansion</key><string>--(a);</string> // CHECK-NEXT: <key>expansion</key><string>--(a);</string>
// Expected warning, and don't crash
const char *traceid(const char *);
int trace(int, const char *, int, ...);
#define TRACE_CALL(tracelevel, ...) \
{ __VA_ARGS__; }
#define TRACE(tracelevel, str, ...) \
TRACE_CALL((tracelevel), trace((0), traceid("formatstr " str), 0, tracelevel, __VA_ARGS__))
#define TRACE_WRAPPER TRACE
void funcXXX(
void *Context_p) {
int localvar;
TRACE_WRAPPER(
localvar,
"localvar=%u ",
0); // expected-warning{{4th function call argument is an uninitialized value}}
}