Skip to content

Commit

Permalink
[-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas
Browse files Browse the repository at this point in the history
Add a pair of clang pragmas:
- `#pragma clang unsafe_buffer_usage begin` and
- `#pragma clang unsafe_buffer_usage end`,
which specify the start and end of an (unsafe buffer checking) opt-out
region, respectively.

Behaviors of opt-out regions conform to the following rules:

- No nested nor overlapped opt-out regions are allowed. One cannot
  start an opt-out region with `... unsafe_buffer_usage begin` but never
  close it with `... unsafe_buffer_usage end`. Mis-use of the pragmas
  will be warned.
- Warnings raised from unsafe buffer operations inside such an opt-out
  region will always be suppressed. This behavior CANNOT be changed by
  `clang diagnostic` pragmas or command-line flags.
- Warnings raised from unsafe operations outside of such opt-out
  regions may be reported on declarations inside opt-out
  regions. These warnings are NOT suppressed.
- An un-suppressed unsafe operation warning may be attached with
  notes. These notes are NOT suppressed as well regardless of whether
  they are in opt-out regions.

The implementation maintains a separate sequence of location pairs
representing opt-out regions in `Preprocessor`.  The `UnsafeBufferUsage`
analyzer reads the region sequence to check if an unsafe operation is
in an opt-out region. If it is, discard the warning raised from the
operation immediately.

Reviewed by: NoQ

Differential revision: https://reviews.llvm.org/D140179
  • Loading branch information
ziqingluo-90 committed Feb 8, 2023
1 parent 233fd47 commit aef05b5
Show file tree
Hide file tree
Showing 12 changed files with 435 additions and 3 deletions.
3 changes: 3 additions & 0 deletions clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ class UnsafeBufferUsageHandler {
s += " #>";
return s;
}

/// Returns a reference to the `Preprocessor`:
virtual const Preprocessor & getPP() const;
};

// This function invokes the analysis and allows the caller to react to it
Expand Down
11 changes: 11 additions & 0 deletions clang/include/clang/Basic/DiagnosticLexKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -945,4 +945,15 @@ def err_dep_source_scanner_unexpected_tokens_at_import : Error<

}

def err_pp_double_begin_pragma_unsafe_buffer_usage :
Error<"already inside '#pragma unsafe_buffer_usage'">;

def err_pp_unmatched_end_begin_pragma_unsafe_buffer_usage :
Error<"not currently inside '#pragma unsafe_buffer_usage'">;

def err_pp_unclosed_pragma_unsafe_buffer_usage :
Error<"'#pragma unsafe_buffer_usage' was not ended">;

def err_pp_pragma_unsafe_buffer_usage_syntax :
Error<"Expected 'begin' or 'end'">;
}
45 changes: 45 additions & 0 deletions clang/include/clang/Lex/Preprocessor.h
Original file line number Diff line number Diff line change
Expand Up @@ -2688,6 +2688,51 @@ class Preprocessor {
void emitMacroDeprecationWarning(const Token &Identifier) const;
void emitRestrictExpansionWarning(const Token &Identifier) const;
void emitFinalMacroWarning(const Token &Identifier, bool IsUndef) const;

/// This boolean state keeps track if the current scanned token (by this PP)
/// is in an "-Wunsafe-buffer-usage" opt-out region. Assuming PP scans a
/// translation unit in a linear order.
bool InSafeBufferOptOutRegion = 0;

/// Hold the start location of the current "-Wunsafe-buffer-usage" opt-out
/// region if PP is currently in such a region. Hold undefined value
/// otherwise.
SourceLocation CurrentSafeBufferOptOutStart; // It is used to report the start location of an never-closed region.

// An ordered sequence of "-Wunsafe-buffer-usage" opt-out regions in one
// translation unit. Each region is represented by a pair of start and end
// locations. A region is "open" if its' start and end locations are
// identical.
SmallVector<std::pair<SourceLocation, SourceLocation>, 8> SafeBufferOptOutMap;

public:
/// \return true iff the given `Loc` is in a "-Wunsafe-buffer-usage" opt-out
/// region. This `Loc` must be a source location that has been pre-processed.
bool isSafeBufferOptOut(const SourceManager&SourceMgr, const SourceLocation &Loc) const;

/// Alter the state of whether this PP currently is in a
/// "-Wunsafe-buffer-usage" opt-out region.
///
/// \param isEnter: true if this PP is entering a region; otherwise, this PP
/// is exiting a region
/// \param Loc: the location of the entry or exit of a
/// region
/// \return true iff it is INVALID to enter or exit a region, i.e.,
/// attempt to enter a region before exiting a previous region, or exiting a
/// region that PP is not currently in.
bool enterOrExitSafeBufferOptOutRegion(bool isEnter,
const SourceLocation &Loc);

/// \return true iff this PP is currently in a "-Wunsafe-buffer-usage"
/// opt-out region
bool isPPInSafeBufferOptOutRegion();

/// \param StartLoc: output argument. It will be set to the start location of
/// the current "-Wunsafe-buffer-usage" opt-out region iff this function
/// returns true.
/// \return true iff this PP is currently in a "-Wunsafe-buffer-usage"
/// opt-out region
bool isPPInSafeBufferOptOutRegion(SourceLocation &StartLoc);
};

