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

Havoc non det functions #1585

Closed
wants to merge 1 commit into from

Conversation

polgreen
Copy link
Contributor

@polgreen polgreen commented Nov 13, 2017

CBMC currently assumes that functions with no body can return a non-deterministic value. This pull request extends that, to assume that functions with no body can also write a non-deterministic value to any non-const function arguments.

@polgreen
Copy link
Contributor Author

Could this be reviewed please, again preferably by @kroening or @peterschrammel, because Michael has already reviewed this internally. Thanks

If function body does not exist, we assume that any pointers passed to function may be written to
with a non-deterministic value, but only with something the size of the pointer type.

regression test for havoc_undefined_functions
@polgreen polgreen force-pushed the havoc_non_det_functions branch from e1734af to b9a5096 Compare November 24, 2017 12:19
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Should this also mess with reachable pointers? For example, if I passed a struct A { B* b }; could it mess with param->b->x, or just param->b? In the latter case this needs extra tests. Suggest also adding tests that exercise the things it should not do -- string constants, for example, I imagine should not be changed even if cast to (char*)?

@polgreen
Copy link
Contributor Author

havoc-undefined-functions is really just a command line argument that applies an existing CBMC feature during analysis instead of it needing to be written into the source code of the program we are analysing before hand. The feature is: __CPROVER_havoc_object, which was introduced here, with regression testing:
beac327

I need to add another test to --havoc-undefined-functions that tests that we don't call havoc_rec() if the subtype is const.

The other questions are really questions about the behaviour of __CPROVER_havoc_object, so I'll try to speak to @kroening and when i'm sure what the semantics should be, extend the regression test in cbmc/ havoc_object1.

(although the answers from my quick testing are that (char*) string constant is not havoc'd and reachable pointers are not havoc'd, which matches what I think would be expected)

@smowton
Copy link
Contributor

smowton commented Nov 24, 2017

That sounds expected. I'd extend the test a little to check that behaviour -- the test will document what happens, if not necessarily what the original author intended :)

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Looks very useful - I do wonder if there is some way of combining the code for havoc_rec (which I've not seen) and the code for generating nondet pointers for input, but almost certainly out of scope for this PR.

It would be good to add some tests for:

  • pointers to structs
  • pointers to structs that contain pointers
  • pointers to const values (to ensure they are not havoc'd)
  • const pointers to non-const values (to ensure they are)

--havoc-undefined-functions
^EXIT=10$
^SIGNAL=0$
^VERIFICATION FAILED$
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: missing empty line

function(&a);
__CPROVER_assert(a==0,"");
return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: missing empty line

@@ -52,6 +52,7 @@ class optionst;
"(show-symbol-table)(show-parse-tree)(show-vcc)" \
"(show-claims)(claim):(show-properties)" \
"(drop-unused-functions)" \
"(havoc-undefined-functions)" \
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a super useful option in other binaries (such as goto-cc/goto-analzyer/goto-instrument), if you have time it'd be great to add it to them as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done during symexing rather than by changing the goto-functions, so I can't very easily move this into goto-instrument, but it can be used for any binary that cbmc can analyse.

I don't really know what you would expect to be inserted into binaries, if it was to be part of goto-instrument, goto-analyze or goto-cc? It could replace the function body with __CPROVER_havoc(whatever the return values and pointer arguments are), but that is only meaningful if you are going to analyze the binary with cbmc afterwards.

@smowton
Copy link
Contributor

smowton commented Dec 3, 2017

One thing we should get an opinion on, probably from @kroening or @peterschrammel: CBMC currently produces interesting stub side effects by giving the GOTO functions bodies (for example, for Java they allocate a fresh object tree and optionally mess with parameters), and test gen does the same, while this uses an intrinsic that symex then interprets. It seems to me we should settle on one method or the other.

@polgreen
Copy link
Contributor Author

polgreen commented Dec 3, 2017

Yes, although CBMC does already assign, during symexing, non-deterministic values to returns from undefined functions, so we already have two different methods without this PR. This is an extension of the existing behaviour during symexing, which is why I implemented it where it is

@peterschrammel
Copy link
Member

peterschrammel commented Dec 3, 2017

There are at least four different things what to do with functions without bodies:

  1. assume(false)
  2. nondeterministic return value only, no side-effects (current default behaviour)
  3. nondeterministic return value and side-effects
  4. use summary

There should be a unified mechanism for choosing between these. Doing it through a goto-program transformation is the obvious candidate.

I suggest creating stub bodies for functions without bodies and filling the bodies with the required goto instructions dependent on the chosen function-without-body-replacement-action 1-4.
This would also seamlessly integrate with lazy loading, I think (replacement-action 0 "load/use actual body if available").

@martin-cs
Copy link
Collaborator

Although it is potentially quite a bit more work ... I agree with @peterschrammel on this one. There are similar issues with goto-analyze and I've been thinking that we need to tidy up the various different options and patches being used there. A properly unified solution would be even better. However I think there are more options than he outlined:

A. Effects on globals (ignore / non-det).
B. assert(false) (finding when the missing function actually matters).
C. Creating new objects (if returning (or modifying) pointers).
D. Ignore completely (assume(true))

@polgreen
Copy link
Contributor Author

polgreen commented Aug 21, 2018

Can we reopen the discussion about the best way of dealing with functions without bodies? @peterschrammel @martin-cs

I appreciate the neatness of having a single solution that uses the goto program transformations, but it's just not quick enough for what I want to do, which is to use the aggressive slicer on Xen.

I have tried running the generate-function-bodies solution on the code that I am analysing, and the workflow is an order of magnitude slower (see issue #2663)

@chrisr-diffblue
Copy link
Contributor

@hannes-steffenhagen-diffblue might want to be involved in the discussion too

@polgreen
Copy link
Contributor Author

Good point. Although I assume he's being spammed by the issue thread anyway..!

@TGWDB
Copy link
Contributor

TGWDB commented Aug 5, 2021

Closing this PR on the basis that although there may have been ideas/discussion three years ago, there has been no action/progress since. Please reopen if you believe this is erroneous of will update this PR with new development.

@TGWDB TGWDB closed this Aug 5, 2021
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.

8 participants