Page MenuHomePhabricator

[analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new.
ClosedPublic

Authored by NoQ on Feb 22 2018, 4:50 PM.

Details

Summary

This is assertion removal that i find valid. With placement new (which isn't even need to be inlined, we used to model it conservatively well enough), anything of any type can have any dynamic type. Even if we have a concrete region of a variable, its dynamic type and its static type can be completely unrelated. This might be UB due to strict aliasing rules, but we shouldn't crash. This patch introduces a relatively sane behavior in this scenario that consists of evalCast()ing this-region to the assumed dynamic type during virtual calls.

In the common case when the dynamic type is a sub-class of the static type, this is worse than calling attemptDownCast() because it adds element region instead of removing base regions (which is not incorrect but produces a non-canonical representation of the SVal). But when the common-case approach is known to have failed, there doesn't seem to be a better option.

A lot of these crashes have suddenly shown up when i was testing temporaries. They have nothing to do with temporaries though, but with a weird implementation detail of std::function that suddenly got some if its methods inlined.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Feb 22 2018, 4:50 PM
george.karpenkov accepted this revision.Feb 22 2018, 5:04 PM

LGTM, though in general I still think we should do something on soft-failure-modes in order to aid future debugging...

Is the failure mode in this case always a UB? In this case, could we emit a warning? If not from CallEvent, then from where?

lib/StaticAnalyzer/Core/CallEvent.cpp
593 ↗(On Diff #135563)

nit: auto?

This revision is now accepted and ready to land.Feb 22 2018, 5:04 PM
NoQ updated this revision to Diff 135572.Feb 22 2018, 5:22 PM
  • Add a definitely-not-UB example (char buffers are special).
  • Bring back an accidentally deleted blank line.
NoQ added inline comments.Feb 22 2018, 5:24 PM
lib/StaticAnalyzer/Core/CallEvent.cpp
593 ↗(On Diff #135563)

Dunno, i somehow like the symmetry with the next line. "Take method decl, take record decl". Maybe it's just me.

NoQ added a comment.Feb 22 2018, 5:27 PM

In this case, could we emit a warning? If not from CallEvent, then from where?

(1) This might result in a buffer overflow, so i home that alpha.ArrayBound may eventually catch it.
(2) It might be a good idea to make a fairly generic checker for the strict aliasing rule.

This revision was automatically updated to reflect the committed changes.