[clang][analyzer] Add checker 'unix.cstring.MissingTerminatingZero' #146664
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-static-analyzer-1 Author: Balázs Kéri (balazske) ChangesA new checker for checking if terminating zero is missing from a string. There is an existing Patch is 21.65 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/146664.diff 9 Files Affected:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 26c5028e04955..16265a9742f16 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2098,6 +2098,58 @@ Check the size argument passed into C string functions for common erroneous patt
// warn: potential buffer overflow
}
+.. _unix-cstring-MissingTerminatingZero:
+
+unix.cstring.MissingTerminatingZero (C)
+"""""""""""""""""""""""""""""""""""""""
+Check for string arguments passed to C library functions where the terminating
+zero is missing.
+
+The checker can only follow initializations with constant values and assignment
+of constant values to string elements.
+
+.. code-block:: c
+
+ int test1() {
+ char buf[4] = {1, 2, 3, 4};
+ return strlen(buf); // warn
+ }
+
+ int test2() {
+ char buf[] = "abcd";
+ buf[4] = 'e';
+ return strlen(buf); // warn
+ }
+
+ int test3() {
+ char buf[4];
+ buf[3] = 100;
+ return strlen(buf + 3); // warn
+ }
+
+**Options**
+
+By default the checker assumes that any parameter of type ``const char *`` to a
+global C system function should be a null-terminated string. Additionally there
+is a list of exceptions which are identified by the function name and parameter
+index. This list is called "ignore list" and contains these default values:
+(``stpncpy``, 1), (``strncat``, 1), (``strncmp``, 0), (``strncmp``, 1),
+(``strncpy``, 1), (``strndup``, 0), (``strnlen``, 0)
+These functions are ignored because they have a length parameter and can work
+with non-null terminated strings. The list can be changed by the following
+options:
+
+* ``OmitDefaultIgnoreFunctions`` (boolean). If true, the default ignore list is
+ cleared. (Independently of ``IgnoreFunctionArgs`` contains values or not.)
+
+* ``IgnoreFunctionArgs`` (string). Can be used to add functions to the ignore
+ list. It should contain entries in form of "<function name> <parameter index>"
+ separated by comma. These values are added to the ignore list. For example
+ ``strlen 0, strcpy 0, strcpy 1`` adds ``strlen`` and ``strcpy`` (both
+ parameters) to the ignore list. A function name can be used at most 2 times
+ (with different parameter values). Default value of the option is an empty
+ string.
+
.. _unix-cstring-NotNullTerminated:
unix.cstring.NotNullTerminated (C)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 2a96df80d1001..31a67c2992edf 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -480,6 +480,34 @@ def CStringSyntaxChecker : Checker<"BadSizeArg">,
Dependencies<[CStringModeling]>,
Documentation<HasDocumentation>;
+def MissingTerminatingZeroChecker
+ : Checker<"MissingTerminatingZero">,
+ HelpText<
+ "Check for string arguments passed to C library functions where the "
+ "terminating zero is missing">,
+ CheckerOptions<
+ [CmdLineOption<
+ Boolean, "OmitDefaultIgnoreFunctions",
+ "The checker checks by default all 'const char *' arguments "
+ "of system library C functions, except for a built-in list "
+ "of functions. If this parameter is set to true, this ignore "
+ "list is not used, but functions can be still specified by "
+ "the 'IgnoreFunctionArgs' option.",
+ "false", Released>,
+ CmdLineOption<
+ String, "IgnoreFunctionArgs",
+ "Specifies a list of functions to ignore by the checker. This "
+ "list should contain comma-separated values in the format "
+ "'<name> <parm>' where <name> is the function name and "
+ "<parm> is the zero-based index of the parameter (with "
+ "'const char *' type). The same function can be specified "
+ "with different parameters at most 2 times. These functions "
+ "are added to the default ignore list, unless "
+ "'OmitDefaultIgnoreFunctions' is true.",
+ "", Released>,
+]>,
+ Documentation<HasDocumentation>;
+
} // end "unix.cstring"
let ParentPackage = CStringAlpha in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 22dd3f0374849..d4960431402c2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -64,6 +64,7 @@ add_clang_library(clangStaticAnalyzerCheckers
MallocChecker.cpp
MallocSizeofChecker.cpp
MismatchedIteratorChecker.cpp
+ MissingTerminatingZeroChecker.cpp
MmapWriteExecChecker.cpp
MIGChecker.cpp
MoveChecker.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/MissingTerminatingZeroChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MissingTerminatingZeroChecker.cpp
new file mode 100644
index 0000000000000..292f0c809ac1a
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/MissingTerminatingZeroChecker.cpp
@@ -0,0 +1,295 @@
+//=== MissingTerminatingZeroChecker.cpp -------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Check for string arguments passed to C library functions where the
+// terminating zero is missing.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
+#include "llvm/ADT/BitVector.h"
+#include <sstream>
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+struct StringData {
+ const MemRegion *StrRegion;
+ int64_t StrLength;
+ unsigned int Offset;
+ const llvm::BitVector *NonNullData;
+};
+
+class MissingTerminatingZeroChecker
+ : public Checker<check::Bind, check::PreCall> {
+public:
+ void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+
+ void initOptions(bool NoDefaultIgnore, StringRef IgnoreList);
+
+private:
+ const BugType BT{this, "Missing terminating zero"};
+
+ using IgnoreEntry = std::pair<int, int>;
+ /// Functions (identified by name only) to ignore.
+ /// The entry stores a parameter index, or -1.
+ llvm::StringMap<IgnoreEntry> FunctionsToIgnore = {
+ {"stpncpy", {1, -1}}, {"strncat", {1, -1}}, {"strncmp", {0, 1}},
+ {"strncpy", {1, -1}}, {"strndup", {0, -1}}, {"strnlen", {0, -1}},
+ };
+
+ bool checkArg(unsigned int ArgI, CheckerContext &C,
+ const CallEvent &Call) const;
+ bool getStringData(StringData &DataOut, ProgramStateRef State,
+ SValBuilder &SVB, const MemRegion *StrReg) const;
+ ProgramStateRef setStringData(ProgramStateRef State, Loc L,
+ const llvm::BitVector &NonNullData) const;
+ void reportBug(ExplodedNode *N, const Expr *E, CheckerContext &C,
+ const char Msg[]) const;
+};
+
+} // namespace
+
+namespace llvm {
+template <> struct FoldingSetTrait<llvm::BitVector> {
+ static inline void Profile(llvm::BitVector X, FoldingSetNodeID &ID) {
+ ID.AddInteger(X.size());
+ for (unsigned int I = 0; I < X.size(); ++I)
+ ID.AddBoolean(X[I]);
+ }
+};
+} // end namespace llvm
+
+// Contains for a "string" (character array) region if elements are known to be
+// non-zero. The bit vector is indexed by the array element index and is true
+// if the element is known to be non-zero. Size of the vector does not
+// correspond to the extent of the memory region (can be smaller), the missing
+// elements are considered to be false.
+// (A value of 'false' means that the string element is zero or unknown.)
+REGISTER_MAP_WITH_PROGRAMSTATE(NonNullnessData, const MemRegion *,
+ llvm::BitVector)
+
+void MissingTerminatingZeroChecker::checkBind(SVal L, SVal V, const Stmt *S,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ const MemRegion *MR = L.getAsRegion();
+
+ if (const ElementRegion *ER = dyn_cast_or_null<ElementRegion>(MR)) {
+ // Assign value to the index of an array.
+ // Check for applicable array type.
+ QualType ElemType = ER->getValueType().getCanonicalType();
+ if (!ElemType->isCharType())
+ return;
+ if (C.getASTContext().getTypeSizeInChars(ElemType).getQuantity() != 1)
+ return;
+
+ RegionRawOffset ROffset = ER->getAsArrayOffset();
+ unsigned int Index = ROffset.getOffset().getQuantity();
+
+ // If the checker has data about the value to bind, use this information.
+ // Otherwise try to get it from the analyzer.
+ auto GetKnownToBeNonNull = [this, State, &C](SVal V) -> ConditionTruthVal {
+ StringData ExistingSData;
+ if (getStringData(ExistingSData, State, C.getSValBuilder(),
+ V.getAsRegion()) &&
+ ExistingSData.Offset < ExistingSData.NonNullData->size()) {
+ return ExistingSData.NonNullData->test(ExistingSData.Offset);
+ } else {
+ return State->isNonNull(V);
+ }
+ };
+ ConditionTruthVal VKnownToBeNonNull = GetKnownToBeNonNull(V);
+
+ if (const llvm::BitVector *NNData =
+ State->get<NonNullnessData>(ROffset.getRegion())) {
+ // Update existing data.
+ unsigned int NewSize =
+ Index < NNData->size() ? NNData->size() : Index + 1;
+ // Only extend the vector with 'true' value.
+ if (NewSize > NNData->size() && !VKnownToBeNonNull.isConstrainedTrue())
+ return;
+ llvm::BitVector NNData1(NewSize);
+ NNData1 |= *NNData;
+ NNData1[Index] = VKnownToBeNonNull.isConstrainedTrue();
+ State = State->set<NonNullnessData>(ROffset.getRegion(), NNData1);
+ } else {
+ // Only add new data if 'true' is found.
+ if (!VKnownToBeNonNull.isConstrainedTrue())
+ return;
+ llvm::BitVector NNData1(Index + 1);
+ NNData1[Index] = true;
+ State = State->set<NonNullnessData>(ROffset.getRegion(), NNData1);
+ }
+ } else if (const TypedValueRegion *TR =
+ dyn_cast_or_null<TypedValueRegion>(MR)) {
+ // Initialize a region with compound value from list or string literal.
+ QualType Type = TR->getValueType().getCanonicalType();
+ if (!Type->isArrayType())
+ return;
+ if (!Type->castAsArrayTypeUnsafe()->getElementType()->isCharType())
+ return;
+
+ if (auto CVal = V.getAs<nonloc::CompoundVal>()) {
+ llvm::BitVector NNData;
+ for (auto Val = CVal->begin(); Val != CVal->end(); ++Val)
+ NNData.push_back(State->isNonNull(*Val).isConstrainedTrue());
+ State = State->set<NonNullnessData>(MR, NNData);
+ } else if (auto MRV = V.getAs<loc::MemRegionVal>()) {
+ if (auto *StrReg = MRV->stripCasts()->getAs<StringRegion>()) {
+ StringRef Str = StrReg->getStringLiteral()->getString();
+ size_t StrL = Str.size();
+ llvm::BitVector NNData(StrL + 1, true);
+ for (unsigned int I = 0; I < StrL; ++I)
+ if (Str[I] == 0)
+ NNData.reset(I);
+ NNData.reset(StrL);
+ State = State->set<NonNullnessData>(MR, NNData);
+ }
+ }
+ }
+ C.addTransition(State);
+}
+
+void MissingTerminatingZeroChecker::checkPreCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ if (!Call.isInSystemHeader() || !Call.isGlobalCFunction())
+ return;
+
+ auto Ignore = [this](StringRef Name) -> IgnoreEntry {
+ auto FIgnore = FunctionsToIgnore.find(Name);
+ if (FIgnore != FunctionsToIgnore.end())
+ return FIgnore->getValue();
+ return IgnoreEntry{-1, -1};
+ }(cast<NamedDecl>(Call.getDecl())->getNameAsString());
+
+ for (const auto &[ArgI, ParmD] : enumerate(Call.parameters())) {
+ QualType ArgTy = ParmD->getType();
+ if (!ArgTy->isPointerType())
+ continue;
+ QualType ArgPointeeTy = ArgTy->getPointeeType();
+ if (!ArgPointeeTy->isCharType() || !ArgPointeeTy.isConstQualified())
+ continue;
+ if (static_cast<int>(ArgI) == Ignore.first ||
+ static_cast<int>(ArgI) == Ignore.second)
+ continue;
+
+ if (checkArg(ArgI, C, Call))
+ return;
+ }
+}
+
+bool MissingTerminatingZeroChecker::checkArg(unsigned int ArgI,
+ CheckerContext &C,
+ const CallEvent &Call) const {
+ SVal StrArgVal = Call.getArgSVal(ArgI);
+
+ StringData SData;
+ if (!getStringData(SData, C.getState(), C.getSValBuilder(),
+ StrArgVal.getAsRegion()))
+ return false;
+
+ // Check if all elements in the string are known to be non-zero.
+ // The stored bitvector can have a smaller size.
+ if (SData.NonNullData->size() < SData.StrLength)
+ return false;
+ for (int64_t I = SData.Offset; I < SData.StrLength; ++I)
+ if (!SData.NonNullData->test(I))
+ return false;
+
+ reportBug(C.getPredecessor(), Call.getArgExpr(ArgI), C,
+ "String contains no terminating zero; At this place a "
+ "null-terminated string is expected");
+ return true;
+}
+
+bool MissingTerminatingZeroChecker::getStringData(
+ StringData &DataOut, ProgramStateRef State, SValBuilder &SVB,
+ const MemRegion *StrReg) const {
+ if (!StrReg)
+ return false;
+
+ unsigned int Offset = 0;
+ if (const auto *ElemReg = dyn_cast_or_null<ElementRegion>(StrReg)) {
+ RegionRawOffset ROffset = ElemReg->getAsArrayOffset();
+ StrReg = ROffset.getRegion();
+ if (!StrReg)
+ return false;
+ Offset = ROffset.getOffset().getQuantity();
+ }
+
+ const llvm::BitVector *NNData = State->get<NonNullnessData>(StrReg);
+ if (!NNData)
+ return false;
+
+ DefinedOrUnknownSVal Extent = getDynamicExtent(State, StrReg, SVB);
+ if (Extent.isUnknown())
+ return false;
+ const llvm::APSInt *KnownExtent = SVB.getKnownValue(State, Extent);
+ if (!KnownExtent)
+ return false;
+
+ DataOut.StrRegion = StrReg;
+ DataOut.StrLength = KnownExtent->getExtValue();
+ DataOut.Offset = Offset;
+ DataOut.NonNullData = NNData;
+
+ return true;
+}
+
+void MissingTerminatingZeroChecker::reportBug(ExplodedNode *N, const Expr *E,
+ CheckerContext &C,
+ const char Msg[]) const {
+ auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
+ bugreporter::trackExpressionValue(N, E, *R);
+ C.emitReport(std::move(R));
+}
+
+void MissingTerminatingZeroChecker::initOptions(bool NoDefaultIgnore,
+ StringRef IgnoreList) {
+ if (NoDefaultIgnore)
+ FunctionsToIgnore.clear();
+ std::istringstream IgnoreInput{std::string(IgnoreList)};
+ std::array<char, 100> I;
+ while (IgnoreInput.getline(&I[0], 100, ';')) {
+ std::istringstream FunctionInput{std::string(&I[0])};
+ std::string FName;
+ int FArg;
+ if (FunctionInput >> FName >> FArg) {
+ IgnoreEntry &E =
+ FunctionsToIgnore.insert({FName, {-1, -1}}).first->getValue();
+ if (E.first == -1)
+ E.first = FArg;
+ else if (E.second == -1)
+ E.second = FArg;
+ }
+ }
+}
+
+void ento::registerMissingTerminatingZeroChecker(CheckerManager &Mgr) {
+ auto *Checker = Mgr.registerChecker<MissingTerminatingZeroChecker>();
+ bool NoDefaultIgnore = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+ Checker, "OmitDefaultIgnoreFunctions");
+ StringRef IgnoreList = Mgr.getAnalyzerOptions().getCheckerStringOption(
+ Checker, "IgnoreFunctionArgs");
+ Checker->initOptions(NoDefaultIgnore, IgnoreList);
+}
+
+bool ento::shouldRegisterMissingTerminatingZeroChecker(
+ const CheckerManager &Mgr) {
+ // Check if the char type has size of 8 bits (to avoid indexing differences)?
+ return true;
+}
diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c
index 7936273415ad4..d477987e51642 100644
--- a/clang/test/Analysis/analyzer-config.c
+++ b/clang/test/Analysis/analyzer-config.c
@@ -137,6 +137,8 @@
// CHECK-NEXT: unix.StdCLibraryFunctions:DisplayLoadedSummaries = false
// CHECK-NEXT: unix.StdCLibraryFunctions:ModelPOSIX = true
// CHECK-NEXT: unix.Stream:Pedantic = false
+// CHECK-NEXT: unix.cstring.MissingTerminatingZero:IgnoreFunctionArgs = ""
+// CHECK-NEXT: unix.cstring.MissingTerminatingZero:OmitDefaultIgnoreFunctions = false
// CHECK-NEXT: unroll-loops = false
// CHECK-NEXT: verbose-report-filename = false
// CHECK-NEXT: widen-loops = false
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index 66b9be9795f12..127bd3298c76c 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -57,6 +57,7 @@
// CHECK-NEXT: unix.StdCLibraryFunctions
// CHECK-NEXT: unix.Vfork
// CHECK-NEXT: unix.cstring.BadSizeArg
+// CHECK-NEXT: unix.cstring.MissingTerminatingZero
// CHECK-NEXT: unix.cstring.NotNullTerminated
// CHECK-NEXT: unix.cstring.NullArg
diff --git a/clang/test/Analysis/cstring-missingterminatingzero-ignore.c b/clang/test/Analysis/cstring-missingterminatingzero-ignore.c
new file mode 100644
index 0000000000000..8b96bf81c55cd
--- /dev/null
+++ b/clang/test/Analysis/cstring-missingterminatingzero-ignore.c
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.MissingTerminatingZero \
+// RUN: -analyzer-config unix.cstring.MissingTerminatingZero:IgnoreFunctionArgs='strlen 0;strcpy 1' -verify=all,ignore %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.MissingTerminatingZero \
+// RUN: -analyzer-config unix.cstring.MissingTerminatingZero:IgnoreFunctionArgs='strlen 0;strcpy 1' \
+// RUN: -analyzer-config unix.cstring.MissingTerminatingZero:OmitDefaultIgnoreFunctions=true -verify=all,omitdefault %s
+
+#include "Inputs/system-header-simulator.h"
+
+size_t test1(int i) {
+ char buf[1] = {1};
+ return strlen(buf);
+}
+
+void test2(char *dst) {
+ char src[1] = {1};
+ strcpy(dst, src);
+}
+
+int test3() {
+ const char buf[1] = {1};
+ return execl("path", buf, 4); // all-warning{{String contains no terminating zero}}
+}
+
+void test4(char *dst) {
+ char src[3] = {1, 2, 3};
+ strncpy(dst, src, 3); // omitdefault-warning{{String contains no terminating zero}}
+}
diff --git a/clang/test/Analysis/cstring-missingterminatingzero.c b/clang/test/Analysis/cstring-missingterminatingzero.c
new file mode 100644
index 0000000000000..d0ae6cb19db87
--- /dev/null
+++ b/clang/test/Analysis/cstring-missingterminatingzero.c
@@ -0,0 +1,91 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.MissingTerminatingZero -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+
+size_t test_init_compound(int i) {
+ char src1[6] = {1,2,3,4,5,6};
+ char src2[6] = {1,2,3,0,5,6};
+ switch (i) {
+ case 1:
+ return strlen(src1); // expected-warning{{String contains no terminating zero}}
+ case 2:
+ return strlen(src1 + 1); // expected-warning{{String contains no terminating zero}}
+ case 3:
+ return strlen(src2);
+ case 4:
+ return strlen(src2 + 4); // expected-warning{{String contains no terminating zero}}
+ case 5:
+ return strlen(src2 + 3);
+ }
+ src1[5] = 0;
+ return strlen(src1);
+}
+
+typedef char CHAR;
+
+size_t test_init_literal(int i) {
+ CHAR src1[] = "abcdef";
+ int l = strlen(src1);
+ src1[6] = '.';
+ src1[3] = 0;
+ switch (i) {
+ case 1:
+ return strlen(src1);
+ case 2:
+ return strlen(src1 + 4); // expected-warning{{String contains no terminating zero}}
+ }
+ return l;
+}
+
+size_t test_init_assign(int i, char a) {
+ char src[6];
+ src[1] = '1';
+ src[2] = '2';
+ src[4] = '4';
+ src[5] = '5';
+
+ switch (i) {
+ case 0:
+ return ...
[truncated]
|
|
@llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) ChangesA new checker for checking if terminating zero is missing from a string. There is an existing Patch is 21.65 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/146664.diff 9 Files Affected:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 26c5028e04955..16265a9742f16 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2098,6 +2098,58 @@ Check the size argument passed into C string functions for common erroneous patt
// warn: potential buffer overflow
}
+.. _unix-cstring-MissingTerminatingZero:
+
+unix.cstring.MissingTerminatingZero (C)
+"""""""""""""""""""""""""""""""""""""""
+Check for string arguments passed to C library functions where the terminating
+zero is missing.
+
+The checker can only follow initializations with constant values and assignment
+of constant values to string elements.
+
+.. code-block:: c
+
+ int test1() {
+ char buf[4] = {1, 2, 3, 4};
+ return strlen(buf); // warn
+ }
+
+ int test2() {
+ char buf[] = "abcd";
+ buf[4] = 'e';
+ return strlen(buf); // warn
+ }
+
+ int test3() {
+ char buf[4];
+ buf[3] = 100;
+ return strlen(buf + 3); // warn
+ }
+
+**Options**
+
+By default the checker assumes that any parameter of type ``const char *`` to a
+global C system function should be a null-terminated string. Additionally there
+is a list of exceptions which are identified by the function name and parameter
+index. This list is called "ignore list" and contains these default values:
+(``stpncpy``, 1), (``strncat``, 1), (``strncmp``, 0), (``strncmp``, 1),
+(``strncpy``, 1), (``strndup``, 0), (``strnlen``, 0)
+These functions are ignored because they have a length parameter and can work
+with non-null terminated strings. The list can be changed by the following
+options:
+
+* ``OmitDefaultIgnoreFunctions`` (boolean). If true, the default ignore list is
+ cleared. (Independently of ``IgnoreFunctionArgs`` contains values or not.)
+
+* ``IgnoreFunctionArgs`` (string). Can be used to add functions to the ignore
+ list. It should contain entries in form of "<function name> <parameter index>"
+ separated by comma. These values are added to the ignore list. For example
+ ``strlen 0, strcpy 0, strcpy 1`` adds ``strlen`` and ``strcpy`` (both
+ parameters) to the ignore list. A function name can be used at most 2 times
+ (with different parameter values). Default value of the option is an empty
+ string.
+
.. _unix-cstring-NotNullTerminated:
unix.cstring.NotNullTerminated (C)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 2a96df80d1001..31a67c2992edf 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -480,6 +480,34 @@ def CStringSyntaxChecker : Checker<"BadSizeArg">,
Dependencies<[CStringModeling]>,
Documentation<HasDocumentation>;
+def MissingTerminatingZeroChecker
+ : Checker<"MissingTerminatingZero">,
+ HelpText<
+ "Check for string arguments passed to C library functions where the "
+ "terminating zero is missing">,
+ CheckerOptions<
+ [CmdLineOption<
+ Boolean, "OmitDefaultIgnoreFunctions",
+ "The checker checks by default all 'const char *' arguments "
+ "of system library C functions, except for a built-in list "
+ "of functions. If this parameter is set to true, this ignore "
+ "list is not used, but functions can be still specified by "
+ "the 'IgnoreFunctionArgs' option.",
+ "false", Released>,
+ CmdLineOption<
+ String, "IgnoreFunctionArgs",
+ "Specifies a list of functions to ignore by the checker. This "
+ "list should contain comma-separated values in the format "
+ "'<name> <parm>' where <name> is the function name and "
+ "<parm> is the zero-based index of the parameter (with "
+ "'const char *' type). The same function can be specified "
+ "with different parameters at most 2 times. These functions "
+ "are added to the default ignore list, unless "
+ "'OmitDefaultIgnoreFunctions' is true.",
+ "", Released>,
+]>,
+ Documentation<HasDocumentation>;
+
} // end "unix.cstring"
let ParentPackage = CStringAlpha in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 22dd3f0374849..d4960431402c2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -64,6 +64,7 @@ add_clang_library(clangStaticAnalyzerCheckers
MallocChecker.cpp
MallocSizeofChecker.cpp
MismatchedIteratorChecker.cpp
+ MissingTerminatingZeroChecker.cpp
MmapWriteExecChecker.cpp
MIGChecker.cpp
MoveChecker.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/MissingTerminatingZeroChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MissingTerminatingZeroChecker.cpp
new file mode 100644
index 0000000000000..292f0c809ac1a
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/MissingTerminatingZeroChecker.cpp
@@ -0,0 +1,295 @@
+//=== MissingTerminatingZeroChecker.cpp -------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Check for string arguments passed to C library functions where the
+// terminating zero is missing.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
+#include "llvm/ADT/BitVector.h"
+#include <sstream>
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+struct StringData {
+ const MemRegion *StrRegion;
+ int64_t StrLength;
+ unsigned int Offset;
+ const llvm::BitVector *NonNullData;
+};
+
+class MissingTerminatingZeroChecker
+ : public Checker<check::Bind, check::PreCall> {
+public:
+ void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+
+ void initOptions(bool NoDefaultIgnore, StringRef IgnoreList);
+
+private:
+ const BugType BT{this, "Missing terminating zero"};
+
+ using IgnoreEntry = std::pair<int, int>;
+ /// Functions (identified by name only) to ignore.
+ /// The entry stores a parameter index, or -1.
+ llvm::StringMap<IgnoreEntry> FunctionsToIgnore = {
+ {"stpncpy", {1, -1}}, {"strncat", {1, -1}}, {"strncmp", {0, 1}},
+ {"strncpy", {1, -1}}, {"strndup", {0, -1}}, {"strnlen", {0, -1}},
+ };
+
+ bool checkArg(unsigned int ArgI, CheckerContext &C,
+ const CallEvent &Call) const;
+ bool getStringData(StringData &DataOut, ProgramStateRef State,
+ SValBuilder &SVB, const MemRegion *StrReg) const;
+ ProgramStateRef setStringData(ProgramStateRef State, Loc L,
+ const llvm::BitVector &NonNullData) const;
+ void reportBug(ExplodedNode *N, const Expr *E, CheckerContext &C,
+ const char Msg[]) const;
+};
+
+} // namespace
+
+namespace llvm {
+template <> struct FoldingSetTrait<llvm::BitVector> {
+ static inline void Profile(llvm::BitVector X, FoldingSetNodeID &ID) {
+ ID.AddInteger(X.size());
+ for (unsigned int I = 0; I < X.size(); ++I)
+ ID.AddBoolean(X[I]);
+ }
+};
+} // end namespace llvm
+
+// Contains for a "string" (character array) region if elements are known to be
+// non-zero. The bit vector is indexed by the array element index and is true
+// if the element is known to be non-zero. Size of the vector does not
+// correspond to the extent of the memory region (can be smaller), the missing
+// elements are considered to be false.
+// (A value of 'false' means that the string element is zero or unknown.)
+REGISTER_MAP_WITH_PROGRAMSTATE(NonNullnessData, const MemRegion *,
+ llvm::BitVector)
+
+void MissingTerminatingZeroChecker::checkBind(SVal L, SVal V, const Stmt *S,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ const MemRegion *MR = L.getAsRegion();
+
+ if (const ElementRegion *ER = dyn_cast_or_null<ElementRegion>(MR)) {
+ // Assign value to the index of an array.
+ // Check for applicable array type.
+ QualType ElemType = ER->getValueType().getCanonicalType();
+ if (!ElemType->isCharType())
+ return;
+ if (C.getASTContext().getTypeSizeInChars(ElemType).getQuantity() != 1)
+ return;
+
+ RegionRawOffset ROffset = ER->getAsArrayOffset();
+ unsigned int Index = ROffset.getOffset().getQuantity();
+
+ // If the checker has data about the value to bind, use this information.
+ // Otherwise try to get it from the analyzer.
+ auto GetKnownToBeNonNull = [this, State, &C](SVal V) -> ConditionTruthVal {
+ StringData ExistingSData;
+ if (getStringData(ExistingSData, State, C.getSValBuilder(),
+ V.getAsRegion()) &&
+ ExistingSData.Offset < ExistingSData.NonNullData->size()) {
+ return ExistingSData.NonNullData->test(ExistingSData.Offset);
+ } else {
+ return State->isNonNull(V);
+ }
+ };
+ ConditionTruthVal VKnownToBeNonNull = GetKnownToBeNonNull(V);
+
+ if (const llvm::BitVector *NNData =
+ State->get<NonNullnessData>(ROffset.getRegion())) {
+ // Update existing data.
+ unsigned int NewSize =
+ Index < NNData->size() ? NNData->size() : Index + 1;
+ // Only extend the vector with 'true' value.
+ if (NewSize > NNData->size() && !VKnownToBeNonNull.isConstrainedTrue())
+ return;
+ llvm::BitVector NNData1(NewSize);
+ NNData1 |= *NNData;
+ NNData1[Index] = VKnownToBeNonNull.isConstrainedTrue();
+ State = State->set<NonNullnessData>(ROffset.getRegion(), NNData1);
+ } else {
+ // Only add new data if 'true' is found.
+ if (!VKnownToBeNonNull.isConstrainedTrue())
+ return;
+ llvm::BitVector NNData1(Index + 1);
+ NNData1[Index] = true;
+ State = State->set<NonNullnessData>(ROffset.getRegion(), NNData1);
+ }
+ } else if (const TypedValueRegion *TR =
+ dyn_cast_or_null<TypedValueRegion>(MR)) {
+ // Initialize a region with compound value from list or string literal.
+ QualType Type = TR->getValueType().getCanonicalType();
+ if (!Type->isArrayType())
+ return;
+ if (!Type->castAsArrayTypeUnsafe()->getElementType()->isCharType())
+ return;
+
+ if (auto CVal = V.getAs<nonloc::CompoundVal>()) {
+ llvm::BitVector NNData;
+ for (auto Val = CVal->begin(); Val != CVal->end(); ++Val)
+ NNData.push_back(State->isNonNull(*Val).isConstrainedTrue());
+ State = State->set<NonNullnessData>(MR, NNData);
+ } else if (auto MRV = V.getAs<loc::MemRegionVal>()) {
+ if (auto *StrReg = MRV->stripCasts()->getAs<StringRegion>()) {
+ StringRef Str = StrReg->getStringLiteral()->getString();
+ size_t StrL = Str.size();
+ llvm::BitVector NNData(StrL + 1, true);
+ for (unsigned int I = 0; I < StrL; ++I)
+ if (Str[I] == 0)
+ NNData.reset(I);
+ NNData.reset(StrL);
+ State = State->set<NonNullnessData>(MR, NNData);
+ }
+ }
+ }
+ C.addTransition(State);
+}
+
+void MissingTerminatingZeroChecker::checkPreCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ if (!Call.isInSystemHeader() || !Call.isGlobalCFunction())
+ return;
+
+ auto Ignore = [this](StringRef Name) -> IgnoreEntry {
+ auto FIgnore = FunctionsToIgnore.find(Name);
+ if (FIgnore != FunctionsToIgnore.end())
+ return FIgnore->getValue();
+ return IgnoreEntry{-1, -1};
+ }(cast<NamedDecl>(Call.getDecl())->getNameAsString());
+
+ for (const auto &[ArgI, ParmD] : enumerate(Call.parameters())) {
+ QualType ArgTy = ParmD->getType();
+ if (!ArgTy->isPointerType())
+ continue;
+ QualType ArgPointeeTy = ArgTy->getPointeeType();
+ if (!ArgPointeeTy->isCharType() || !ArgPointeeTy.isConstQualified())
+ continue;
+ if (static_cast<int>(ArgI) == Ignore.first ||
+ static_cast<int>(ArgI) == Ignore.second)
+ continue;
+
+ if (checkArg(ArgI, C, Call))
+ return;
+ }
+}
+
+bool MissingTerminatingZeroChecker::checkArg(unsigned int ArgI,
+ CheckerContext &C,
+ const CallEvent &Call) const {
+ SVal StrArgVal = Call.getArgSVal(ArgI);
+
+ StringData SData;
+ if (!getStringData(SData, C.getState(), C.getSValBuilder(),
+ StrArgVal.getAsRegion()))
+ return false;
+
+ // Check if all elements in the string are known to be non-zero.
+ // The stored bitvector can have a smaller size.
+ if (SData.NonNullData->size() < SData.StrLength)
+ return false;
+ for (int64_t I = SData.Offset; I < SData.StrLength; ++I)
+ if (!SData.NonNullData->test(I))
+ return false;
+
+ reportBug(C.getPredecessor(), Call.getArgExpr(ArgI), C,
+ "String contains no terminating zero; At this place a "
+ "null-terminated string is expected");
+ return true;
+}
+
+bool MissingTerminatingZeroChecker::getStringData(
+ StringData &DataOut, ProgramStateRef State, SValBuilder &SVB,
+ const MemRegion *StrReg) const {
+ if (!StrReg)
+ return false;
+
+ unsigned int Offset = 0;
+ if (const auto *ElemReg = dyn_cast_or_null<ElementRegion>(StrReg)) {
+ RegionRawOffset ROffset = ElemReg->getAsArrayOffset();
+ StrReg = ROffset.getRegion();
+ if (!StrReg)
+ return false;
+ Offset = ROffset.getOffset().getQuantity();
+ }
+
+ const llvm::BitVector *NNData = State->get<NonNullnessData>(StrReg);
+ if (!NNData)
+ return false;
+
+ DefinedOrUnknownSVal Extent = getDynamicExtent(State, StrReg, SVB);
+ if (Extent.isUnknown())
+ return false;
+ const llvm::APSInt *KnownExtent = SVB.getKnownValue(State, Extent);
+ if (!KnownExtent)
+ return false;
+
+ DataOut.StrRegion = StrReg;
+ DataOut.StrLength = KnownExtent->getExtValue();
+ DataOut.Offset = Offset;
+ DataOut.NonNullData = NNData;
+
+ return true;
+}
+
+void MissingTerminatingZeroChecker::reportBug(ExplodedNode *N, const Expr *E,
+ CheckerContext &C,
+ const char Msg[]) const {
+ auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
+ bugreporter::trackExpressionValue(N, E, *R);
+ C.emitReport(std::move(R));
+}
+
+void MissingTerminatingZeroChecker::initOptions(bool NoDefaultIgnore,
+ StringRef IgnoreList) {
+ if (NoDefaultIgnore)
+ FunctionsToIgnore.clear();
+ std::istringstream IgnoreInput{std::string(IgnoreList)};
+ std::array<char, 100> I;
+ while (IgnoreInput.getline(&I[0], 100, ';')) {
+ std::istringstream FunctionInput{std::string(&I[0])};
+ std::string FName;
+ int FArg;
+ if (FunctionInput >> FName >> FArg) {
+ IgnoreEntry &E =
+ FunctionsToIgnore.insert({FName, {-1, -1}}).first->getValue();
+ if (E.first == -1)
+ E.first = FArg;
+ else if (E.second == -1)
+ E.second = FArg;
+ }
+ }
+}
+
+void ento::registerMissingTerminatingZeroChecker(CheckerManager &Mgr) {
+ auto *Checker = Mgr.registerChecker<MissingTerminatingZeroChecker>();
+ bool NoDefaultIgnore = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+ Checker, "OmitDefaultIgnoreFunctions");
+ StringRef IgnoreList = Mgr.getAnalyzerOptions().getCheckerStringOption(
+ Checker, "IgnoreFunctionArgs");
+ Checker->initOptions(NoDefaultIgnore, IgnoreList);
+}
+
+bool ento::shouldRegisterMissingTerminatingZeroChecker(
+ const CheckerManager &Mgr) {
+ // Check if the char type has size of 8 bits (to avoid indexing differences)?
+ return true;
+}
diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c
index 7936273415ad4..d477987e51642 100644
--- a/clang/test/Analysis/analyzer-config.c
+++ b/clang/test/Analysis/analyzer-config.c
@@ -137,6 +137,8 @@
// CHECK-NEXT: unix.StdCLibraryFunctions:DisplayLoadedSummaries = false
// CHECK-NEXT: unix.StdCLibraryFunctions:ModelPOSIX = true
// CHECK-NEXT: unix.Stream:Pedantic = false
+// CHECK-NEXT: unix.cstring.MissingTerminatingZero:IgnoreFunctionArgs = ""
+// CHECK-NEXT: unix.cstring.MissingTerminatingZero:OmitDefaultIgnoreFunctions = false
// CHECK-NEXT: unroll-loops = false
// CHECK-NEXT: verbose-report-filename = false
// CHECK-NEXT: widen-loops = false
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index 66b9be9795f12..127bd3298c76c 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -57,6 +57,7 @@
// CHECK-NEXT: unix.StdCLibraryFunctions
// CHECK-NEXT: unix.Vfork
// CHECK-NEXT: unix.cstring.BadSizeArg
+// CHECK-NEXT: unix.cstring.MissingTerminatingZero
// CHECK-NEXT: unix.cstring.NotNullTerminated
// CHECK-NEXT: unix.cstring.NullArg
diff --git a/clang/test/Analysis/cstring-missingterminatingzero-ignore.c b/clang/test/Analysis/cstring-missingterminatingzero-ignore.c
new file mode 100644
index 0000000000000..8b96bf81c55cd
--- /dev/null
+++ b/clang/test/Analysis/cstring-missingterminatingzero-ignore.c
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.MissingTerminatingZero \
+// RUN: -analyzer-config unix.cstring.MissingTerminatingZero:IgnoreFunctionArgs='strlen 0;strcpy 1' -verify=all,ignore %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.MissingTerminatingZero \
+// RUN: -analyzer-config unix.cstring.MissingTerminatingZero:IgnoreFunctionArgs='strlen 0;strcpy 1' \
+// RUN: -analyzer-config unix.cstring.MissingTerminatingZero:OmitDefaultIgnoreFunctions=true -verify=all,omitdefault %s
+
+#include "Inputs/system-header-simulator.h"
+
+size_t test1(int i) {
+ char buf[1] = {1};
+ return strlen(buf);
+}
+
+void test2(char *dst) {
+ char src[1] = {1};
+ strcpy(dst, src);
+}
+
+int test3() {
+ const char buf[1] = {1};
+ return execl("path", buf, 4); // all-warning{{String contains no terminating zero}}
+}
+
+void test4(char *dst) {
+ char src[3] = {1, 2, 3};
+ strncpy(dst, src, 3); // omitdefault-warning{{String contains no terminating zero}}
+}
diff --git a/clang/test/Analysis/cstring-missingterminatingzero.c b/clang/test/Analysis/cstring-missingterminatingzero.c
new file mode 100644
index 0000000000000..d0ae6cb19db87
--- /dev/null
+++ b/clang/test/Analysis/cstring-missingterminatingzero.c
@@ -0,0 +1,91 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.MissingTerminatingZero -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+
+size_t test_init_compound(int i) {
+ char src1[6] = {1,2,3,4,5,6};
+ char src2[6] = {1,2,3,0,5,6};
+ switch (i) {
+ case 1:
+ return strlen(src1); // expected-warning{{String contains no terminating zero}}
+ case 2:
+ return strlen(src1 + 1); // expected-warning{{String contains no terminating zero}}
+ case 3:
+ return strlen(src2);
+ case 4:
+ return strlen(src2 + 4); // expected-warning{{String contains no terminating zero}}
+ case 5:
+ return strlen(src2 + 3);
+ }
+ src1[5] = 0;
+ return strlen(src1);
+}
+
+typedef char CHAR;
+
+size_t test_init_literal(int i) {
+ CHAR src1[] = "abcdef";
+ int l = strlen(src1);
+ src1[6] = '.';
+ src1[3] = 0;
+ switch (i) {
+ case 1:
+ return strlen(src1);
+ case 2:
+ return strlen(src1 + 4); // expected-warning{{String contains no terminating zero}}
+ }
+ return l;
+}
+
+size_t test_init_assign(int i, char a) {
+ char src[6];
+ src[1] = '1';
+ src[2] = '2';
+ src[4] = '4';
+ src[5] = '5';
+
+ switch (i) {
+ case 0:
+ return ...
[truncated]
|
|
Pointer escape and region changes are not handled yet. |
NagyDonat
reviewed
Jul 3, 2025
| return strlen(src1); | ||
| } | ||
|
|
||
| typedef char CHAR; |
There was a problem hiding this comment.
Why do you use this typedef?
Comment on lines
+83
to
+84
| REGISTER_MAP_WITH_PROGRAMSTATE(NonNullnessData, const MemRegion *, | ||
| llvm::BitVector) |
There was a problem hiding this comment.
Why do you introduce this map instead of relying on the standard RegionStore? Unless there is a really compelling reason, we shouldn't duplicate the same data in two different locations.
There was a problem hiding this comment.
I thought the CStringChecker has the CStringLength trait that maps lvalues to the symbolic length of the null-terminated string. In other words, CStringLength represents the position of the null-terminator.
Have you thought of reusing the same trait for representing this?
There was a problem hiding this comment.
I found that CStringLength is only set when a string literal is used at initialization and is not updated at later changes (if value is assigned to array element). Other ("compound") initializations can be handled but we must use a special value if there is not a terminating zero. Cases like char a[5] = {1, 0, 1, 0, 1} are still difficult to handle in this way. I plan to add support for functions like memcpy (if both argument strings and the size are known to the checker the non-null-data can be updated). These changes would be more difficult to handle if only the length of the strings is known.
Another difference is that the checker stores positions of known nonzero characters, not of a known zero (at the end). A bug is reported only if the whole string contains known nonzero values. In this way we should not get false positives from the checker (or probably for other reasons like escape issues).
There was a problem hiding this comment.
Why do you introduce this map instead of relying on the standard
RegionStore? Unless there is a really compelling reason, we shouldn't duplicate the same data in two different locations.
If I remember my first idea was to only check the affected (element) regions and it would be a simple checker. But somehow I did not find a way to iterate over the array elements (or subregions). Probably array initialization did not appear in the element regions in all cases too. Additionally string copy functions can be more difficult to handle in that way. (If copy is added it should be in a different checker, probably CStringChecker.)
There was a problem hiding this comment.
Or would it be better to "get" (or create?) an element region for all elements of the string when for example a strlen is called?
There was a problem hiding this comment.
I discussed this architectural question with @dkrupp and @gamesh411 and we concluded that it would be better to improve and use the standard RegionStore instead of introducing a new custom state component.
We understand that improving RegionStore is more difficult than implementing this fresh specialized data structure, but on a long term it would be a nightmare to maintain two (or more if other checkers follow this example) overlapping memory models that each have their own advantages and drawbacks.
If I remember my first idea was to only check the affected (element) regions and it would be a simple checker. But somehow I did not find a way to iterate over the array elements (or subregions).
I also recall that this limitation exists, but it could and should be addressed by improving the interface of RegionStore.
Probably array initialization did not appear in the element regions in all cases too.
Yes, there is a performance optimization that array initialization only initializes the first few elements, but strings initialized by array initialization should be very rare in real-world code, so this shouldn't be a serious limitation. If the current limitation is too restrictive, you can increase it a bit to get a few more initialized elements.
Additionally string copy functions can be more difficult to handle in that way.
But modeling the string and memory copy functions is also needed by the other checkers (that get data from RegionStore), so we should model them by updating RegionStore even if that's more difficult to implement. (By the way @steakhal do you have any plans/ideas about modeling strcpy / memcpy etc.? How difficult would it be to model them with a default binding that places a suitable LazyCompoundVal?)
There was a problem hiding this comment.
Or would it be better to "get" (or create?) an element region for all elements of the string when for example a
strlenis called?
Yes, this is the standard approach, so it would be better (unless there are unavoidable issues with it). Obviously we must introduce a practical limit (32? 128? 256?) for the number of checked elements -- to avoid performance issues -- but otherwise this should work.
If I understand it correctly an element region (i.e. ElementRegion object) is a lightweight identifier-like object, so creating it shouldn't cause a significant inherent performance burden. Alternatively, if querying all elements of a string separately is too slow, then we could extend the interface of RegionStore with "bulk query" methods that can return knowledge about a larger memory area (e.g. string or other array) without splitting it into separate elements. (The bulk query should be able to combine multiple smaller separate bindings, but should be efficient for the common case when the whole region is initialized from e.g. a string literal.)
Comment on lines
+2134
to
+2137
| is a list of exceptions which are identified by the function name and parameter | ||
| index. This list is called "ignore list" and contains these default values: | ||
| (``stpncpy``, 1), (``strncat``, 1), (``strncmp``, 0), (``strncmp``, 1), | ||
| (``strncpy``, 1), (``strndup``, 0), (``strnlen``, 0) |
There was a problem hiding this comment.
Why are you tracking the parameter indices on the exception list? Do you know about any significant builtin functions where one parameter is an exception (doesn't need to be null-terminated) while another parameter must be null-terminated?
If not, then it would be much simpler to just track the function names.
There was a problem hiding this comment.
Functions like strncpy have a destination that should be null-terminated and a source string that can be not null-terminated.
There was a problem hiding this comment.
The destination parameter is a non-const char * while (at least according to the documentation) your checker only checks the const char * parameters of the C library functions.
Comment on lines
+2145
to
+2151
| * ``IgnoreFunctionArgs`` (string). Can be used to add functions to the ignore | ||
| list. It should contain entries in form of "<function name> <parameter index>" | ||
| separated by comma. These values are added to the ignore list. For example | ||
| ``strlen 0, strcpy 0, strcpy 1`` adds ``strlen`` and ``strcpy`` (both | ||
| parameters) to the ignore list. A function name can be used at most 2 times | ||
| (with different parameter values). Default value of the option is an empty | ||
| string. |
There was a problem hiding this comment.
If you do keep the logic that parameters are excluded separately, then please get rid of the limitation that "a function name can be used at most 2 times" -- arbitrary constraints this have no place in modern software.
(It would be somewhat understandable in a project written in C, but here you can easily use SmallVector instead of std::pair so there is no reason to limit the number of arguments that can be specified.)
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.
A new checker for checking if terminating zero is missing from a string. There is an existing
unix.cstring.NotNullTerminatedchecker that looks similar but checks just if a non-string like object is passed to a function. The new checker tests if really the terminating zero is missing. This is the initial version, it can be improved to handle string (or memory) copy functions.