Skip to content
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.

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

Bounds checking for pointer dereferences and array subscripts #1176

Merged
merged 24 commits into from
Sep 7, 2021

Conversation

kkjeer
Copy link
Contributor

@kkjeer kkjeer commented Sep 1, 2021

This PR updates bounds validation to handle the following cases:

  1. Validate the bounds of a pointer dereference *p or array subscript p[i].
  2. Account for uses of pointer dereferences and array subscripts in observed bounds

The behavior for synthesizing member expressions whose bounds depend on a member expression that is being modified via an assignment has been extended: we now synthesize member expressions whose bounds depend on an lvalue expression that uses a member expression to update memory via an assignment. For example:

struct S {
  _Array_ptr<int> f : count(*ptr_to_len);
  _Array_ptr<int> ptr_to_len : count(10);
};

void f(struct S *s) {
  *s->ptr_to_len = 0;
}

The lvalue expression *s->ptr_to_len uses the member expression s->ptr_to_len to write to memory. We synthesize the member expression s->f whose declared bounds bounds(s->f, s->f + *s->ptr_to_len) depend on *s->ptr_to_len.

There is also a minor fix included in this PR that the pointer bounds checking depends on: in PreorderAST, the canonical form of e1[e2] is now *(e1 + e2 + 0) rather than *(e1 + e2). This enables bounds validation to treat expressions such as p[i] and *(p + i) as equivalent, since PreorderAST canonicalizes *(p + i) to *(p + i + 0).

kakje added 22 commits August 30, 2021 13:01
… subscript expressions in UpdateAfterAssignment
…nced member expressions to pointer-dereference-bounds.c
…dereferenced/subscripted member expressions to synthesized-members.c
// Observed bounds context after increment: { }
++*p;
// Observed bounds context after increment: { *p => bounds(*p - 1, (*p - 1) + 0) }
++*p; // expected-warning {{cannot prove declared bounds for '*p' are valid after increment}} \
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this behavior of the compiler is incorrect. Perhaps, at present, we should restrict the bounds validation for *p and p[i] when the type of *p or p[i] is a checked pointer type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this particular test case, *p has type _Nt_array_ptr<int>, since p has type _Nt_array_ptr<int> *.

In general, if e is a pointer dereference *e1 or an array subscript e1[e2], we will only check the bounds of e if e fulfills one of two conditions:

  1. e has type _Ptr<T>. In this case, the target bounds of e are bounds((_Array_ptr<T>)e, (_Array_ptr<T>)e + 1).
  2. e has type _Nt_array_ptr<T>. In this case, the target bounds of e are bounds(e, e + 0).

We do not explicitly check the type of e in UpdateAfterAssignment or ValidateBoundsContext. Instead, the AbstractSet containing e is added to ObservedBounds if and only if e has target bounds, which will only happen in the two cases described above.

The methods UnaryOperatorTargetBounds and ArraySubscriptExprTargetBounds in SemaBounds.cpp include comments that state that "Currently, we don't know the target bounds of a pointer stored in a pointer dereference (or returned by a subscripting operation), unless it is a _Ptr type or an _Nt_array_ptr."

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification (I missed noticing the *)!

Copy link
Contributor

@sulekhark sulekhark left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

if (Lex.CompareExprSemantically(LValue, E)) {
if (OriginalValue)
return OriginalValue;
else
Copy link

Choose a reason for hiding this comment

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

Nit: else is not needed after a return.

return OriginalValue;
else
return ExprError();
} else
Copy link

Choose a reason for hiding this comment

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

Same here.

if (Lex.CompareExprSemantically(LValue, E)) {
if (OriginalValue)
return OriginalValue;
else
Copy link

Choose a reason for hiding this comment

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

Same comments here.

@mgrang
Copy link

mgrang commented Sep 7, 2021

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants