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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D45050#1127449, @Charusso wrote:

> In https://reviews.llvm.org/D45050#1119974, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D45050#1119973, @Charusso wrote:
> >
> > > In https://reviews.llvm.org/D45050#1116446, @lebedev.ri wrote:
> > >
> > > > I would like to see more negative tests.
> > > >  What does it do if the len/size is a constant?
> > > >  Variable that wasn't just assigned with `strlen()` return?
> > >
> > >
> > > Thanks for the comment! What do you mean by negative test?
> >
> >
> > The tests where the check matches and issues a warning is a positive test.
> >  The tests where the check does not match and/or does not issue a warning is a negative test.
>
>
> @lebedev.ri there is all the false positive results from the last publicated result-dump:
>
> 1. F6334659: curl-lib-curl_path-c.html <https://reviews.llvm.org/F6334659>
> 2. second result: F6334660: ffmpeg-libavformat-sdp.c.html <https://reviews.llvm.org/F6334660>
> 3. all result: F6334663: openssl-apps-ca-c.html <https://reviews.llvm.org/F6334663>
> 4. F6334665: postgresql-src-interfaces-ecpg-preproc-pgc-c.html <https://reviews.llvm.org/F6334665>
> 5. F6334667: redis-src-redis-benchmark-c.html <https://reviews.llvm.org/F6334667>
> 6. may false positive: F6334693: ffmpeg-libavformat-rdt-c.html <https://reviews.llvm.org/F6334693>
>
>   (Note: the two `memchr()` result were false positive in that post and there is no new result with the new matcher.)
>
>   Does the new test cases cover your advice?


Not exactly.
I want to see **tests**.
I.e. they should be in `test/clang-tidy/bugprone-not-null-terminated-result-*`.
E.g. i did not find the following tests:

  void test1(char* dst, char* src) {
    strcpy(dst, src, 10);
  }
  void test2(char* dst, char* src, int len) {
    strcpy(dst, src, len);
  }
  void test3(char* dst, char* src) {
    memcpy(dst, src, 10);
  }
  void test4(char* dst, char* src, int len) {
    memcpy(dst, src, len);
  }
  ...



================
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");
----------------
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


================
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);
+}
+
----------------
??
These look like false-negatives.
How do you make an assumption about `dest` buffer from `src` string length?


https://reviews.llvm.org/D45050