Page MenuHomePhabricator

[analyzer] Handle comparison between non-default AS symbol and constant

Authored by dstenb on Feb 26 2019, 1:22 AM.



When comparing a symbolic region and a constant, the constant would be
widened or truncated to the width of a void pointer, meaning that the
constant could be incorrectly truncated when handling symbols for
non-default address spaces. In the attached test case this resulted in a
false positive since the constant was truncated to zero. To fix this,
widen/truncate the constant to the width of the symbol expression's

This commit does not consider non-symbolic regions as I'm not sure how
to generalize getting the type there.

This fixes PR40814.

Diff Detail

rC Clang

Event Timeline

dstenb created this revision.Feb 26 2019, 1:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2019, 1:22 AM
NoQ added a comment.Feb 26 2019, 10:59 AM

Hi, thanks!, i think this is correct. As in, LocAsInteger was clearly a mistake to begin with, but this change shouldn't make it worse.

You should be able to get away with not supporting comparisons between regions without symbolic base [as integers] and concrete integers because they usually yield fairly dumb values anyway. Eg., it's impossible for an address of a variable to be equal to a null pointer, and whether a variable's address is currently equal to 0xbadf00d is undefined anyway - it will cause problems when we try to symbolicate our undefined values, but for now we prefer to interrupt the analysis whenever they actually end up being used (which causes the problem to disappear because symbolication only matters when we use the symbol more than once).


Pls be more specific in this comment that we're making this extra check in order to support non-default address spaces. Eg., "If the region has a symbolic base, pay attention to the type; it might be coming from a non-default address space. For non-symbolic regions it doesn't matter that much because such comparisons would most likely evaluate to concrete false anyway. FIXME: We might still need to handle the non-comparison case."


Pls add a // no-warning here, so that it was clear what the test is about.

dstenb updated this revision to Diff 188538.Feb 27 2019, 7:53 AM

Address comments.

dstenb marked 2 inline comments as done.Feb 27 2019, 7:53 AM
NoQ accepted this revision.Mar 6 2019, 7:04 PM

Thanks again! Please commit.

This revision is now accepted and ready to land.Mar 6 2019, 7:04 PM

Thanks for the review! I'll submit this shortly then.

This revision was automatically updated to reflect the committed changes.