Skip to content

generate assert(false) when calling undefined function #8292

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

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

kroening
Copy link
Member

@kroening kroening commented May 13, 2024

This changes the symbolic execution engine to emit an assert(false) when processing a call to a function without body, instead of merely emitting a warning.

The key benefit is that undefined function bodies are a threat to soundness, especially when CBMC is run without a human operator (say in CI) who might spot the warning. A common scenario is a call to a function that was renamed, or whose signature has changed. This scenario now triggers a verification failure.

Users who prefer the previous, or other alternative behaviors, can achieve this via program instrumentation, say using goto-instrument.

  • 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.
  • 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 kroening added Symbolic Execution soundness Soundness bug? Review and add "aws" if it is, or remove "soundness" if it isn't. labels May 13, 2024
@kroening kroening force-pushed the no-body-assertion branch 7 times, most recently from ef8bc33 to 020649b Compare May 13, 2024 20:43
Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.38%. Comparing base (c3e5ba8) to head (6c99724).

Files Patch % Lines
src/goto-symex/symex_function_call.cpp 81.81% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8292      +/-   ##
===========================================
- Coverage    78.40%   78.38%   -0.02%     
===========================================
  Files         1722     1722              
  Lines       188252   188267      +15     
  Branches     18497    18487      -10     
===========================================
- Hits        147593   147572      -21     
- Misses       40659    40695      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kroening kroening force-pushed the no-body-assertion branch from 020649b to 038854d Compare May 14, 2024 05:49
@kroening kroening marked this pull request as ready for review May 14, 2024 05:49
@kroening kroening force-pushed the no-body-assertion branch 3 times, most recently from 8846400 to 64074af Compare May 14, 2024 07:07
@martin-cs
Copy link
Collaborator

The idea of getting rid of quiet-ish potential unsoundness is good but for a long time we have been telling people that type nondet_type(); is a good way to model non-determinism. What is the new recommended way?

Also should this change other tools? Could we make the semantics consistent across tools and analyses by simply adding a pass that calls generate_function_bodies with assert-false and nondet-return (or whatever the appropriate level of over-approximation we decide on).

@kroening
Copy link
Member Author

The idea of getting rid of quiet-ish potential unsoundness is good but for a long time we have been telling people that type nondet_type(); is a good way to model non-determinism. What is the new recommended way?

That keeps working (and hasn't emitted a warning before, the prefix nondet_ makes the difference).

@kroening
Copy link
Member Author

Also should this change other tools? Could we make the semantics consistent across tools and analyses by simply adding a pass that calls generate_function_bodies with assert-false and nondet-return (or whatever the appropriate level of over-approximation we decide on).

Worth considering! This may be somewhat specific to the tool, depending on how function pointers are handled.

@kroening kroening force-pushed the no-body-assertion branch 3 times, most recently from 1d8d9fd to c2c896f Compare May 14, 2024 12:20
@kroening kroening added the Version 6 Pull requests and issues requiring a major version bump label May 14, 2024
@kroening kroening force-pushed the no-body-assertion branch 3 times, most recently from bb69a46 to 2e71950 Compare May 15, 2024 07:40
@karkhaz
Copy link
Collaborator

karkhaz commented May 16, 2024

Users who prefer the previous, or other alternative behaviors, can achieve this via program instrumentation, say using goto-instrument.

Can I suggest documenting how to do this, say in cprover-manual/modeling-nondeterminism.md? Something like:

 Note that the body of the function is not defined. The name of the function itself is irrelevant (save for the prefix), but must be unique. Also note that a nondeterministic choice is not to be confused with a probabilistic (or random) choice.

+ You can also cause existing function calls in your program to return a nondeterministic value by using `goto-instrument --generate-havocing-body`.
+ This is particularly useful for functions that do not have an implementation available.
+ By default, such functions are modelled as having a body of `assert(false)`, and CBMC prints a "`no body for callee`" warning.
+ This means that verification will fail on code that calls these functions.
+ Generating a havocing body for functions whose body has not been linked in ensures that you can verify code that calls these functions.
 

@karkhaz karkhaz self-requested a review May 16, 2024 17:17
@kroening
Copy link
Member Author

Can I suggest documenting how to do this, say in cprover-manual/modeling-nondeterminism.md? Something like:

I think it belongs into a section about modular reasoning.

Comment on lines -288 to -290
if(cmdline.isset("havoc-undefined-functions"))
options.set_option("havoc-undefined-functions", true);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also needs to be removed from the man page.

This changes the symbolic execution engine to emit an assert(false) when
processing a call to a function without body, instead of merely emitting a
warning.

The key benefit is that undefined function bodies are a threat to soundness,
especially when CBMC is run without a human operator (say in CI) who might
spot the warning.  A common scenario is a call to a function that was
renamed, or whose signature has changed.  This scenario now triggers a
verification failure.

Users who prefer the previous, or other alternative behaviors, can achieve
this via program instrumentation, say using goto-instrument.
@kroening kroening force-pushed the no-body-assertion branch from 2e71950 to 6c99724 Compare June 12, 2024 18:21
@kroening kroening requested review from jimgrundy and TGWDB as code owners June 12, 2024 18:21
@kroening kroening enabled auto-merge June 12, 2024 18:22
@kroening kroening merged commit 8919fdc into develop Jun 12, 2024
39 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
soundness Soundness bug? Review and add "aws" if it is, or remove "soundness" if it isn't. Symbolic Execution Version 6 Pull requests and issues requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants