Skip to content

Commit ce8017e

Browse files
committed
[clang-tidy] Add bugprone-suspicious-stringview-data-usage check
This check identifies suspicious usages of std::string_view::data() that could lead to reading out-of-bounds data due to inadequate or incorrect string null termination.
1 parent 051e910 commit ce8017e

File tree

9 files changed

+263
-0
lines changed

9 files changed

+263
-0
lines changed

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@
7272
#include "SuspiciousReallocUsageCheck.h"
7373
#include "SuspiciousSemicolonCheck.h"
7474
#include "SuspiciousStringCompareCheck.h"
75+
#include "SuspiciousStringviewDataUsageCheck.h"
7576
#include "SwappedArgumentsCheck.h"
7677
#include "SwitchMissingDefaultCaseCheck.h"
7778
#include "TerminatingContinueCheck.h"
@@ -217,6 +218,8 @@ class BugproneModule : public ClangTidyModule {
217218
"bugprone-suspicious-semicolon");
218219
CheckFactories.registerCheck<SuspiciousStringCompareCheck>(
219220
"bugprone-suspicious-string-compare");
221+
CheckFactories.registerCheck<SuspiciousStringviewDataUsageCheck>(
222+
"bugprone-suspicious-stringview-data-usage");
220223
CheckFactories.registerCheck<SwappedArgumentsCheck>(
221224
"bugprone-swapped-arguments");
222225
CheckFactories.registerCheck<TerminatingContinueCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ add_clang_library(clangTidyBugproneModule
2626
ImplicitWideningOfMultiplicationResultCheck.cpp
2727
InaccurateEraseCheck.cpp
2828
IncorrectEnableIfCheck.cpp
29+
SuspiciousStringviewDataUsageCheck.cpp
2930
SwitchMissingDefaultCaseCheck.cpp
3031
IncDecInConditionsCheck.cpp
3132
IncorrectRoundingsCheck.cpp
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
//===--- SuspiciousStringviewDataUsageCheck.cpp - clang-tidy --------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "SuspiciousStringviewDataUsageCheck.h"
10+
#include "../utils/Matchers.h"
11+
#include "../utils/OptionsUtils.h"
12+
#include "clang/AST/ASTContext.h"
13+
#include "clang/ASTMatchers/ASTMatchFinder.h"
14+
15+
using namespace clang::ast_matchers;
16+
17+
namespace clang::tidy::bugprone {
18+
19+
SuspiciousStringviewDataUsageCheck::SuspiciousStringviewDataUsageCheck(
20+
StringRef Name, ClangTidyContext *Context)
21+
: ClangTidyCheck(Name, Context),
22+
StringViewTypes(utils::options::parseStringList(Options.get(
23+
"StringViewTypes", "::std::basic_string_view;::llvm::StringRef"))),
24+
AllowedCallees(
25+
utils::options::parseStringList(Options.get("AllowedCallees", ""))) {}
26+
27+
void SuspiciousStringviewDataUsageCheck::storeOptions(
28+
ClangTidyOptions::OptionMap &Opts) {
29+
Options.store(Opts, "StringViewTypes",
30+
utils::options::serializeStringList(StringViewTypes));
31+
Options.store(Opts, "AllowedCallees",
32+
utils::options::serializeStringList(AllowedCallees));
33+
}
34+
35+
bool SuspiciousStringviewDataUsageCheck::isLanguageVersionSupported(
36+
const LangOptions &LangOpts) const {
37+
return LangOpts.CPlusPlus;
38+
}
39+
40+
std::optional<TraversalKind>
41+
SuspiciousStringviewDataUsageCheck::getCheckTraversalKind() const {
42+
return TK_AsIs;
43+
}
44+
45+
void SuspiciousStringviewDataUsageCheck::registerMatchers(MatchFinder *Finder) {
46+
47+
auto AncestorCall = anyOf(
48+
cxxConstructExpr(), callExpr(unless(cxxOperatorCallExpr())), lambdaExpr(),
49+
initListExpr(
50+
hasType(qualType(hasCanonicalType(hasDeclaration(recordDecl()))))));
51+
52+
auto DataMethod =
53+
cxxMethodDecl(hasName("data"),
54+
ofClass(matchers::matchesAnyListedName(StringViewTypes)));
55+
56+
auto DataWithSelfCall =
57+
cxxMemberCallExpr(on(ignoringParenImpCasts(expr().bind("self"))),
58+
callee(DataMethod))
59+
.bind("data-call");
60+
auto SizeCall = cxxMemberCallExpr(
61+
callee(cxxMethodDecl(hasAnyName("size", "length"))),
62+
on(ignoringParenImpCasts(
63+
matchers::isStatementIdenticalToBoundNode("self"))));
64+
65+
Finder->addMatcher(
66+
cxxMemberCallExpr(
67+
on(ignoringParenImpCasts(expr().bind("self"))), callee(DataMethod),
68+
expr().bind("data-call"),
69+
hasParent(expr(anyOf(
70+
invocation(
71+
expr().bind("call"), unless(cxxOperatorCallExpr()),
72+
hasAnyArgument(
73+
ignoringParenImpCasts(equalsBoundNode("data-call"))),
74+
unless(hasAnyArgument(ignoringParenImpCasts(SizeCall))),
75+
unless(hasAnyArgument(hasDescendant(expr(
76+
SizeCall,
77+
hasAncestor(expr(AncestorCall).bind("ancestor-size")),
78+
hasAncestor(expr(equalsBoundNode("call"),
79+
equalsBoundNode("ancestor-size"))))))),
80+
hasDeclaration(namedDecl(
81+
unless(matchers::matchesAnyListedName(AllowedCallees))))),
82+
initListExpr(expr().bind("init"),
83+
hasType(qualType(hasCanonicalType(hasDeclaration(
84+
recordDecl(unless(matchers::matchesAnyListedName(
85+
AllowedCallees))))))),
86+
unless(has(ignoringParenImpCasts(SizeCall)))))))),
87+
this);
88+
}
89+
90+
void SuspiciousStringviewDataUsageCheck::check(
91+
const MatchFinder::MatchResult &Result) {
92+
const auto *DataCallExpr =
93+
Result.Nodes.getNodeAs<CXXMemberCallExpr>("data-call");
94+
diag(DataCallExpr->getExprLoc(),
95+
"result of a `data()` call may not be null terminated, provide size "
96+
"information to the callee to prevent potential issues")
97+
<< DataCallExpr->getCallee()->getSourceRange();
98+
}
99+
100+
} // namespace clang::tidy::bugprone
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
//===--- SuspiciousStringviewDataUsageCheck.h - clang-tidy -------//C++ -*-===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSSTRINGVIEWDATAUSAGECHECK_H
10+
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSSTRINGVIEWDATAUSAGECHECK_H
11+
12+
#include "../ClangTidyCheck.h"
13+
14+
namespace clang::tidy::bugprone {
15+
16+
/// Identifies suspicious usages of std::string_view::data() that could lead to
17+
/// reading out-of-bounds data due to inadequate or incorrect string null
18+
/// termination.
19+
///
20+
/// For the user-facing documentation see:
21+
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/suspicious-stringview-data-usage.html
22+
class SuspiciousStringviewDataUsageCheck : public ClangTidyCheck {
23+
public:
24+
SuspiciousStringviewDataUsageCheck(StringRef Name, ClangTidyContext *Context);
25+
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
26+
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
27+
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
28+
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
29+
std::optional<TraversalKind> getCheckTraversalKind() const override;
30+
31+
private:
32+
std::vector<llvm::StringRef> StringViewTypes;
33+
std::vector<llvm::StringRef> AllowedCallees;
34+
};
35+
36+
} // namespace clang::tidy::bugprone
37+
38+
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSSTRINGVIEWDATAUSAGECHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,13 @@ Improvements to clang-tidy
104104
New checks
105105
^^^^^^^^^^
106106

107+
- New :doc:`bugprone-suspicious-stringview-data-usage
108+
<clang-tidy/checks/bugprone/suspicious-stringview-data-usage>` check.
109+
110+
Identifies suspicious usages of ``std::string_view::data()`` that could lead
111+
to reading out-of-bounds data due to inadequate or incorrect string null
112+
termination.
113+
107114
- New :doc:`modernize-use-designated-initializers
108115
<clang-tidy/checks/modernize/use-designated-initializers>` check.
109116

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
.. title:: clang-tidy - bugprone-suspicious-stringview-data-usage
2+
3+
bugprone-suspicious-stringview-data-usage
4+
=========================================
5+
6+
Identifies suspicious usages of ``std::string_view::data()`` that could lead to
7+
reading out-of-bounds data due to inadequate or incorrect string null
8+
termination.
9+
10+
It warns when the result of ``data()`` is passed to a constructor or function
11+
without also passing the corresponding result of ``size()`` or ``length()``
12+
member function. Such usage can lead to unintended behavior, particularly when
13+
assuming the data pointed to by ``data()`` is null-terminated.
14+
15+
The absence of a ``c_str()`` method in ``std::string_view`` often leads
16+
developers to use ``data()`` as a substitute, especially when interfacing with
17+
C APIs that expect null-terminated strings. However, since ``data()`` does not
18+
guarantee null termination, this can result in unintended behavior if the API
19+
relies on proper null termination for correct string interpretation.
20+
21+
In today's programming landscape, this scenario can occur when implicitly
22+
converting an ``std::string_view`` to an ``std::string``. Since the constructor
23+
in ``std::string`` designed for string-view-like objects is ``explicit``,
24+
attempting to pass an ``std::string_view`` to a function expecting an
25+
``std::string`` will result in a compilation error. As a workaround, developers
26+
may be tempted to utilize the ``.data()`` method to achieve compilation,
27+
introducing potential risks.
28+
29+
For instance:
30+
31+
.. code-block:: c++
32+
33+
void printString(const std::string& str) {
34+
std::cout << "String: " << str << std::endl;
35+
}
36+
37+
void something(std::string_view sv) {
38+
printString(sv.data());
39+
}
40+
41+
In this example, directly passing ``sv`` to the ``printString`` function would
42+
lead to a compilation error due to the explicit nature of the ``std::string``
43+
constructor. Consequently, developers might opt for ``sv.data()`` to resolve the
44+
compilation error, albeit introducing potential hazards as discussed.
45+
46+
.. option:: StringViewTypes
47+
48+
Option allows users to specify custom string view-like types for analysis. It
49+
accepts a semicolon-separated list of type names or regular expressions
50+
matching these types. Default value is:
51+
`::std::basic_string_view;::llvm::StringRef`.
52+
53+
.. option:: AllowedCallees
54+
55+
Specifies methods, functions, or classes where the result of ``.data()`` is
56+
passed to. Allows to exclude such calls from the analysis. Accepts a
57+
semicolon-separated list of names or regular expressions matching these
58+
entities. Default value is: empty string.

clang-tools-extra/docs/clang-tidy/checks/list.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ Clang-Tidy Checks
138138
:doc:`bugprone-suspicious-realloc-usage <bugprone/suspicious-realloc-usage>`,
139139
:doc:`bugprone-suspicious-semicolon <bugprone/suspicious-semicolon>`, "Yes"
140140
:doc:`bugprone-suspicious-string-compare <bugprone/suspicious-string-compare>`, "Yes"
141+
:doc:`bugprone-suspicious-stringview-data-usage <bugprone/suspicious-stringview-data-usage>`,
141142
:doc:`bugprone-swapped-arguments <bugprone/swapped-arguments>`, "Yes"
142143
:doc:`bugprone-switch-missing-default-case <bugprone/switch-missing-default-case>`,
143144
:doc:`bugprone-terminating-continue <bugprone/terminating-continue>`, "Yes"

clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ struct basic_string {
2424
basic_string();
2525
basic_string(const C *p, const A &a = A());
2626
basic_string(const C *p, size_type count);
27+
basic_string(const C *b, const C *e);
2728

2829
~basic_string();
2930

@@ -85,6 +86,12 @@ struct basic_string_view {
8586
const C *str;
8687
constexpr basic_string_view(const C* s) : str(s) {}
8788

89+
const C *data() const;
90+
91+
bool empty() const;
92+
size_type size() const;
93+
size_type length() const;
94+
8895
size_type find(_Type v, size_type pos = 0) const;
8996
size_type find(C ch, size_type pos = 0) const;
9097
size_type find(const C* s, size_type pos, size_type count) const;
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-suspicious-stringview-data-usage %t -- -- -isystem %clang_tidy_headers
2+
#include <string>
3+
4+
struct View {
5+
const char* str;
6+
};
7+
8+
struct ViewWithSize {
9+
const char* str;
10+
std::string_view::size_type size;
11+
};
12+
13+
void something(const char*);
14+
void something(const char*, unsigned);
15+
void something(const char*, unsigned, const char*);
16+
void something_str(std::string, unsigned);
17+
18+
void invalid(std::string_view sv, std::string_view sv2) {
19+
std::string s(sv.data());
20+
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: result of a `data()` call may not be null terminated, provide size information to the callee to prevent potential issues
21+
std::string si{sv.data()};
22+
// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: result of a `data()` call may not be null terminated, provide size information to the callee to prevent potential issues
23+
std::string_view s2(sv.data());
24+
// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: result of a `data()` call may not be null terminated, provide size information to the callee to prevent potential issues
25+
something(sv.data());
26+
// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: result of a `data()` call may not be null terminated, provide size information to the callee to prevent potential issues
27+
something(sv.data(), sv.size(), sv2.data());
28+
// CHECK-MESSAGES: :[[@LINE-1]]:39: warning: result of a `data()` call may not be null terminated, provide size information to the callee to prevent potential issues
29+
something_str(sv.data(), sv.size());
30+
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: result of a `data()` call may not be null terminated, provide size information to the callee to prevent potential issues
31+
View view{sv.data()};
32+
// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: result of a `data()` call may not be null terminated, provide size information to the callee to prevent potential issues
33+
}
34+
35+
void valid(std::string_view sv) {
36+
std::string s1(sv.data(), sv.data() + sv.size());
37+
std::string s2(sv.data(), sv.data() + sv.length());
38+
std::string s3(sv.data(), sv.size() + sv.data());
39+
std::string s4(sv.data(), sv.length() + sv.data());
40+
std::string s5(sv.data(), sv.size());
41+
std::string s6(sv.data(), sv.length());
42+
something(sv.data(), sv.size());
43+
something(sv.data(), sv.length());
44+
ViewWithSize view1{sv.data(), sv.size()};
45+
ViewWithSize view2{sv.data(), sv.length()};
46+
47+
const char* str{sv.data()};
48+
}

0 commit comments

Comments
 (0)