This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] PR41185: Fix regression where __builtin_* functions weren't recognized
ClosedPublic

Authored by Szelethus on Mar 26 2019, 4:03 AM.

Details

Summary

For the following code snippet:

void builtin_function_call_crash_fixes(char *c) {
  __builtin_strncpy(c, "", 6);
  __builtin_memset(c, '\0', (0));
  __builtin_memcpy(c, c, 0);
}

security.insecureAPI.DeprecatedOrUnsafeBufferHandling caused a regression, as it didn't recognize functions starting with __builtin_. Fixed exactly that.

I wanted to modify an existing test file, but the two I found didn't seem like perfect candidates. While I was there, I prettified their RUN: lines.

Diff Detail

Repository
rL LLVM

Event Timeline

koldaniel created this revision.Mar 26 2019, 4:03 AM
Szelethus requested changes to this revision.Mar 26 2019, 5:02 AM

Please add a testcase on which the checker crashed, but this patch resolves it.

Also, I think (in this particular case) that removing an assert is not a good idea. It is there for a reason, without it, we wouldn't have discovered this issue either. I think, as I mentioned in the inline, we should just strip `"__builtin_".

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
138–140 ↗(On Diff #192255)

Shouldn't we just do this instead?

155–162 ↗(On Diff #192255)

This guarantees that if we strip "__builtin_" within WalkAST::checkDeprecatedOrUnsafeBufferHandling, we won't run into an assertation failure.

This revision now requires changes to proceed.Mar 26 2019, 5:02 AM
alexfh added a subscriber: alexfh.Apr 3 2019, 10:32 AM

Thanks for addressing this! Please add the test cases from https://bugs.llvm.org/show_bug.cgi?id=41185

Artem, I'd appreciate, if you found time to finish this fix. The bug is causing most failures in our setup. Thanks!

Szelethus commandeered this revision.Apr 17 2019, 11:36 AM
Szelethus edited reviewers, added: koldaniel; removed: Szelethus.

Consider it done! In a little while.

Szelethus retitled this revision from [analyzer] Detect usages of unsafe I/O functions - Bug fixing to [analyzer] PR41185: Fix regression where __builtin_* functions weren't recognized.
Szelethus edited the summary of this revision. (Show Details)
Szelethus edited reviewers, added: baloghadamsoftware, alexfh; removed: zaks.anna.
Szelethus set the repository for this revision to rC Clang.
Szelethus edited projects, added Restricted Project; removed Restricted Project.

Implemented the fix I proposed in my earlier comment. While I was there, I prettified some of the other tests, and also made sure that the core package is enabled in their run lines.

Szelethus edited the summary of this revision. (Show Details)Apr 17 2019, 12:14 PM
Szelethus edited the summary of this revision. (Show Details)

Uhh, sorry for the spam. Realized that these are not path sensitive checkers. Removed the core packages from the invocations. Actual logic of the patch is untouched.

alexfh accepted this revision.Apr 17 2019, 12:21 PM

Awesome, thanks!
LG

This revision is now accepted and ready to land.Apr 17 2019, 12:21 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2019, 12:56 PM