Skip to content

Audit copy_nonoverlapping intrinsic#1201

Merged
adpaco-aws merged 7 commits intomodel-checking:mainfrom
adpaco-aws:audit-copy-nonoverlap
May 20, 2022
Merged

Audit copy_nonoverlapping intrinsic#1201
adpaco-aws merged 7 commits intomodel-checking:mainfrom
adpaco-aws:audit-copy-nonoverlap

Conversation

@adpaco-aws
Copy link
Contributor

Description of changes:

Completes the audit for the copy_nonoverlapping intrinsic by reusing the work done on the copy intrinsic (see #1199):

  • Adds alignment checks for both src and dst.
  • Adds overflow check for the count in bytes (this is not mentioned in the documentation).
  • Adds tests for all possible cases.

Resolved issues:

Resolves #1198
Part of #1163

Testing:

  • How is this change tested? Existing regression and new tests

  • Is this a refactor change? No.

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@adpaco-aws adpaco-aws requested a review from a team as a code owner May 16, 2022 20:59
@adpaco-aws adpaco-aws force-pushed the audit-copy-nonoverlap branch from 1c61e9d to 03d7f54 Compare May 16, 2022 21:00
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Thanks for reducing the duplicated code. I just have a few comments besides my previous comment that I published too soon. ☺️

@@ -0,0 +1 @@
memcpy src/dst overlap No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a CBMC message? I was wondering if we change it to say copy_nonoverlap here. Do we use memcpy anywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a CBMC message so there's not much we can do. We use memcpy in our __rust_realloc C implementation but we don't generate calls to memcpy anywhere else.

Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks

@adpaco-aws adpaco-aws merged commit c941888 into model-checking:main May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

copy_nonoverlapping is handled with StatementKind, not an intrinsic

3 participants