Skip to content

Check that variables and member expressions used in return bounds are unmodified#1170

Merged
kkjeer merged 7 commits into
masterfrom
exprs-used-in-return-bounds
Aug 28, 2021
Merged

Check that variables and member expressions used in return bounds are unmodified#1170
kkjeer merged 7 commits into
masterfrom
exprs-used-in-return-bounds

Conversation

@kkjeer

@kkjeer kkjeer commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

After each assignment expression to a variable or member expression e, this PR checks that e is not used in the declared return bounds (if any) for the enclosing function.

In unchecked scopes, if the enclosing function has a bounds-safe interface, this checking is not performed (regardless of the type of e or the type of the RHS expression that was assigned to e).

Comment thread clang/lib/Sema/SemaBounds.cpp Outdated

// Account for uses of LValue in the declared return bounds (if any)
// for the enclosing function.
UpdateReturnBoundsAfterAssignment(LValue, E, Src, CSS);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The name UpdateRetunrBoundsAfterAssignment may be misleading. There is really no "update" happening - my understanding is that this function checks if LValue occurs in ReturnBounds.

Comment thread clang/lib/Sema/SemaBounds.cpp Outdated
// TODO: track an observed return bounds expression as a global property
// of the function body so that invertibility of lvalue expressions can
// be taken into account.
void UpdateReturnBoundsAfterAssignment(Expr *LValue, Expr *E, Expr *Src,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like the third parameter Src is not used anywhere in this function.


SourceLocation DeclaredLoc = FunctionDeclaration->getLocation();
S.Diag(DeclaredLoc, diag::note_declared_return_bounds)
<< ReturnBounds << ReturnBounds->getSourceRange();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suppose there were multiple program points in a function where a variable or a member expression used in ReturnBounds got modified, then would multiple error messages be emitted?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, there would be an error emitted at each assignment to a variable or member expression that is used in ReturnBounds.

@sulekhark

Copy link
Copy Markdown
Contributor

In unchecked scopes, if the enclosing function has a bounds-safe interface, this checking is not performed (regardless of the type of e or the type of the RHS expression that was assigned to e).

In unchecked scopes, it may probably be more consistent with our discussed policy to perform this check whenever the return bounds are checked (i.e. when the return expression has a checked pointer type, or the return expression has an unchecked pointer type because of implicit casting from checked to unchecked pointer types).

In other words, in unchecked scopes, either:

  1. The return bounds are checked and the return bounds expression is checked to ensure that the variables and member expressions occurring in it are unmodified, or
  2. Both of the above checks are not done.

@kkjeer

kkjeer commented Aug 26, 2021

Copy link
Copy Markdown
Contributor Author

In unchecked scopes, if the enclosing function has a bounds-safe interface, this checking is not performed (regardless of the type of e or the type of the RHS expression that was assigned to e).

In unchecked scopes, it may probably be more consistent with our discussed policy to perform this check whenever the return bounds are checked (i.e. when the return expression has a checked pointer type, or the return expression has an unchecked pointer type because of implicit casting from checked to unchecked pointer types).

In other words, in unchecked scopes, either:

  1. The return bounds are checked and the return bounds expression is checked to ensure that the variables and member expressions occurring in it are unmodified, or
  2. Both of the above checks are not done.

In the following example:

int *f(int *p : count(i), _Array_ptr<int> q : count(i), unsigned int i) : count(i) _Unchecked {
  i++;
  if (i > 0) {
    return q; // checked return value
  }
  return p; // unchecked return value
}

The assignment i++ modifies i which is used in the declared return bounds. At this point, the bounds checker has no knowledge of any return expressions and whether or not they are checked pointers.

In an analogous example for checking the bounds of a variable with a bounds-safe interface:

int *f(int *p : count(i), _Array_ptr<int> q : count(i), unsigned int i) : count(i) _Unchecked {
  i++;
  if (i > 0) {
    p = q; // a checked pointer is assigned to p
  }
  p++; // an unchecked pointer is assigned to p
}

The assignment i++ does not result in a bounds checking warning for p. At the assignment p = q, since q is a checked pointer, we check the bounds of p. However, we do not check at this point that the variable i which is used in the bounds of p was modified.

As I understand it, our current policy for checking the bounds of a variable p with a bounds-safe interface in an unchecked scope are: at the end of checking a statement S:

  1. If at any point during S, a checked pointer was assigned to p, we check the bounds of p.
  2. Otherwise, we do not check the bounds of p.

I would propose the following policy for checking the bounds of a function f with a bounds-safe interface in an unchecked scope: at the end of checking a statement S:

  1. If S returns a checked pointer, we check the bounds of f (including checking that S did not modify any variables or member expressions that are used in the bounds of f).
  2. Otherwise, we do not check that S did not modify any variables or member expressions that are used in the bounds of f.

I'd like to discuss these policies further.

@sulekhark

Copy link
Copy Markdown
Contributor

Yes, let's discuss it tomorrow.

@kkjeer

kkjeer commented Aug 26, 2021

Copy link
Copy Markdown
Contributor Author

Per offline discussion, we have the following policy for checking that variable or member expressions used in declared return bounds are unmodified:

  1. If an assignment e1 = e2 occurs in an unchecked scope, and the enclosing function f has a bounds-safe interface, then we do not check whether e1 occurs in the declared bounds of f.
  2. Otherwise, we do check whether e1 occurs in the declared bounds of f.

This is commented on line 4603 of SemaBounds.cpp in this PR.

@sulekhark sulekhark left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thank you!

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.

2 participants