Page MenuHomePhabricator

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

Authored by dkrupp on Aug 10 2019, 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

Event Timeline

dkrupp created this revision.Aug 10 2019, 1:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2019, 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.Aug 11 2019, 11:32 AM
Szelethus edited the summary of this revision. (Show Details)
NoQ added a comment.Aug 12 2019, 4:28 PM

Oh, these false positives! Thanks!!

lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1627 ↗(On Diff #214512)

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 ↗(On Diff #214512)

Strange whitespace here and in the next comment (:

1647 ↗(On Diff #214512)

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.Aug 13 2019, 6:05 AM

Fix comments from @NoQ

dkrupp marked 3 inline comments as done.Aug 13 2019, 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?

The logic of the patch is fine in my eyes, but I dislike the formatting. Could you please make the code obey the LLVM coding style, and after that use clang-format-diff.py?
https://llvm.org/docs/CodingStandards.html
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

lib/StaticAnalyzer/Checkers/CStringChecker.cpp
31 ↗(On Diff #214817)

Please capitalize these. Also, ConcatFnKind might be more descriptive? Ad absurdum, we might as well pass the CallDescription that is responsible for the concatenation. That maaay just be a little over engineered though.

133–134 ↗(On Diff #214817)

CaPitALizeE :D

1556 ↗(On Diff #214817)

Please terminate this sentence.

1580 ↗(On Diff #214817)

Ah, okay, so the assumption is that bounded functions' third argument is always a numerical size parameter. Why isn't that enforced at all?

1596 ↗(On Diff #214817)

Can we just do a switch-case? If someone were to add a new concat type, it would even give a warning in case it was left unhandled.

1628 ↗(On Diff #214817)

Please put comments like these before the branch. I like the following format better:

// While unlikely, it is possible that the subtraction is too complexity
// to complex to compute, let's check whether it succeeded.
if (Optional<NonLoc> freeSpaceNL = freeSpace.getAs<NonLoc>())
1630 ↗(On Diff #214817)

hasEnoughSpace

1637–1640 ↗(On Diff #214817)

All of these too, and omit braces too:

// srcStrLength <= size - dstStrLength -1
if (TrueState && !FalseState)
  amountCopied = strLength;
1657 ↗(On Diff #214817)

Would prefer a switch case here too.

dkrupp updated this revision to Diff 224088.Wed, Oct 9, 9:44 AM
dkrupp marked 9 inline comments as done.

@Szelethus thanks for your review.
I fixed your suggestions.

dkrupp updated this revision to Diff 224090.Wed, Oct 9, 10:01 AM

Fixing minor capitalization issue and removing an extra newline.

Szelethus accepted this revision.Thu, Oct 10, 7:07 AM

LGTM!

This requires a username/pw, but if there isn't any noticeable change on open source code, this change mustn't hurt much.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1652–1654

We usually omit braces when the branch consists of a single line.

This revision is now accepted and ready to land.Thu, Oct 10, 7:07 AM

Thanks for the reviews!
Could you pls commit this for me?