[clang][analyzer] Bring checker 'alpha.unix.cstring.NotNullTerminated' out of alpha #113899
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) ChangesI have tested it on the usual amount of C and C++ projects, no results were found. This checker is only for very rare cases, I do not know if it is really useful but it is there and can be moved out of alpha. There is no test when only this checker is enabled (and not full unix.cstring), but I have checked and it works too. Full diff: https://github.com/llvm/llvm-project/pull/113899.diff 7 Files Affected:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 58dbd686a6dc9f..15c08081e174d4 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1901,6 +1901,29 @@ Check the size argument passed into C string functions for common erroneous patt
.. _unix-cstring-NullArg:
+.. _alpha-unix-cstring-NotNullTerminated:
+
+unix.cstring.NotNullTerminated (C)
+""""""""""""""""""""""""""""""""""
+Check for arguments which are not null-terminated strings;
+applies to the ``strlen``, ``strcpy``, ``strcat``, ``strcmp`` family of functions.
+
+Only very fundamental cases are detected where the passed memory block is
+absolutely different from a null-terminated string. This checker does not
+find if a memory buffer is passed where the terminating zero character
+is missing.
+
+.. code-block:: c
+
+ void test1() {
+ int l = strlen((char *)&test); // warn
+ }
+
+ void test2() {
+ label:
+ int l = strlen((char *)&&label); // warn
+ }
+
unix.cstring.NullArg (C)
""""""""""""""""""""""""
Check for null pointers being passed as arguments to C string functions:
@@ -3367,29 +3390,6 @@ Checks for overlap in two buffer arguments. Applies to: ``memcpy, mempcpy, wmem
memcpy(a + 2, a + 1, 8); // warn
}
-.. _alpha-unix-cstring-NotNullTerminated:
-
-alpha.unix.cstring.NotNullTerminated (C)
-""""""""""""""""""""""""""""""""""""""""
-Check for arguments which are not null-terminated strings;
-applies to the ``strlen``, ``strcpy``, ``strcat``, ``strcmp`` family of functions.
-
-Only very fundamental cases are detected where the passed memory block is
-absolutely different from a null-terminated string. This checker does not
-find if a memory buffer is passed where the terminating zero character
-is missing.
-
-.. code-block:: c
-
- void test1() {
- int l = strlen((char *)&test); // warn
- }
-
- void test2() {
- label:
- int l = strlen((char *)&&label); // warn
- }
-
.. _alpha-unix-cstring-OutOfBounds:
alpha.unix.cstring.OutOfBounds (C)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 349040c15eeb83..7ce2b26a27dd27 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -459,6 +459,12 @@ def CStringModeling : Checker<"CStringModeling">,
Documentation<NotDocumented>,
Hidden;
+def CStringNotNullTerm : Checker<"NotNullTerminated">,
+ HelpText<"Check for arguments passed to C string functions which are not "
+ "null-terminated strings">,
+ Dependencies<[CStringModeling]>,
+ Documentation<HasDocumentation>;
+
def CStringNullArg : Checker<"NullArg">,
HelpText<"Check for null pointers being passed as arguments to C string "
"functions">,
@@ -485,11 +491,6 @@ def CStringBufferOverlap : Checker<"BufferOverlap">,
Dependencies<[CStringModeling]>,
Documentation<HasDocumentation>;
-def CStringNotNullTerm : Checker<"NotNullTerminated">,
- HelpText<"Check for arguments which are not null-terminating strings">,
- Dependencies<[CStringModeling]>,
- Documentation<HasDocumentation>;
-
def CStringUninitializedRead : Checker<"UninitializedRead">,
HelpText<"Checks if the string manipulation function would read uninitialized bytes">,
Dependencies<[CStringModeling]>,
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index e605c62a66ad0e..a84a0c2211fde0 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -53,6 +53,7 @@
// CHECK-NEXT: unix.StdCLibraryFunctions
// CHECK-NEXT: unix.Vfork
// CHECK-NEXT: unix.cstring.BadSizeArg
+// CHECK-NEXT: unix.cstring.NotNullTerminated
// CHECK-NEXT: unix.cstring.NullArg
int main() {
diff --git a/clang/test/Analysis/bstring.cpp b/clang/test/Analysis/bstring.cpp
index 1b6397c3455ebd..9c30bef15d407a 100644
--- a/clang/test/Analysis/bstring.cpp
+++ b/clang/test/Analysis/bstring.cpp
@@ -2,7 +2,7 @@
// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,unix.Malloc,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,unix.Malloc,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,alpha.unix.cstring.NotNullTerminated,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,unix.cstring.NotNullTerminated,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
#include "Inputs/system-header-simulator-cxx.h"
#include "Inputs/system-header-simulator-for-malloc.h"
diff --git a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
index 345a4e8f44efd1..3d1d3c561a5580 100644
--- a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -61,6 +61,7 @@
// CHECK-NEXT: unix.StdCLibraryFunctions
// CHECK-NEXT: unix.Vfork
// CHECK-NEXT: unix.cstring.BadSizeArg
+// CHECK-NEXT: unix.cstring.NotNullTerminated
// CHECK-NEXT: unix.cstring.NullArg
int main() {
diff --git a/clang/test/Analysis/string.c b/clang/test/Analysis/string.c
index 2e0a49d083b0b0..e017aff3b4a1db 100644
--- a/clang/test/Analysis/string.c
+++ b/clang/test/Analysis/string.c
@@ -38,7 +38,7 @@
// RUN: -analyzer-checker=unix.cstring \
// RUN: -analyzer-checker=unix.Malloc \
// RUN: -analyzer-checker=alpha.unix.cstring.BufferOverlap \
-// RUN: -analyzer-checker=alpha.unix.cstring.NotNullTerminated \
+// RUN: -analyzer-checker=unix.cstring.NotNullTerminated \
// RUN: -analyzer-checker=debug.ExprInspection \
// RUN: -analyzer-config eagerly-assume=false
diff --git a/clang/test/Analysis/string.cpp b/clang/test/Analysis/string.cpp
index c09422d1922369..e6cc950f30c9a0 100644
--- a/clang/test/Analysis/string.cpp
+++ b/clang/test/Analysis/string.cpp
@@ -53,3 +53,7 @@ struct TestNotNullTerm {
strlen((char *)&x); // expected-warning{{Argument to string length function is not a null-terminated string}}
}
};
+
+void test_notcstring_tempobject() {
+ strlen((char[]){'a', 0}); // expected-warning{{Argument to string length function is a C++ temp object of type char[2], which is not a null-terminated string}}
+}
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Kéri (balazske) ChangesI have tested it on the usual amount of C and C++ projects, no results were found. This checker is only for very rare cases, I do not know if it is really useful but it is there and can be moved out of alpha. There is no test when only this checker is enabled (and not full unix.cstring), but I have checked and it works too. Full diff: https://github.com/llvm/llvm-project/pull/113899.diff 7 Files Affected:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 58dbd686a6dc9f..15c08081e174d4 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1901,6 +1901,29 @@ Check the size argument passed into C string functions for common erroneous patt
.. _unix-cstring-NullArg:
+.. _alpha-unix-cstring-NotNullTerminated:
+
+unix.cstring.NotNullTerminated (C)
+""""""""""""""""""""""""""""""""""
+Check for arguments which are not null-terminated strings;
+applies to the ``strlen``, ``strcpy``, ``strcat``, ``strcmp`` family of functions.
+
+Only very fundamental cases are detected where the passed memory block is
+absolutely different from a null-terminated string. This checker does not
+find if a memory buffer is passed where the terminating zero character
+is missing.
+
+.. code-block:: c
+
+ void test1() {
+ int l = strlen((char *)&test); // warn
+ }
+
+ void test2() {
+ label:
+ int l = strlen((char *)&&label); // warn
+ }
+
unix.cstring.NullArg (C)
""""""""""""""""""""""""
Check for null pointers being passed as arguments to C string functions:
@@ -3367,29 +3390,6 @@ Checks for overlap in two buffer arguments. Applies to: ``memcpy, mempcpy, wmem
memcpy(a + 2, a + 1, 8); // warn
}
-.. _alpha-unix-cstring-NotNullTerminated:
-
-alpha.unix.cstring.NotNullTerminated (C)
-""""""""""""""""""""""""""""""""""""""""
-Check for arguments which are not null-terminated strings;
-applies to the ``strlen``, ``strcpy``, ``strcat``, ``strcmp`` family of functions.
-
-Only very fundamental cases are detected where the passed memory block is
-absolutely different from a null-terminated string. This checker does not
-find if a memory buffer is passed where the terminating zero character
-is missing.
-
-.. code-block:: c
-
- void test1() {
- int l = strlen((char *)&test); // warn
- }
-
- void test2() {
- label:
- int l = strlen((char *)&&label); // warn
- }
-
.. _alpha-unix-cstring-OutOfBounds:
alpha.unix.cstring.OutOfBounds (C)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 349040c15eeb83..7ce2b26a27dd27 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -459,6 +459,12 @@ def CStringModeling : Checker<"CStringModeling">,
Documentation<NotDocumented>,
Hidden;
+def CStringNotNullTerm : Checker<"NotNullTerminated">,
+ HelpText<"Check for arguments passed to C string functions which are not "
+ "null-terminated strings">,
+ Dependencies<[CStringModeling]>,
+ Documentation<HasDocumentation>;
+
def CStringNullArg : Checker<"NullArg">,
HelpText<"Check for null pointers being passed as arguments to C string "
"functions">,
@@ -485,11 +491,6 @@ def CStringBufferOverlap : Checker<"BufferOverlap">,
Dependencies<[CStringModeling]>,
Documentation<HasDocumentation>;
-def CStringNotNullTerm : Checker<"NotNullTerminated">,
- HelpText<"Check for arguments which are not null-terminating strings">,
- Dependencies<[CStringModeling]>,
- Documentation<HasDocumentation>;
-
def CStringUninitializedRead : Checker<"UninitializedRead">,
HelpText<"Checks if the string manipulation function would read uninitialized bytes">,
Dependencies<[CStringModeling]>,
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index e605c62a66ad0e..a84a0c2211fde0 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -53,6 +53,7 @@
// CHECK-NEXT: unix.StdCLibraryFunctions
// CHECK-NEXT: unix.Vfork
// CHECK-NEXT: unix.cstring.BadSizeArg
+// CHECK-NEXT: unix.cstring.NotNullTerminated
// CHECK-NEXT: unix.cstring.NullArg
int main() {
diff --git a/clang/test/Analysis/bstring.cpp b/clang/test/Analysis/bstring.cpp
index 1b6397c3455ebd..9c30bef15d407a 100644
--- a/clang/test/Analysis/bstring.cpp
+++ b/clang/test/Analysis/bstring.cpp
@@ -2,7 +2,7 @@
// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,unix.Malloc,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,unix.Malloc,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,alpha.unix.cstring.NotNullTerminated,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,unix.cstring.NotNullTerminated,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
#include "Inputs/system-header-simulator-cxx.h"
#include "Inputs/system-header-simulator-for-malloc.h"
diff --git a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
index 345a4e8f44efd1..3d1d3c561a5580 100644
--- a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -61,6 +61,7 @@
// CHECK-NEXT: unix.StdCLibraryFunctions
// CHECK-NEXT: unix.Vfork
// CHECK-NEXT: unix.cstring.BadSizeArg
+// CHECK-NEXT: unix.cstring.NotNullTerminated
// CHECK-NEXT: unix.cstring.NullArg
int main() {
diff --git a/clang/test/Analysis/string.c b/clang/test/Analysis/string.c
index 2e0a49d083b0b0..e017aff3b4a1db 100644
--- a/clang/test/Analysis/string.c
+++ b/clang/test/Analysis/string.c
@@ -38,7 +38,7 @@
// RUN: -analyzer-checker=unix.cstring \
// RUN: -analyzer-checker=unix.Malloc \
// RUN: -analyzer-checker=alpha.unix.cstring.BufferOverlap \
-// RUN: -analyzer-checker=alpha.unix.cstring.NotNullTerminated \
+// RUN: -analyzer-checker=unix.cstring.NotNullTerminated \
// RUN: -analyzer-checker=debug.ExprInspection \
// RUN: -analyzer-config eagerly-assume=false
diff --git a/clang/test/Analysis/string.cpp b/clang/test/Analysis/string.cpp
index c09422d1922369..e6cc950f30c9a0 100644
--- a/clang/test/Analysis/string.cpp
+++ b/clang/test/Analysis/string.cpp
@@ -53,3 +53,7 @@ struct TestNotNullTerm {
strlen((char *)&x); // expected-warning{{Argument to string length function is not a null-terminated string}}
}
};
+
+void test_notcstring_tempobject() {
+ strlen((char[]){'a', 0}); // expected-warning{{Argument to string length function is a C++ temp object of type char[2], which is not a null-terminated string}}
+}
|
NagyDonat
reviewed
Oct 28, 2024
There was a problem hiding this comment.
Overall I support moving this checker out of alpha, because its narrow functionality indeed seems to be stable enough for general use.
However, eventually it would be good to extend this checker to get a "real" NonNullTerminated checker that can deduce that certain "normal" strings don't have a proper null terminator.
clang/test/Analysis/string.cpp
Outdated
Comment on lines
57
to
59
| void test_notcstring_tempobject() { | ||
| strlen((char[]){'a', 0}); // expected-warning{{Argument to string length function is a C++ temp object of type char[2], which is not a null-terminated string}} | ||
| } |
There was a problem hiding this comment.
To me this result looks like a false positive (I'd guess that this strlen call is well-defined and returns 1, but I'm not familiar with the exact rules for C++ temp objects).
If this is indeed a false positive, then either fix it, or add a comment like "FIXME: In this corner case the checker produces a false positive".
If the report is a true positive, then perhaps explain why is it a true positive.
There was a problem hiding this comment.
This is taken from a FIXME in line 991 of the checker code. This exact case is really a false positive, a warning is needed only if a write happens into such a region. For this exact case (temporary object that behaves like a C string) the checker can be improved. But such a change would affect other checkers in the 'cstring' group.
steakhal
reviewed
Oct 28, 2024
|
I have added a FIXME to the place where no warning should appear. Additionally a new test file is added where only this checker is turned on (not the full |
NagyDonat
approved these changes
Nov 26, 2024
steakhal
approved these changes
Nov 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have tested it on the usual amount of C and C++ projects, no results were found. This checker is only for very rare cases, I do not know if it is really useful but it is there and can be moved out of alpha. There is no test when only this checker is enabled (and not full unix.cstring), but I have checked and it works too.