Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,11 @@ class UnsafeBufferUsageHandler {
bool IsRelatedToDecl, ASTContext &Ctx) {
handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx);
}

virtual void handleUnsafeCountAttributedPointerAssignment(
const BinaryOperator *Assign, bool IsRelatedToDecl, ASTContext &Ctx) {
handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx);
}
/* TO_UPSTREAM(BoundsSafety) OFF */

/// Invoked when a fix is suggested against a variable. This function groups
Expand Down
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -14296,6 +14296,9 @@ def warn_duplicated_assignment_in_bounds_attributed_group : Warning<
def warn_assigned_and_used_in_bounds_attributed_group : Warning<
"%select{variable|parameter|member}0 %1 is assigned and used in the same "
"bounds-attributed group">, InGroup<UnsafeBufferUsage>, DefaultIgnore;
def warn_unsafe_count_attributed_pointer_assignment : Warning<
"unsafe assignment to count-attributed pointer">,
InGroup<UnsafeBufferUsage>, DefaultIgnore;
#ifndef NDEBUG
// Not a user-facing diagnostic. Useful for debugging false negatives in
// -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits).
Expand Down
57 changes: 55 additions & 2 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6020,6 +6020,60 @@ static bool checkAssignedAndUsed(const BoundsAttributedAssignmentGroup &Group,
return IsGroupSafe;
}

// Checks if each assignment to count-attributed pointer in the group is safe.
static bool
checkAssignmentPatterns(const BoundsAttributedAssignmentGroup &Group,
UnsafeBufferUsageHandler &Handler, ASTContext &Ctx) {
// Collect dependent values.
DependentValuesTy DependentValues;
for (size_t I = 0, N = Group.AssignedObjects.size(); I < N; ++I) {
const ValueDecl *VD = Group.AssignedObjects[I].Decl;
const auto *Attr = VD->getAttr<DependerDeclsAttr>();
if (!Attr)
continue;

const BinaryOperator *Assign = Group.Assignments[I];
const Expr *Value = Assign->getRHS();

[[maybe_unused]] bool Inserted =
DependentValues.insert({{VD, Attr->getIsDeref()}, Value}).second;
// Previous checks in `checkBoundsAttributedGroup` should have validated
// that we have only a single assignment.
assert(Inserted);
}

bool IsGroupSafe = true;

// Check every pointer in the group.
for (size_t I = 0, N = Group.AssignedObjects.size(); I < N; ++I) {
const ValueDecl *VD = Group.AssignedObjects[I].Decl;

QualType Ty = VD->getType();
const auto *CAT = Ty->getAs<CountAttributedType>();
if (!CAT && Ty->isPointerType())
CAT = Ty->getPointeeType()->getAs<CountAttributedType>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have tests covering this case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, those cases happen when we have a dereference on LHS (*ptr = ). We have tests like this:

void good_inout_span(int *__counted_by(*count) *p, size_t *count, std::span<int> sp) {
  *p = sp.data();
  *count = sp.size();
}

if (!CAT)
continue;

const BinaryOperator *Assign = Group.Assignments[I];

// TODO: Move this logic to isCountAttributedPointerArgumentSafeImpl.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the parameters of isCountAttributedPointerArgumentSafeImpl aren't the most intuitive. Technically, we do not need the caller to pass CountArg because the count expression with DependentValues already contain that information.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take care of this TODO in my refactoring patch.

const Expr *CountArg =
DependentValues.size() == 1 ? DependentValues.begin()->second : nullptr;

bool IsSafe = isCountAttributedPointerArgumentSafeImpl(
Ctx, Assign->getRHS(), CountArg, CAT, CAT->getCountExpr(),
CAT->isCountInBytes(), CAT->isOrNull(), &DependentValues);
if (!IsSafe) {
Handler.handleUnsafeCountAttributedPointerAssignment(
Assign, /*IsRelatedToDecl=*/false, Ctx);
IsGroupSafe = false;
}
}

return IsGroupSafe;
}

// Checks if the bounds-attributed group is safe. This function returns false
// iff the assignment group is unsafe and diagnostics have been emitted.
static bool
Expand All @@ -6031,8 +6085,7 @@ checkBoundsAttributedGroup(const BoundsAttributedAssignmentGroup &Group,
return false;
if (!checkAssignedAndUsed(Group, Handler, Ctx))
return false;
// TODO: Add more checks.
return true;
return checkAssignmentPatterns(Group, Handler, Ctx);
}

static void
Expand Down
7 changes: 7 additions & 0 deletions clang/lib/Sema/AnalysisBasedWarnings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2695,6 +2695,13 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
<< getBoundsAttributedObjectKind(VD) << VD;
S.Diag(Use->getBeginLoc(), diag::note_used_here);
}

void handleUnsafeCountAttributedPointerAssignment(
const BinaryOperator *Assign, [[maybe_unused]] bool IsRelatedToDecl,
[[maybe_unused]] ASTContext &Ctx) override {
S.Diag(Assign->getOperatorLoc(),
diag::warn_unsafe_count_attributed_pointer_assignment);
}
/* TO_UPSTREAM(BoundsSafety) OFF */

void handleUnsafeVariableGroup(const VarDecl *Variable,
Expand Down
Loading