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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
