Skip to content

Commit

Permalink
[clang-tidy] Added support for 3-argument std::string ctor in bugpron…
Browse files Browse the repository at this point in the history 
…e-string-constructor check (llvm#123413)

This PR add diagnostics for 3-parameter `std::basic_string(const char*
t, size_type pos, size_type count)` constructor in
bugprone-string-constructor check:

```cpp
  std::string r1("test", 1, 0); // constructor creating an empty string
  std::string r2("test", 0, -4); // negative value used as length parameter
  // more examples in test file
  ```

Fixes false-positives reported in llvm#123198.
  • Loading branch information
@vbvictor
vbvictor authored Feb 11, 2025
1 parent 9cc7ee1 commit 0b922d6
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 5 deletions.
50 changes: 45 additions & 5 deletions clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void StringConstructorCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
cxxConstructExpr(
hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
hasArgument(0, hasType(qualType(isInteger()))),
argumentCountIs(2), hasArgument(0, hasType(qualType(isInteger()))),
hasArgument(1, hasType(qualType(isInteger()))),
anyOf(
// Detect the expression: string('x', 40);
Expand All @@ -102,7 +102,7 @@ void StringConstructorCheck::registerMatchers(MatchFinder *Finder) {
cxxConstructExpr(
hasDeclaration(cxxConstructorDecl(ofClass(
cxxRecordDecl(hasAnyName(removeNamespaces(StringNames)))))),
hasArgument(0, hasType(CharPtrType)),
argumentCountIs(2), hasArgument(0, hasType(CharPtrType)),
hasArgument(1, hasType(isInteger())),
anyOf(
// Detect the expression: string("...", 0);
Expand All @@ -114,7 +114,34 @@ void StringConstructorCheck::registerMatchers(MatchFinder *Finder) {
// Detect the expression: string("lit", 5)
allOf(hasArgument(0, ConstStrLiteral.bind("literal-with-length")),
hasArgument(1, ignoringParenImpCasts(
integerLiteral().bind("int"))))))
integerLiteral().bind("length"))))))
.bind("constructor"),
this);

// Check the literal string constructor with char pointer, start position and
// length parameters. [i.e. string (const char* s, size_t pos, size_t count);]
Finder->addMatcher(
cxxConstructExpr(
hasDeclaration(cxxConstructorDecl(ofClass(
cxxRecordDecl(hasAnyName(removeNamespaces(StringNames)))))),
argumentCountIs(3), hasArgument(0, hasType(CharPtrType)),
hasArgument(1, hasType(qualType(isInteger()))),
hasArgument(2, hasType(qualType(isInteger()))),
anyOf(
// Detect the expression: string("...", 1, 0);
hasArgument(2, ZeroExpr.bind("empty-string")),
// Detect the expression: string("...", -4, 1);
hasArgument(1, NegativeExpr.bind("negative-pos")),
// Detect the expression: string("...", 0, -4);
hasArgument(2, NegativeExpr.bind("negative-length")),
// Detect the expression: string("lit", 0, 0x1234567);
hasArgument(2, LargeLengthExpr.bind("large-length")),
// Detect the expression: string("lit", 1, 5)
allOf(hasArgument(0, ConstStrLiteral.bind("literal-with-length")),
hasArgument(
1, ignoringParenImpCasts(integerLiteral().bind("pos"))),
hasArgument(2, ignoringParenImpCasts(
integerLiteral().bind("length"))))))
.bind("constructor"),
this);

