Skip to content

[-Wunsafe-buffer-usage] Check for too complex count-attributed assignments #11524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: next
Choose a base branch
from
Open

[-Wunsafe-buffer-usage] Check for too complex count-attributed assignments #11524

wants to merge 1 commit into from
+473 −3

Conversation

patrykstefanski
Copy link

@patrykstefanski patrykstefanski commented Sep 30, 2025 •
edited
Loading

This is an initial part of an analysis of count-attributed assignment groups. This commit adds an AST visitor that is responsible for finding bounds-attributed assignment groups and assignments to bounds-attributed objects (pointers and dependent counts) that are too complex to verify.

As a PoC, this commit adds checks for too complex assignments, which are assignments that are not directly inside of a compound statement (like other assignment groups) and modify the pointer or count in some way. Our model rejects those and requires the user to simplify their code.

For example:

  void foo(int *__counted_by(count) p, int count) {
    q = p = ...;
          ^ this is rejected
    n = count = ...;
              ^ this is rejected
    // the following is fine:
    p = ...;
    count = ...;
  }

rdar://161607826

@patrykstefanski patrykstefanski self-assigned this Sep 30, 2025
@patrykstefanski patrykstefanski added the clang:bounds-safety Issue relating to the experimental -fbounds-safety feature in Clang label Sep 30, 2025
@patrykstefanski
Copy link
Author

This is taken from the original PR: #11490

@patrykstefanski
Copy link
Author

Copy-paste from the original PR to continue the discussion:

q = p = q;     // warn: assignment to bounds-attributed pointer 'p' must be inside of a bounds-attributed group in a compound statement

I feel like the diagnostic message may confuse users. What is a bounds-attributed group? How about just say

assignment to bounds-attributed pointer 'p' must be a simple statement 'p = RHS;' followed by a simple statement 'count = RHS;'.

When the count expression is a constant, we hide followed by a simple statement 'count = RHS;'.

I agree that my initial diagnostic needs rework. I think your diagnostic is better. Though, does 'p' must be a simple statement 'p = RHS;' imply that { p = RHS; ... } (assignment directly in compound) is fine but q = p = RHS; is not? I feel like we should provide some info that the assignment cannot be nested.

@patrykstefanski patrykstefanski force-pushed the eng/pstefanski/PR-161607826 branch from 3e72540 to 5c2cda7 Compare September 30, 2025 21:54
@patrykstefanski
Copy link
Author

I updated the warning to:

assignment to count-attributed pointer 'p' must be a simple statement 'p = ...'

@ziqingluo-90
Copy link

ziqingluo-90 commented Oct 7, 2025 •
edited
Loading

... Though, does 'p' must be a simple statement 'p = RHS;' imply that { p = RHS; ... } (assignment directly in compound) is fine but q = p = RHS; is not? I feel like we should provide some info that the assignment cannot be nested.

To be pedantic🧐, the p = RHS part in q = p = RHS; is a sub-expression and not a statement, so it is not a "simple assignment statement". On the other hand, we probably could accept p = q = RHS; as a simple assignment statement for p. The q = RHS part maybe problematic but it is irrelevant to p.

But I think people will understand what we mean there unambiguously.
And yes, getting rid of "RHS" in the diagnostic message is a good call.

@ziqingluo-90
Copy link

Thank you @patrykstefanski , I don't have other major concerns.

@patrykstefanski patrykstefanski force-pushed the eng/pstefanski/PR-161607826 branch from 5c2cda7 to 70254e2 Compare October 8, 2025 18:13
@patrykstefanski patrykstefanski changed the title [-Wunsafe-buffer-usage] Check standalone count-attributed assignments [-Wunsafe-buffer-usage] Check for too complex count-attributed assignments Oct 8, 2025
@patrykstefanski
Copy link
Author

@ziqingluo-90 I update the naming in code to reflect that the assignment must be simple or is too complex (instead of 'standalone').

ziqingluo-90
ziqingluo-90 approved these changes Oct 8, 2025
View reviewed changes
clang/lib/Analysis/UnsafeBufferUsage.cpp

static std::optional<BoundsAttributedObject>
getBoundsAttributedObject(const Expr *E) {
E = E->IgnoreParenCasts();
Copy link

Choose a reason for hiding this comment

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

Do you want IgnoreParenImpCasts here?

Copy link
Author

Choose a reason for hiding this comment

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

IgnoreParenCasts lets us see through explicit casts as well, which in turn lets us detect more assignments to bounds-attributed objects.

@patrykstefanski patrykstefanski force-pushed the eng/pstefanski/PR-161607826 branch from 70254e2 to e0ee8ec Compare October 8, 2025 23:35
@patrykstefanski
Copy link
Author

@swift-ci test llvm

@patrykstefanski
[-Wunsafe-buffer-usage] Check for too complex count-attributed assign…
ed31544 
…ments

This is an initial part of an analysis of count-attributed assignment
groups. This commit adds an AST visitor that is responsible for finding
bounds-attributed assignment groups and assignments to bounds-attributed
objects (pointers and dependent counts) that are too complex to verify.

As a PoC, this commit adds checks for too complex assignments, which are
assignments that are not directly inside of a compound statement (like
other assignment groups) and modify the pointer or count in some way.
Our model rejects those and requires the user to simplify their code.
For example:

```
  void foo(int *__counted_by(count) p, int count) {
    q = p = ...;
          ^ this is rejected
    n = count = ...;
              ^ this is rejected
    // the following is fine:
    p = ...;
    count = ...;
  }
```

rdar://161607826
@patrykstefanski patrykstefanski force-pushed the eng/pstefanski/PR-161607826 branch from e0ee8ec to ed31544 Compare October 8, 2025 23:45
@patrykstefanski
Copy link
Author

@swift-ci test llvm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ziqingluo-90 ziqingluo-90 ziqingluo-90 approved these changes

@delcypher delcypher Awaiting requested review from delcypher

@hnrklssn hnrklssn Awaiting requested review from hnrklssn

@rapidsna rapidsna Awaiting requested review from rapidsna

Assignees

@patrykstefanski patrykstefanski

Labels
clang:bounds-safety Issue relating to the experimental -fbounds-safety feature in Clang
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@patrykstefanski @ziqingluo-90