Page MenuHomePhabricator

[analyzer] do not crash when trying to convert an APSInt to an unexpected type
ClosedPublic

Authored by george.karpenkov on Nov 9 2017, 12:23 PM.

Details

Summary

This is the issue breaking the postgresql bot, purely by chance exposed through taint checker, somehow appearing after https://reviews.llvm.org/D38358 got committed.

The backstory is that the taint checker requests SVal for the value of the pointer, and analyzer has a "fast path" in the getter to return a constant when we know that the value is constant.
Unfortunately, the getter requires a cast to get signedness correctly, and for the pointer void * the cast crashes.

This is more of a band-aid patch, as I am not sure what could be done here "correctly", but it should be applied in any case to avoid the crash.

@dcoughlin faster review here would be appreciated to get the bot back to green.

Diff Detail

Repository
rL LLVM

Event Timeline

dcoughlin accepted this revision.Nov 9 2017, 1:19 PM

This seems reasonable to me. Please commit it. @NoQ can do a post-commit review and fix it up if he would rather address the issue differently.

This revision is now accepted and ready to land.Nov 9 2017, 1:19 PM
NoQ edited edge metadata.Nov 9 2017, 1:20 PM

I'm curious if the crash would turn into an assertion failure during getRawSVal() after D38801 is committed.

The problem with this checker producing void symbols is known since D26837. If this problem is fixed on the checker side (it probably should be - the checker , we can probably put stronger asserts onto types suitable for symbols.

lib/StaticAnalyzer/Core/ProgramState.cpp
265 ↗(On Diff #122306)

If a type is an integral or enumeration type or a Loc type, then it is definitely not null.

I'm curious if the crash would turn into an assertion failure during getRawSVal() after D38801 is committed.

I guess we would see?

If this problem is fixed on the checker side (it probably should be - the checker , we can probably put stronger asserts onto types suitable for symbols.

Of course I'm new, but I disagree with this statement: in order to have a robust API, the function should not crash, unless the caller violates an explicit precondition.
getSVal is just a function for getting a symbolic value for a particular statement, it seems totally valid to query it for an expression which type is void.

lib/StaticAnalyzer/Core/ProgramState.cpp
265 ↗(On Diff #122306)

But the check itself will crash if the type is null.

This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Nov 9 2017, 11:29 PM

Of course I'm new, but I disagree with this statement: in order to have a robust API, the function should not crash, unless the caller violates an explicit precondition.
getSVal is just a function for getting a symbolic value for a particular statement, it seems totally valid to query it for an expression which type is void.

Not for an expression - this overload retrieves a value stored in the memory region. I think that we should inform the checker's author when he tries to interpret the value in memory as a void value, because i believe that in pretty much all cases he has a better type to provide.

lib/StaticAnalyzer/Core/ProgramState.cpp
265 ↗(On Diff #122306)

Whoops right sry.