Szelethus added a comment.
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
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:31
namespace {
+enum class appendKind { none = 0, strcat = 1, strlcat = 2 };
class CStringChecker : public Checker< eval::Call,
----------------
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.
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:133-134
void evalStrlcpy(CheckerContext &C, const CallExpr *CE) const;
- void evalStrcpyCommon(CheckerContext &C,
- const CallExpr *CE,
- bool returnEnd,
- bool isBounded,
- bool isAppending,
+ void evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, bool returnEnd,
+ bool isBounded, appendKind appendK,
bool returnPtr = true) const;
----------------
CaPitALizeE :D
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1556
+
+ // Get the string length of the destination
+ SVal dstStrLength = getCStringLength(C, state, Dst, DstVal);
----------------
Please terminate this sentence.
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1580
// If the function is strncpy, strncat, etc... it is bounded.
if (isBounded) {
----------------
Ah, okay, so the assumption is that bounded functions' third argument is always
a numerical size parameter. Why isn't that enforced at all?
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1596
- // Check if the max number to copy is less than the length of the src.
- // If the bound is equal to the source length, strncpy won't null-
- // terminate the result!
- std::tie(stateSourceTooLong, stateSourceNotTooLong) = state->assume(
- svalBuilder.evalBinOpNN(state, BO_GE, *strLengthNL, *lenValNL, cmpTy)
- .castAs<DefinedOrUnknownSVal>());
+ if (appendK == appendKind::none || appendK == appendKind::strcat) {
+ ProgramStateRef stateSourceTooLong, stateSourceNotTooLong;
----------------
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.
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1628
+ if (Optional<NonLoc> freeSpaceNL = freeSpace.getAs<NonLoc>()) {
+ // Symbolic expression not too complex.
+
----------------
Please put comments like these before the branch. I like the following format
better:
```lang=c++
// 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>())
```
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1630
+
+ SVal haveEnoughSpace = svalBuilder.evalBinOpNN(
+ state, BO_LE, *strLengthNL, *freeSpaceNL, cmpTy);
----------------
`hasEnoughSpace`
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1637-1640
+ if (TrueState && !FalseState) {
+ // srcStrLength <= size - dstStrLength -1
+ amountCopied = strLength;
+ }
----------------
All of these too, and omit braces too:
```lang=c++
// srcStrLength <= size - dstStrLength -1
if (TrueState && !FalseState)
amountCopied = strLength;
```
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1657
if (lenValNL) {
- if (isAppending) {
+ if (appendK == appendKind::strcat) {
// For strncat, the check is strlen(dst) + lenVal < sizeof(dst)
----------------
Would prefer a switch case here too.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66049/new/
https://reviews.llvm.org/D66049
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
