Page MenuHomePhabricator

[analyzer] PR41729: Fix some false positives and improve strlcat and strlcpy modeling
Needs ReviewPublic

Authored by dkrupp on Sat, Aug 10, 1:08 AM.

Details

Summary

Fixes Bug 41729 (https://bugs.llvm.org/show_bug.cgi?id=41729)
and the following errors:

  • Fixes false positive reports of strlcat
  • The return value of strlcat and strlcpy is now correctly calculated
  • The resulting string length of strlcat and strlcpy is now correctly calculated

Diff Detail

Repository
rC Clang

Event Timeline

dkrupp created this revision.Sat, Aug 10, 1:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Aug 10, 1:08 AM
Szelethus retitled this revision from Fixes Bug 41729 and improves strlcat and strlcpy modeling to [analyzer] PR41729: Fix some false positives and improve strlcat and strlcpy modeling.Sun, Aug 11, 11:32 AM
Szelethus edited the summary of this revision. (Show Details)
NoQ added a comment.Mon, Aug 12, 4:28 PM

Oh, these false positives! Thanks!!

lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1627

Please handle the situation when freeSpaceNL is None before dereferencing. I suspect that this might actually happen when you overwhelm the symbol complexity by subtracting 1. I'm not really super sure this can happen, given that symbols we're working with are very specific as of now, but this is something that should be made possible if we ever go far enough in our attempts to improve this checker.

1637

Strange whitespace here and in the next comment (:

1647

Great job not introducing the state split here!

test/Analysis/cstring-syntax.c
57 ↗(On Diff #214512)

I think this test doesn't really demonstrate that bug 41729 is fixed, as the offending checker isn't enabled on this test.

dkrupp updated this revision to Diff 214817.Tue, Aug 13, 6:05 AM

Fix comments from @NoQ

dkrupp marked 3 inline comments as done.Tue, Aug 13, 6:06 AM

Thanks for the comments @NoQ , all of them addressed.

Nice improvements! It was a good catch that a substantial part of the checker could be cut out. Accurate modelling of CString and related API-s is really important, and this is a good step in that direction.
I would test this on a larger codebase as well before committing, but LGTM otherwise.

I agree with @gamesh411 here, would it be much trouble to see how this behaves on a bigger codebase?