Skip to content

Expanding __CPROVER_{r,w,rw}_ok must not introduce overflow [depends-on: #6616] #6656

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

tautschnig
Copy link
Collaborator

@tautschnig tautschnig commented Feb 10, 2022

Distributing signed-to-unsigned type casts over addition can result in
an unsigned overflow where previously there was no overflow. That's ok
at the logic level for any expressions yield the same models. We just
need to make sure that expressions generated are not subject to
goto_check_ct generating additional checks: goto_check_ct generates C
language checks, not ones for internally generated expressions.

  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • n/a My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@kroening
Copy link
Member

The simplifier is the wrong place to fix this. The simplifier (is meant to) works on fragments of logic, where there is no notion of overflow. All it is supposed to deliver is an expression that has the same models. This, of course, does not imply that programs using such expressions behave identically.

@tautschnig
Copy link
Collaborator Author

The simplifier is the wrong place to fix this. The simplifier (is meant to) works on fragments of logic, where there is no notion of overflow. All it is supposed to deliver is an expression that has the same models. This, of course, does not imply that programs using such expressions behave identically.

Ok, but then we'll need extra care in goto_check_ct (at least), which inserts simplified expressions into goto programs, where those goto programs may be subject to further transformation. (Imagine a workflow that runs goto-instrument --X-check followed by cbmc --Y-check.)

@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #6656 (29d63d3) into develop (e021eef) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #6656   +/-   ##
========================================
  Coverage    76.74%   76.74%           
========================================
  Files         1579     1579           
  Lines       182171   182189   +18     
========================================
+ Hits        139802   139823   +21     
+ Misses       42369    42366    -3     
Impacted Files Coverage Δ
src/analyses/goto_check_c.cpp 90.93% <100.00%> (+0.12%) ⬆️
src/ansi-c/ansi_c_parser.cpp 90.26% <100.00%> (+1.87%) ⬆️
src/goto-programs/goto_convert_side_effect.cpp 95.66% <100.00%> (+0.03%) ⬆️
src/pointer-analysis/value_set.cpp 83.29% <0.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e08c77d...29d63d3. Read the comment docs.

We do not necessarily have any pragmas set, and therefore must be able
to handle an empty stack.

Surprisingly, this only seg faulted when building in exactly the way
check-ubuntu-20_04-make-gcc is doing this in CI, and only on one test
added in the next commit.
This is required to make the conversion of __builtin_X_overflow not
generate spurious conversion checks.

Fixes: diffblue#6452
@tautschnig tautschnig force-pushed the bugfixes/simplifier-typecast-overflow branch from b89ed34 to 8c3f5da Compare February 10, 2022 17:41
@tautschnig tautschnig changed the title Rewriting of (T)(x OP y) must not introduce overflow Rewriting of (T)(x OP y) must not introduce overflow [depends-on: #6616] Feb 10, 2022
@tautschnig tautschnig self-assigned this Feb 10, 2022
@tautschnig
Copy link
Collaborator Author

The simplifier is the wrong place to fix this. The simplifier (is meant to) works on fragments of logic, where there is no notion of overflow. All it is supposed to deliver is an expression that has the same models. This, of course, does not imply that programs using such expressions behave identically.

Ok, but then we'll need extra care in goto_check_ct (at least), which inserts simplified expressions into goto programs, where those goto programs may be subject to further transformation. (Imagine a workflow that runs goto-instrument --X-check followed by cbmc --Y-check.)

I have now completely changed the implementation (and also the test to demonstrate where the original problem came from). This, however, requires #6616 to be merged first (which are the first two commits in here).

@tautschnig tautschnig changed the title Rewriting of (T)(x OP y) must not introduce overflow [depends-on: #6616] Expanding __CPROVER_{r,w,rw}_ok must not introduce overflow [depends-on: #6616] Feb 10, 2022
Distributing signed-to-unsigned type casts over addition can result in
an unsigned overflow where previously there was no overflow. That's ok
at the logic level for any expressions yield the same models. We just
need to make sure that expressions generated are not subject to
goto_check_ct generating additional checks: goto_check_ct generates C
language checks, not ones for internally generated expressions.
@tautschnig tautschnig force-pushed the bugfixes/simplifier-typecast-overflow branch from 8c3f5da to 29d63d3 Compare February 10, 2022 21:32
Copy link

@chris-ryder chris-ryder left a comment

Choose a reason for hiding this comment

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

Seems like a sensible and simple approach.

@@ -2020,6 +2047,10 @@ optionalt<exprt> goto_check_ct::rw_ok_check(exprt expr)
exprt c = conjunction(conjuncts);
if(enable_simplify)
simplify(c, ns);

// this is a logic expression, C language checks no longer apply

Choose a reason for hiding this comment

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

Possible worth rewording the comment here to be super explicit?

Suggested change
// this is a logic expression, C language checks no longer apply
// this is an internal logic expression to support contracts, not a C language expression.
// C language checks should not be applied to it

@remi-delmas-3000
Copy link
Collaborator

Hello,

additionally it would be nice to have a theorem that says that if rw_ok(p, s) holds, then the absence of pointer overflow is guaranteed when computing char * q = ((char *) p + s;, meaning offset(p) + s does not overflow when computed as a signed integer using offset_bits = pointer_width - object_bits.

@feliperodri feliperodri added the aws Bugs or features of importance to AWS CBMC users label Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws Bugs or features of importance to AWS CBMC users bugfix dependent - do not merge Property Instrumentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants