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

Analysis of longjmp/setjmp #970

Merged
merged 186 commits into from
Mar 24, 2023
Merged

Analysis of longjmp/setjmp #970

merged 186 commits into from
Mar 24, 2023

Conversation

michael-schwarz
Copy link
Member

@michael-schwarz michael-schwarz commented Jan 22, 2023

PR to document the ongoing progress towards sound handling of longjmp/setjmp.

Corresponding draft for those with access to versioncontrolseidl: https://versioncontrolseidl.in.tum.de/schwarz/longjmp-and-setjmp

Issues that need to be handled:

  • Non-unique jump buffers
  • Jumping from several functions deep
  • Jumping in recursive calls
  • Returning a value that is potentially zero in longjmp call
  • Warn for longjmps that propagate out of the current thread
  • ...?
  • Detecting misuse
    • Jumping from different thread
    • Accessing non-volatile local variable that has been potentially modified since call
    • setjmp in invalid place (not directly used for branching) (deemed out of scope)
    • sigsetjmp

Closes #887

src/analyses/base.ml Fixed Show fixed Hide fixed
src/framework/constraints.ml Fixed Show fixed Hide fixed
@sim642 sim642 self-requested a review March 22, 2023 09:33
@michael-schwarz
Copy link
Member Author

I'll also start a testrun of this with the benchmarks we used for SOAP.

@michael-schwarz
Copy link
Member Author

The changes increase the runtime for libpng by ~ 10%. I could not inspect warnings, as they now are in the behavior category, so I restarted a run where I increased the level of output again.

@sim642
Copy link
Member

sim642 commented Mar 23, 2023

Interesting, I'm not sure what might've caused it but I have a few ideas from my changes in #1015:

  1. Replace context hashes with ControlSpecC (1f7424f). This makes things more reliable but such elements in sets are also more expensive to compare, etc than just ints.
  2. I also changed the normal combine (7e70553). Previously it changed local without adapting ask.
  3. Using Access events (ba3437a, d7708d3, f2eb6e0) might be making things more expensive while in single-threaded mode. Normally those events are not emitted, but with these new analyses they are.

@michael-schwarz
Copy link
Member Author

I was able to verify that the output is still as expected. W.r.t. the slowdown, I don't think it is critical for this PR, I think it can be addressed in a separate PR.

@michael-schwarz michael-schwarz merged commit c3639a8 into master Mar 24, 2023
@michael-schwarz michael-schwarz deleted the longjmp branch March 24, 2023 15:26
@sim642
Copy link
Member

sim642 commented Mar 25, 2023

For some reason marshaling tests on the new test group fail across the board: https://github.com/goblint/analyzer/actions/runs/4516943406.

@michael-schwarz
Copy link
Member Author

Maybe it's because contexts are not relifted inside domain values?

@michael-schwarz
Copy link
Member Author

After relifting the contents of jumpbuffers, it works if done manually:

./regtest.sh 68 02 --set save_run save
./regtest.sh 68 02 --set load_run save

For some reason, it still fails when invoked via the script.

@sim642
Copy link
Member

sim642 commented Mar 28, 2023

Interesting, I'm not sure what might've caused it but I have a few ideas from my changes in #1015:

  1. I also changed the normal combine 7e70553. Previously it changed local without adapting ask.

I benchmarked this just this change on sv-benchmarks with master and there wasn't a performance difference.
So at least this change, which affects Goblint even without longjmp analyses, doesn't hurt us in general.

@sim642
Copy link
Member

sim642 commented Mar 28, 2023

After relifting the contents of jumpbuffers, it works if done manually:

./regtest.sh 68 02 --set save_run save
./regtest.sh 68 02 --set load_run save

For some reason, it still fails when invoked via the script.

That's not how marshaling tests do it though. The second run uses --conf save/config.json --set save_run '' --set load_run save.
Exactly using the full commands used by update_suite.rb, I can reproduce the fixpoint error still.

@michael-schwarz
Copy link
Member Author

For some reason, there are two paths when the incremental analysis checks if the fixpoint is reached, but only one in the marshalled result.

@sim642
Copy link
Member

sim642 commented Mar 28, 2023

I've started looking into this myself already. We have quite many relifts missing, but that's hidden by the default one. Making the default one in Printable.Std fail allows tracking down any missing ones that we've been too lazy to add. I've done that for everything in this particular example, but there's still an issue, but it's now inside base's jmpbuf domain.

@michael-schwarz
Copy link
Member Author

I didn't know you already started looking into it. I added some of these relifts directly on master (fae675b), these are probably superseded by your changes.

@sim642 sim642 added this to the v2.2.0 milestone Apr 5, 2023
sim642 added a commit to sim642/opam-repository that referenced this pull request Sep 13, 2023
CHANGES:

* Add `setjmp`/`longjmp` analysis (goblint/analyzer#887, goblint/analyzer#970, goblint/analyzer#1015, goblint/analyzer#1019).
* Refactor race analysis to lazy distribution (goblint/analyzer#1084, goblint/analyzer#1089, goblint/analyzer#1136, goblint/analyzer#1016).
* Add thread-unsafe library function call analysis (goblint/analyzer#723, goblint/analyzer#1082).
* Add mutex type analysis and mutex API analysis (goblint/analyzer#800, goblint/analyzer#839, goblint/analyzer#1073).
* Add interval set domain and string literals domain (goblint/analyzer#901, goblint/analyzer#966, goblint/analyzer#994, goblint/analyzer#1048).
* Add affine equalities analysis (goblint/analyzer#592).
* Add use-after-free analysis (goblint/analyzer#1050, goblint/analyzer#1114).
* Add dead code elimination transformation (goblint/analyzer#850, goblint/analyzer#979).
* Add taint analysis for partial contexts (goblint/analyzer#553, goblint/analyzer#952).
* Add YAML witness validation via unassume (goblint/analyzer#796, goblint/analyzer#977, goblint/analyzer#1044, goblint/analyzer#1045, goblint/analyzer#1124).
* Add incremental analysis rename detection (goblint/analyzer#774, goblint/analyzer#777).
* Fix address sets unsoundness (goblint/analyzer#822, goblint/analyzer#967, goblint/analyzer#564, goblint/analyzer#1032, goblint/analyzer#998, goblint/analyzer#1031).
* Fix thread escape analysis unsoundness (goblint/analyzer#939, goblint/analyzer#984, goblint/analyzer#1074, goblint/analyzer#1078).
* Fix many incremental analysis issues (goblint/analyzer#627, goblint/analyzer#836, goblint/analyzer#835, goblint/analyzer#841, goblint/analyzer#932, goblint/analyzer#678, goblint/analyzer#942, goblint/analyzer#949, goblint/analyzer#950, goblint/analyzer#957, goblint/analyzer#955, goblint/analyzer#954, goblint/analyzer#960, goblint/analyzer#959, goblint/analyzer#1004, goblint/analyzer#558, goblint/analyzer#1010, goblint/analyzer#1091).
* Fix server mode for abstract debugging (goblint/analyzer#983, goblint/analyzer#990, goblint/analyzer#997, goblint/analyzer#1000, goblint/analyzer#1001, goblint/analyzer#1013, goblint/analyzer#1018, goblint/analyzer#1017, goblint/analyzer#1026, goblint/analyzer#1027).
* Add documentation for configuration JSON schema and OCaml API (goblint/analyzer#999, goblint/analyzer#1054, goblint/analyzer#1055, goblint/analyzer#1053).
* Add many library function specifications (goblint/analyzer#962, goblint/analyzer#996, goblint/analyzer#1028, goblint/analyzer#1079, goblint/analyzer#1121, goblint/analyzer#1135, goblint/analyzer#1138).
* Add OCaml 5.0 support (goblint/analyzer#1003, goblint/analyzer#945, goblint/analyzer#1162).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Soundly handle setjmp and longjmp
3 participants