Page MenuHomePhabricator

[analyzer] InnerPointerChecker: Fix a segfault.
ClosedPublic

Authored by NoQ on Aug 28 2018, 2:59 PM.

Details

Summary

Return value of dyn_cast_or_null should be checked before use. Otherwise we may put a null pointer into the map as a key and eventually crash in checkDeadSymbols.

Reka: Why did we restrict ourselves to TypedValueRegions here? While we are mostly interested in local string variables and temporaries, which would of course be typed, i guess there's nothing that prevents us from checking that we don't delete or mutate a string in a SymbolicRegion somewhere between obtaining and using its inner pointer.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Aug 28 2018, 2:59 PM

Return value of dyn_cast_or_null should be checked before use. Otherwise we may put a null pointer into the map as a key and eventually crash in checkDeadSymbols.

Hm, so with the last CallDescription patch we removed some code here that essentially checked if the same region was null before this cast, which means two things: a) in the previous version it probably should have been a dyn_cast instead of dyn_cast_or_null, but now that makes it accidentally fine, and b) I should have thought about this when that code was removed.

Reka: Why did we restrict ourselves to TypedValueRegions here? While we are mostly interested in local string variables and temporaries, which would of course be typed, i guess there's nothing that prevents us from checking that we don't delete or mutate a string in a SymbolicRegion somewhere between obtaining and using its inner pointer.

I think the reason is that previously CallDescriptions didn't match fully qualified function names and the type was needed to see if the object was a string.

george.karpenkov accepted this revision.Aug 29 2018, 1:41 PM
This revision is now accepted and ready to land.Aug 29 2018, 1:41 PM
NoQ added a comment.Aug 30 2018, 11:42 AM

Ok, i'll get to the rest of the stuff a bit later (unless you pick it up).

This revision was automatically updated to reflect the committed changes.