This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] alpha.unix.cstring.OutOfBounds checker enable/disable fix
ClosedPublic

Authored by dkrupp on Jul 2 2018, 7:56 AM.

Details

Summary

It was not possible to disable alpha.unix.cstring.OutOfBounds checker's reports
since unix.Malloc checker always implicitly enabled the filter.
Moreover if the checker was disabled from command line (-analyzer-disable-checker ...)
the out of bounds warnings were nevertheless emitted under different
checker names such as unix.cstring.NullArg, or unix.Malloc.

This patch fixes the case sot that Malloc checker only enables implicitly the underlying
modeling of strcpy, memcpy etc. but not the warning messages that would have been
emmitted by alpha.unix.cstring.OutOfBounds

Diff Detail

Event Timeline

dkrupp created this revision.Jul 2 2018, 7:56 AM
dkrupp set the repository for this revision to rC Clang.
dkrupp removed a project: Restricted Project.
NoQ added a comment.Jul 2 2018, 4:38 PM

Uhm, so we had an alpha checker enabled all along? Thanks for patching this up!

lib/StaticAnalyzer/Checkers/CStringChecker.cpp
307–308

Accidental double space.

310

Could we preserve the other portion of the assertion on this branch? I.e., assert(Filter.CheckCStringNullArg).

Additionally, do you really want to continue analysis on this path? Maybe return nullptr to sink?

dkrupp updated this revision to Diff 153905.Jul 3 2018, 6:21 AM

The patch has been updated.
Changes:

-The analysis path is cut if overvlow is detected even if CStringOutOfBounds is disabled

The assert(Filter.CheckCStringOutOfBounds || Filter.CheckCStringNullArg); cannot be put back, because if both checkers are disabled, the assertion is not true.

dkrupp marked 2 inline comments as done.Jul 3 2018, 6:30 AM
dkrupp added inline comments.
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
310

I was unsure whether to return nullptr or StOutBound. I thought that alpha.unix.cstring.OutOfBounds is in alpha because it may falsely detect buffer overflows and then we would cut the path unnecessarily.
But OK, it is safer this way.

I could not put back the assertion, because if only unix.Malloc checker is enabled (and CStringOutOfBounds and CStringNullArg are not) the assertion is not true.

LGTM! But wait for Artem's acceptance before submitting.

lib/StaticAnalyzer/Checkers/CStringChecker.cpp
310

I think the assertion could be preserved after the early exit, but it has no point to repeat the same condition in subsequent lines.

This revision is now accepted and ready to land.Jul 5 2018, 11:21 PM
dkrupp marked an inline comment as done.Jul 13 2018, 6:04 AM

@NoQ do we need any more update to this patch? Thanks.

NoQ accepted this revision.Jul 13 2018, 6:31 AM

Whoops, sry, yeah, looks good, please commit!

This revision was automatically updated to reflect the committed changes.
baloghadamsoftware retitled this revision from alpha.unix.cstring.OutOfBounds checker enable/disable fix to [Analyzer] alpha.unix.cstring.OutOfBounds checker enable/disable fix.Jul 13 2018, 6:51 AM