https://github.com/zeyi2 updated https://github.com/llvm/llvm-project/pull/198968
>From fe4bc0d4f0820a49be58cf8da8bcd94dbe4e61d0 Mon Sep 17 00:00:00 2001 From: Zeyi Xu <[email protected]> Date: Thu, 21 May 2026 13:26:13 +0800 Subject: [PATCH 1/2] [clang-tidy] Avoid strncmp FP in bugprone-not-null-terminated-result --- .../bugprone/NotNullTerminatedResultCheck.cpp | 58 +++++++++++++++---- clang-tools-extra/docs/ReleaseNotes.rst | 5 ++ .../bugprone/not-null-terminated-result.rst | 5 +- ...rminated-result-in-initialization-strlen.c | 16 ++--- .../not-null-terminated-result-strlen.c | 41 +++++++++---- .../not-null-terminated-result-wcslen.cpp | 29 ++++++---- 6 files changed, 108 insertions(+), 46 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp index 77b21d84529ce..0758e6c78e663 100644 --- a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp @@ -92,6 +92,40 @@ static unsigned getLength(const Expr *E, return 0; } +// Returns the number of characters that may be read from \p E when it is used +// as an argument of an 'ncmp' function. +static unsigned getNcmpLength(const Expr *E, + const MatchFinder::MatchResult &Result) { + if (!E) + return 0; + + E = E->IgnoreParenImpCasts(); + + // String literals including the null terminator. + // e.g. "foo" -> 4. + if (const auto *SL = dyn_cast<StringLiteral>(E)) + return SL->getLength() + 1; + + if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) + if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) { + // Character arrays are readable up to their full array bound. + // e.g. char buf[8] = "foo" -> 8. + if (const auto *CAT = + Result.Context->getAsConstantArrayType(VD->getType())) + if (CAT->getElementType()->isAnyCharacterType()) + return CAT->getSize().getZExtValue(); + + // Pointer variables initialized from literals inherit the literal size. + // e.g. const char *buf = "foo" -> 4. + if (const Expr *Init = VD->getInit()) + if (const auto *SL = + dyn_cast<StringLiteral>(Init->IgnoreParenImpCasts())) + return SL->getLength() + 1; + } + + return 0; +} + // Returns the capacity of the destination array. // For example in 'char dest[13]; memcpy(dest, ...)' it returns 13. static int getDestCapacity(const MatchFinder::MatchResult &Result) { @@ -957,25 +991,27 @@ void NotNullTerminatedResultCheck::strerrorSFix( void NotNullTerminatedResultCheck::ncmpFix( StringRef Name, const MatchFinder::MatchResult &Result) { const auto *FunctionExpr = Result.Nodes.getNodeAs<CallExpr>(FunctionExprName); - const Expr *FirstArgExpr = FunctionExpr->getArg(0)->IgnoreImpCasts(); - const Expr *SecondArgExpr = FunctionExpr->getArg(1)->IgnoreImpCasts(); + const Expr *FirstArgExpr = FunctionExpr->getArg(0)->IgnoreParenImpCasts(); + const Expr *SecondArgExpr = FunctionExpr->getArg(1)->IgnoreParenImpCasts(); + const int GivenLength = getGivenLength(Result); + const Expr *NcmpArgExpr = Result.Nodes.getNodeAs<Expr>(SrcExprName); bool IsLengthTooLong = false; if (const CallExpr *StrlenExpr = getStrlenExpr(Result)) { - const Expr *LengthExprArg = StrlenExpr->getArg(0); + const Expr *LengthExprArg = StrlenExpr->getArg(0)->IgnoreParenImpCasts(); const StringRef FirstExprStr = exprToStr(FirstArgExpr, Result).trim(); const StringRef SecondExprStr = exprToStr(SecondArgExpr, Result).trim(); const StringRef LengthArgStr = exprToStr(LengthExprArg, Result).trim(); - IsLengthTooLong = - LengthArgStr == FirstExprStr || LengthArgStr == SecondExprStr; - } else { - const int SrcLength = - getLength(Result.Nodes.getNodeAs<Expr>(SrcExprName), Result); - const int GivenLength = getGivenLength(Result); - if (SrcLength != 0 && GivenLength != 0) - IsLengthTooLong = GivenLength > SrcLength; + if (LengthArgStr == FirstExprStr) + NcmpArgExpr = SecondArgExpr; + else if (LengthArgStr == SecondExprStr) + NcmpArgExpr = FirstArgExpr; } + const int NcmpLength = getNcmpLength(NcmpArgExpr, Result); + if (NcmpLength != 0 && GivenLength != 0) + IsLengthTooLong = GivenLength > NcmpLength; + if (!IsLengthTooLong && !isStringDataAndLength(Result)) return; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 9f601babf7b27..15a37943326c6 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -383,6 +383,11 @@ Changes in existing checks <clang-tidy/checks/bugprone/narrowing-conversions>` check by fixing a false positive when converting a ``bool`` to a signed integer type. +- Improved :doc:`bugprone-not-null-terminated-result + <clang-tidy/checks/bugprone/not-null-terminated-result>` check by avoiding + false positives for ``strncmp`` and ``wcsncmp`` calls whose comparison length + does not exceed a known readable object size. + - Improved :doc:`bugprone-pointer-arithmetic-on-polymorphic-object <clang-tidy/checks/bugprone/pointer-arithmetic-on-polymorphic-object>` check by fixing a false positive when ``operator[]`` is used in a dependent context. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/not-null-terminated-result.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/not-null-terminated-result.rst index db86e94063ec0..0bc24f4ac16d9 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/not-null-terminated-result.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/not-null-terminated-result.rst @@ -118,8 +118,9 @@ String handler functions The given length is incremented by one. ``strncmp`` -If the third argument is the first or the second argument's ``length + 1`` -it has to be truncated without the ``+ 1`` operation. +If the third argument exceeds the known readable length of the first or second +argument, it has to be truncated to avoid reading past the end of that object. +For string literals, this length includes the null terminator. ``strxfrm`` The given length is incremented by one. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-in-initialization-strlen.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-in-initialization-strlen.c index 99d19ec953592..9aa7ef157b7b9 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-in-initialization-strlen.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-in-initialization-strlen.c @@ -36,31 +36,25 @@ void good_strerror_s(int errno) { strerror_s(dst, length + 1, errno); } -int bad_strncmp_1(char *str1, const char *str2) { +int good_strncmp_1(char *str1, const char *str2) { int length = strlen(str1) + 1; return strncmp(str1, str2, length); - // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] - // CHECK-FIXES: return strncmp(str1, str2, length - 1); } -int good_strncmp_1(char *str1, const char *str2) { +int good_strncmp_2(char *str1, const char *str2) { int length = strlen(str1) + 1; return strncmp(str1, str2, length - 1); } -int bad_strncmp_2(char *str2) { +int good_strncmp_3(char *str2) { return strncmp(str2, "foobar", (strlen("foobar") + 1)); - // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] - // CHECK-FIXES: return strncmp(str2, "foobar", (strlen("foobar"))); } -int bad_strncmp_3(char *str3) { +int good_strncmp_4(char *str3) { return strncmp(str3, "foobar", 1 + strlen("foobar")); - // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] - // CHECK-FIXES: return strncmp(str3, "foobar", strlen("foobar")); } -int good_strncmp_2_3(char *str) { +int good_strncmp_5(char *str) { return strncmp(str, "foobar", strlen("foobar")); } diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c index ca86986f4afce..00650b3731dde 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c @@ -62,32 +62,51 @@ void good_strerror_s(int errno) { strerror_s(dst, strlen(strerror(errno)) + 1, errno); } -int bad_strncmp_1(char *str0, const char *str1) { +int good_strncmp_1(char *str0, const char *str1) { return strncmp(str0, str1, (strlen(str0) + 1)); - // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] - // CHECK-FIXES: return strncmp(str0, str1, (strlen(str0))); } -int bad_strncmp_2(char *str2, const char *str3) { +int good_strncmp_2(char *str2, const char *str3) { return strncmp(str2, str3, 1 + strlen(str2)); - // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] - // CHECK-FIXES: return strncmp(str2, str3, strlen(str2)); } -int good_strncmp_1_2(char *str4, const char *str5) { +int good_strncmp_3(char *str4, const char *str5) { return strncmp(str4, str5, strlen(str4)); } -int bad_strncmp_3(char *str6) { +int good_strncmp_4(char *str6) { return strncmp(str6, "string", 7); - // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] - // CHECK-FIXES: return strncmp(str6, "string", 6); } -int good_strncmp_3(char *str7) { +int good_strncmp_5(char *str7) { return strncmp(str7, "string", 6); } +int good_strncmp_6(void) { + char blah[8] = "blah"; + return strncmp(blah, "1234567", 8); +} + +int bad_strncmp_1(char *str6) { + return strncmp(str6, "string", 8); + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: return strncmp(str6, "string", 7); +} + +int bad_strncmp_2(char *str8) { + const char *literal = "string"; + return strncmp(str8, literal, 8); + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: return strncmp(str8, literal, 7); +} + +int bad_strncmp_3(void) { + char buf[8] = "blah"; + return strncmp(buf, "1234567", 9); + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: return strncmp(buf, "1234567", 8); +} + void bad_strxfrm_1(const char *long_source_name) { char long_destination_array_name[13]; strxfrm(long_destination_array_name, long_source_name, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp index 688e41471d867..d16d5384c82d1 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp @@ -50,32 +50,39 @@ void good_wmemmove_s_1(wchar_t *dest, const wchar_t *src) { wmemmove_s(dest, 13, src, wcslen(src) + 1); } -int bad_wcsncmp_1(wchar_t *wcs0, const wchar_t *wcs1) { +int good_wcsncmp_1(wchar_t *wcs0, const wchar_t *wcs1) { return wcsncmp(wcs0, wcs1, (wcslen(wcs0) + 1)); - // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] - // CHECK-FIXES: return wcsncmp(wcs0, wcs1, (wcslen(wcs0))); } -int bad_wcsncmp_2(wchar_t *wcs2, const wchar_t *wcs3) { +int good_wcsncmp_2(wchar_t *wcs2, const wchar_t *wcs3) { return wcsncmp(wcs2, wcs3, 1 + wcslen(wcs2)); - // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] - // CHECK-FIXES: return wcsncmp(wcs2, wcs3, wcslen(wcs2)); } -int good_wcsncmp_1_2(wchar_t *wcs4, const wchar_t *wcs5) { +int good_wcsncmp_3(wchar_t *wcs4, const wchar_t *wcs5) { return wcsncmp(wcs4, wcs5, wcslen(wcs4)); } -int bad_wcsncmp_3(wchar_t *wcs6) { +int good_wcsncmp_4(wchar_t *wcs6) { return wcsncmp(wcs6, L"string", 7); - // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] - // CHECK-FIXES: return wcsncmp(wcs6, L"string", 6); } -int good_wcsncmp_3(wchar_t *wcs7) { +int good_wcsncmp_5(wchar_t *wcs7) { return wcsncmp(wcs7, L"string", 6); } +int bad_wcsncmp_1(wchar_t *wcs6) { + return wcsncmp(wcs6, L"string", 8); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: return wcsncmp(wcs6, L"string", 7); +} + +int bad_wcsncmp_2(wchar_t *wcs8) { + const wchar_t *literal = L"string"; + return wcsncmp(wcs8, literal, 8); + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: return wcsncmp(wcs8, literal, 7); +} + void bad_wcsxfrm_1(const wchar_t *long_source_name) { wchar_t long_destination_array_name[13]; wcsxfrm(long_destination_array_name, long_source_name, >From 61291708bc80339552d6b44c0a24f8fdbce48f47 Mon Sep 17 00:00:00 2001 From: Zeyi Xu <[email protected]> Date: Thu, 21 May 2026 13:52:25 +0800 Subject: [PATCH 2/2] add tests for \0... will anyone really write code like this? --- .../not-null-terminated-result-strlen.c | 18 +++++++++++++++--- .../not-null-terminated-result-wcslen.cpp | 16 ++++++++++++++-- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c index 00650b3731dde..b74a700416d71 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c @@ -87,26 +87,38 @@ int good_strncmp_6(void) { return strncmp(blah, "1234567", 8); } -int bad_strncmp_1(char *str6) { +int bad_strncmp_1(char *str9) { + // FIXME: This should warn. strncmp stops comparing at the embedded null. + return strncmp(str9, "foo\0bar", 8); +} + +int bad_strncmp_2(char *str6) { return strncmp(str6, "string", 8); // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] // CHECK-FIXES: return strncmp(str6, "string", 7); } -int bad_strncmp_2(char *str8) { +int bad_strncmp_3(char *str8) { const char *literal = "string"; return strncmp(str8, literal, 8); // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] // CHECK-FIXES: return strncmp(str8, literal, 7); } -int bad_strncmp_3(void) { +int bad_strncmp_4(void) { char buf[8] = "blah"; return strncmp(buf, "1234567", 9); // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] // CHECK-FIXES: return strncmp(buf, "1234567", 8); } +int bad_strncmp_5(char *str9) { + // FIXME: This should suggest 4. strncmp stops comparing at the embedded null. + return strncmp(str9, "foo\0bar", 9); + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: return strncmp(str9, "foo\0bar", 8); +} + void bad_strxfrm_1(const char *long_source_name) { char long_destination_array_name[13]; strxfrm(long_destination_array_name, long_source_name, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp index d16d5384c82d1..3533b80cc6c44 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp @@ -70,19 +70,31 @@ int good_wcsncmp_5(wchar_t *wcs7) { return wcsncmp(wcs7, L"string", 6); } -int bad_wcsncmp_1(wchar_t *wcs6) { +int bad_wcsncmp_1(wchar_t *wcs9) { + // FIXME: This should warn. wcsncmp stops comparing at the embedded null. + return wcsncmp(wcs9, L"foo\0bar", 8); +} + +int bad_wcsncmp_2(wchar_t *wcs6) { return wcsncmp(wcs6, L"string", 8); // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] // CHECK-FIXES: return wcsncmp(wcs6, L"string", 7); } -int bad_wcsncmp_2(wchar_t *wcs8) { +int bad_wcsncmp_3(wchar_t *wcs8) { const wchar_t *literal = L"string"; return wcsncmp(wcs8, literal, 8); // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] // CHECK-FIXES: return wcsncmp(wcs8, literal, 7); } +int bad_wcsncmp_4(wchar_t *wcs9) { + // FIXME: This should suggest 4. wcsncmp stops comparing at the embedded null. + return wcsncmp(wcs9, L"foo\0bar", 9); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: return wcsncmp(wcs9, L"foo\0bar", 8); +} + void bad_wcsxfrm_1(const wchar_t *long_source_name) { wchar_t long_destination_array_name[13]; wcsxfrm(long_destination_array_name, long_source_name, _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