Expand Down Expand Up @@ -155,14 +182,27 @@ void StringConstructorCheck::check(const MatchFinder::MatchResult &Result) {
diag(Loc, "constructor creating an empty string");
} else if (Result.Nodes.getNodeAs<Expr>("negative-length")) {
diag(Loc, "negative value used as length parameter");
} else if (Result.Nodes.getNodeAs<Expr>("negative-pos")) {
diag(Loc, "negative value used as position of the "
"first character parameter");
} else if (Result.Nodes.getNodeAs<Expr>("large-length")) {
if (WarnOnLargeLength)
diag(Loc, "suspicious large length parameter");
} else if (Result.Nodes.getNodeAs<Expr>("literal-with-length")) {
const auto *Str = Result.Nodes.getNodeAs<StringLiteral>("str");
const auto *Lit = Result.Nodes.getNodeAs<IntegerLiteral>("int");
if (Lit->getValue().ugt(Str->getLength())) {
const auto *Length = Result.Nodes.getNodeAs<IntegerLiteral>("length");
if (Length->getValue().ugt(Str->getLength())) {
diag(Loc, "length is bigger than string literal size");
return;
}
if (const auto *Pos = Result.Nodes.getNodeAs<IntegerLiteral>("pos")) {
if (Pos->getValue().uge(Str->getLength())) {
diag(Loc, "position of the first character parameter is bigger than "
"string literal character range");
} else if (Length->getValue().ugt(
(Str->getLength() - Pos->getValue()).getZExtValue())) {
diag(Loc, "length is bigger than remaining string literal size");
}
}
} else if (const auto *Ptr = Result.Nodes.getNodeAs<Expr>("from-ptr")) {
Expr::EvalResult ConstPtr;
Expand Down
5 changes: 5 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ New check aliases
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^

- Improved :doc:`bugprone-string-constructor
<clang-tidy/checks/bugprone/string-constructor>` check to find suspicious
calls of ``std::string`` constructor with char pointer, start position and
length parameters.

- Improved :doc:`bugprone-unsafe-functions
<clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying
additional C++ member functions to match.
Expand Down
12 changes: 12 additions & 0 deletions clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Examples:
.. code-block:: c++

std::string("test", 200); // Will include random characters after "test".
std::string("test", 2, 5); // Will include random characters after "st".
std::string_view("test", 200);

Creating an empty string from constructors with parameters is considered
Expand All @@ -31,8 +32,19 @@ Examples:
.. code-block:: c++

std::string("test", 0); // Creation of an empty string.
std::string("test", 1, 0);
std::string_view("test", 0);

Passing an invalid first character position parameter to constructor will
cause ``std::out_of_range`` exception at runtime.

Examples:

.. code-block:: c++

std::string("test", -1, 10); // Negative first character position.
std::string("test", 10, 10); // First character position is bigger than string literal character range".

Options
-------

Expand Down
30 changes: 30 additions & 0 deletions clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ struct basic_string {
basic_string(const C*, unsigned int size);
basic_string(const C *, const A &allocator = A());
basic_string(unsigned int size, C c);
basic_string(const C*, unsigned int pos, unsigned int size);
};
typedef basic_string<char> string;
typedef basic_string<wchar_t> wstring;
Expand Down Expand Up @@ -61,6 +62,21 @@ void Test() {
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructing string from nullptr is undefined behaviour
std::string q7 = 0;
// CHECK-MESSAGES: [[@LINE-1]]:20: warning: constructing string from nullptr is undefined behaviour

std::string r1("test", 1, 0);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor creating an empty string
std::string r2("test", 0, -4);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: negative value used as length parameter
std::string r3("test", -4, 1);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: negative value used as position of the first character parameter
std::string r4("test", 0, 0x1000000);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: suspicious large length parameter
std::string r5("test", 0, 5);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger than string literal size
std::string r6("test", 3, 2);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger than remaining string literal size
std::string r7("test", 4, 1);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: position of the first character parameter is bigger than string literal character range
}

void TestView() {
Expand All @@ -82,6 +98,17 @@ void TestView() {
// CHECK-MESSAGES: [[@LINE-1]]:25: warning: constructing string from nullptr is undefined behaviour
}

void TestUnsignedArguments() {
std::string s0("test", 0u);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor creating an empty string
std::string s1(0x1000000ull, 'x');
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: suspicious large length parameter
std::string s2("test", 3ull, 2u);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger than remaining string literal size
std::string s3("test", 0u, 5ll);
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger than string literal size
}

std::string StringFromZero() {
return 0;
// CHECK-MESSAGES: [[@LINE-1]]:10: warning: constructing string from nullptr is undefined behaviour
Expand All @@ -101,6 +128,9 @@ void Valid() {
std::string s3("test");
std::string s4("test\000", 5);
std::string s6("te" "st", 4);
std::string s7("test", 0, 4);
std::string s8("test", 3, 1);
std::string s9("te" "st", 1, 2);

std::string_view emptyv();
std::string_view sv1("test", 4);
Expand Down

0 comments on commit 0b922d6

Please sign in to comment.