[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added inline comments.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:377-381
+        FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), ") + 1");
+    Diag << InsertFirstParenFix << InsertPlusOneAndSecondParenFix;
+  } else {
+    const auto InsertPlusOne =
+        FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), " + 1");
----------------
lebedev.ri wrote:
> The fixits are incorrect.
> Increment by `int(1)` can result in overflow, which is then extended to `size_t`
> It should be `+ 1UL`.
> https://godbolt.org/g/4nQiek
Could you show me a true example where it causes a problem? I think it is too rare to rewrite all code as you mentioned.


================
Comment at: test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c:158-180
+void bad_memset_1(char *dest, const char *src) {
+  int length = getLengthWithInc(src);
+  memset(dest, '-', length);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memset' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: memset(dest, '-', length - 1);
+}
+
----------------
lebedev.ri wrote:
> ??
> These look like false-negatives.
> How do you make an assumption about `dest` buffer from `src` string length?
My idea here is you do not want to set your null terminator to anything else, because then the result is not null-terminated.


https://reviews.llvm.org/D45050