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

Escaped variables are not communicated between threads #1074

Closed
jerhard opened this issue Jun 5, 2023 · 3 comments · Fixed by #1078
Closed

Escaped variables are not communicated between threads #1074

jerhard opened this issue Jun 5, 2023 · 3 comments · Fixed by #1078
Assignees
Milestone

Comments

@jerhard
Copy link
Member

jerhard commented Jun 5, 2023

In this test case, Goblint, with the default configuration, does not relax information about the escaped local variables x, even though another thread may be writing into it. In particular, Goblint unsoundly claims that the second __goblint_check will succeed:

int let_escape(){
int x = 23;
g = 23;
__goblint_check(x == 23);
p = &x;
sleep(5);
__goblint_check(x == 23); //UNKNOWN!
}

@jerhard jerhard added the unsound label Jun 5, 2023
@sim642 sim642 added the bug label Jun 5, 2023
@jerhard
Copy link
Member Author

jerhard commented Jun 6, 2023

When *p is written in thread2, it the Base analysis knows that p may point to x, however is_global ask x returns false. This is due to the ThreadEscape analysis claiming that x has not escaped.

For the MayEscape x query, the ThreadEscape analysis looks up whether x is in its local state. But x escaped in thread1, and there seems to be no synchronization of this information to the analysis of thread2. So one somehow needs to answer the MayEscape queries based on some flow-insensitive-invariant.
There may be some ways to avoid being completely flow-insensitive by excluding some escapes that definitely have not happened yet: e.g. when there is a unique thread, a local variable of it must have escaped in its own past, to be considered esacped.
@michael-schwarz and I discussed some of this shortly yesterday.

@jerhard jerhard changed the title Goblint computes unsound value for escaped local variable Escaped variables are not communicated between threads Jun 6, 2023
@sim642
Copy link
Member

sim642 commented Jun 6, 2023

In #686 we already added a global invariant to the threadescape analysis. Do you know why that doesn't already account for this?

@jerhard
Copy link
Member Author

jerhard commented Jun 6, 2023

In the current state of the analysis, there is no ctx.getg, i.e. things that are side-effected are never considered, also not when answering queries.

Actually, in special and threadenter, the ctx.global is called, but this is not sufficient.

@jerhard jerhard self-assigned this Jun 6, 2023
@sim642 sim642 added this to the v2.2.0 milestone Sep 11, 2023
sim642 added a commit to sim642/opam-repository that referenced this issue 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