/// Abstract base class that describes a handler that will receive
Expand Down
14 changes: 11 additions & 3 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/SmallVector.h"
#include <memory>
Expand Down Expand Up @@ -117,6 +118,12 @@ AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher<Stmt>, innerMatcher) {
return Visitor.findMatch(DynTypedNode::create(Node));
}

// Matches a `Stmt` node iff the node is in a safe-buffer opt-out region
AST_MATCHER_P(Stmt, notInSafeBufferOptOut, const Preprocessor *, PP) {
const SourceManager &SM = Finder->getASTContext().getSourceManager();
return !PP->isSafeBufferOptOut(SM, Node.getBeginLoc());
}

AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
}
Expand Down Expand Up @@ -548,7 +555,8 @@ class Strategy {
} // namespace

/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker> findGadgets(const Decl *D) {
static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker>
findGadgets(const Decl *D, const Preprocessor &PP) {

struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
Expand Down Expand Up @@ -620,7 +628,7 @@ static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker> findGadg
stmt(anyOf(
// Add Gadget::matcher() for every gadget in the registry.
#define WARNING_GADGET(x) \
x ## Gadget::matcher().bind(#x),
allOf(x ## Gadget::matcher().bind(#x), notInSafeBufferOptOut(&PP)),
#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
// In parallel, match all DeclRefExprs so that to find out
// whether there are any uncovered by gadgets.
Expand Down Expand Up @@ -1009,7 +1017,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
DeclUseTracker Tracker;

{
auto [FixableGadgets, WarningGadgets, TrackerRes] = findGadgets(D);
auto [FixableGadgets, WarningGadgets, TrackerRes] = findGadgets(D, Handler.getPP());
UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
FixablesForUnsafeVars = groupFixablesByVar(std::move(FixableGadgets));
Tracker = std::move(TrackerRes);
Expand Down
9 changes: 9 additions & 0 deletions clang/lib/Lex/PPLexerChange.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,15 @@ bool Preprocessor::HandleEndOfFile(Token &Result, bool isEndOfMacro) {
assert(!CurTokenLexer &&
"Ending a file when currently in a macro!");

SourceLocation UnclosedSafeBufferOptOutLoc;

if (IncludeMacroStack.empty() &&
isPPInSafeBufferOptOutRegion(UnclosedSafeBufferOptOutLoc)) {
// To warn if a "-Wunsafe-buffer-usage" opt-out region is still open by the
// end of a file.
Diag(UnclosedSafeBufferOptOutLoc,
diag::err_pp_unclosed_pragma_unsafe_buffer_usage);
}
// If we have an unclosed module region from a pragma at the end of a
// module, complain and close it now.
const bool LeavingSubmodule = CurLexer && CurLexerSubmodule;
Expand Down
29 changes: 29 additions & 0 deletions clang/lib/Lex/Pragma.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1243,6 +1243,32 @@ struct PragmaDebugHandler : public PragmaHandler {
#endif
};

struct PragmaUnsafeBufferUsageHandler : public PragmaHandler {
PragmaUnsafeBufferUsageHandler() : PragmaHandler("unsafe_buffer_usage") {}
void HandlePragma(Preprocessor &PP, PragmaIntroducer Introducer,
Token &FirstToken) override {
Token Tok;

PP.LexUnexpandedToken(Tok);
if (Tok.isNot(tok::identifier)) {
PP.Diag(Tok, diag::err_pp_pragma_unsafe_buffer_usage_syntax);
return;
}

IdentifierInfo *II = Tok.getIdentifierInfo();
SourceLocation Loc = Tok.getLocation();

if (II->isStr("begin")) {
if (PP.enterOrExitSafeBufferOptOutRegion(true, Loc))
PP.Diag(Loc, diag::err_pp_double_begin_pragma_unsafe_buffer_usage);
} else if (II->isStr("end")) {
if (PP.enterOrExitSafeBufferOptOutRegion(false, Loc))
PP.Diag(Loc, diag::err_pp_unmatched_end_begin_pragma_unsafe_buffer_usage);
} else
PP.Diag(Tok, diag::err_pp_pragma_unsafe_buffer_usage_syntax);
}
};

/// PragmaDiagnosticHandler - e.g. '\#pragma GCC diagnostic ignored "-Wformat"'
struct PragmaDiagnosticHandler : public PragmaHandler {
private:
Expand Down Expand Up @@ -2128,6 +2154,9 @@ void Preprocessor::RegisterBuiltinPragmas() {
ModuleHandler->AddPragma(new PragmaModuleBuildHandler());
ModuleHandler->AddPragma(new PragmaModuleLoadHandler());

// Safe Buffers pragmas
AddPragmaHandler("clang", new PragmaUnsafeBufferUsageHandler);

// Add region pragmas.
AddPragmaHandler(new PragmaRegionHandler("region"));
AddPragmaHandler(new PragmaRegionHandler("endregion"));
Expand Down
69 changes: 69 additions & 0 deletions clang/lib/Lex/Preprocessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1462,6 +1462,75 @@ void Preprocessor::emitFinalMacroWarning(const Token &Identifier,
Diag(*A.FinalAnnotationLoc, diag::note_pp_macro_annotation) << 2;
}

bool Preprocessor::isSafeBufferOptOut(const SourceManager &SourceMgr,
const SourceLocation &Loc) const {
// Try to find a region in `SafeBufferOptOutMap` where `Loc` is in:
auto FirstRegionEndingAfterLoc = llvm::partition_point(
SafeBufferOptOutMap,
[&SourceMgr,
&Loc](const std::pair<SourceLocation, SourceLocation> &Region) {
return SourceMgr.isBeforeInTranslationUnit(Region.second, Loc);
});

if (FirstRegionEndingAfterLoc != SafeBufferOptOutMap.end()) {
// To test if the start location of the found region precedes `Loc`:
return SourceMgr.isBeforeInTranslationUnit(FirstRegionEndingAfterLoc->first,
Loc);
}
// If we do not find a region whose end location passes `Loc`, we want to
// check if the current region is still open:
if (!SafeBufferOptOutMap.empty() &&
SafeBufferOptOutMap.back().first == SafeBufferOptOutMap.back().second)
return SourceMgr.isBeforeInTranslationUnit(SafeBufferOptOutMap.back().first,
Loc);
return false;
}

bool Preprocessor::enterOrExitSafeBufferOptOutRegion(
bool isEnter, const SourceLocation &Loc) {
if (isEnter) {
if (isPPInSafeBufferOptOutRegion())
return true; // invalid enter action
InSafeBufferOptOutRegion = true;
CurrentSafeBufferOptOutStart = Loc;

// To set the start location of a new region:

if (!SafeBufferOptOutMap.empty()) {
auto *PrevRegion = &SafeBufferOptOutMap.back();
assert(PrevRegion->first != PrevRegion->second &&
"Shall not begin a safe buffer opt-out region before closing the "
"previous one.");
}
// If the start location equals to the end location, we call the region a
// open region or a unclosed region (i.e., end location has not been set
// yet).
SafeBufferOptOutMap.emplace_back(Loc, Loc);
} else {
if (!isPPInSafeBufferOptOutRegion())
return true; // invalid enter action
InSafeBufferOptOutRegion = false;

// To set the end location of the current open region:

assert(!SafeBufferOptOutMap.empty() &&
"Misordered safe buffer opt-out regions");
auto *CurrRegion = &SafeBufferOptOutMap.back();
assert(CurrRegion->first == CurrRegion->second &&
"Set end location to a closed safe buffer opt-out region");
CurrRegion->second = Loc;
}
return false;
}

bool Preprocessor::isPPInSafeBufferOptOutRegion() {
return InSafeBufferOptOutRegion;
}
bool Preprocessor::isPPInSafeBufferOptOutRegion(SourceLocation &StartLoc) {
StartLoc = CurrentSafeBufferOptOutStart;
return InSafeBufferOptOutRegion;
}

ModuleLoader::~ModuleLoader() = default;

CommentHandler::~CommentHandler() = default;
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/Sema/AnalysisBasedWarnings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2213,6 +2213,10 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
FD << F;
}
}

const clang::Preprocessor & getPP() const override {
return S.getPreprocessor();
}
};
} // namespace

Expand Down
107 changes: 107 additions & 0 deletions clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s

void basic(int * x) {
int tmp;
int *p1 = new int[10]; // no fix
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
int *p2 = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p2"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
#pragma clang unsafe_buffer_usage begin
tmp = p1[5];
#pragma clang unsafe_buffer_usage end
tmp = p2[5];
}

void withDiagnosticWarning() {
int tmp;
int *p1 = new int[10]; // no fix
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
int *p2 = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p2"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"

// diagnostics in opt-out region
#pragma clang unsafe_buffer_usage begin
tmp = p1[5]; // not to warn
tmp = p2[5]; // not to warn
#pragma clang diagnostic push
#pragma clang diagnostic warning "-Wunsafe-buffer-usage"
tmp = p1[5]; // not to warn
tmp = p2[5]; // not to warn
#pragma clang diagnostic warning "-Weverything"
tmp = p1[5]; // not to warn
tmp = p2[5]; // not to warn
#pragma clang diagnostic pop
#pragma clang unsafe_buffer_usage end

// opt-out region under diagnostic warning
#pragma clang diagnostic push
#pragma clang diagnostic warning "-Wunsafe-buffer-usage"
#pragma clang unsafe_buffer_usage begin
tmp = p1[5]; // not to warn
tmp = p2[5]; // not to warn
#pragma clang unsafe_buffer_usage end
#pragma clang diagnostic pop

tmp = p2[5];
}


void withDiagnosticIgnore() {
int tmp;
int *p1 = new int[10];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
int *p2 = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p2"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
int *p3 = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p3"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"

#pragma clang unsafe_buffer_usage begin
tmp = p1[5]; // not to warn
tmp = p2[5]; // not to warn
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunsafe-buffer-usage"
tmp = p1[5]; // not to warn
tmp = p2[5]; // not to warn
#pragma clang diagnostic ignored "-Weverything"
tmp = p1[5]; // not to warn
tmp = p2[5]; // not to warn
#pragma clang diagnostic pop
#pragma clang unsafe_buffer_usage end

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunsafe-buffer-usage"
#pragma clang unsafe_buffer_usage begin
tmp = p1[5]; // not to warn
tmp = p2[5]; // not to warn
#pragma clang unsafe_buffer_usage end
#pragma clang diagnostic pop

tmp = p2[5];

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunsafe-buffer-usage"
#pragma clang unsafe_buffer_usage begin
tmp = p1[5]; // not to warn
tmp = p2[5]; // not to warn
#pragma clang unsafe_buffer_usage end
tmp = p3[5]; // expected-note{{used in buffer access here}}
#pragma clang diagnostic pop
}

void noteGoesWithVarDeclWarning() {
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunsafe-buffer-usage"
int *p = new int[10]; // not to warn
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
#pragma clang diagnostic pop

p[5]; // not to note since the associated warning is suppressed
}
Loading

0 comments on commit aef05b5

Please sign in to comment.