Skip to content

AddressSet.meet: Convert offsets to intdomain values to check overlap#967

Merged
jerhard merged 45 commits intomasterfrom
issue_564
Mar 31, 2023
Merged

AddressSet.meet: Convert offsets to intdomain values to check overlap#967
jerhard merged 45 commits intomasterfrom
issue_564

Conversation

@jerhard
Copy link
Copy Markdown
Member

@jerhard jerhard commented Jan 17, 2023

This PR changes the implementation of the AddressSet

Fixes #822.

@jerhard jerhard changed the title AddressSet`.meet: Convert offsets to intdomain values to check overlap AddressSet.meet: Convert offsets to intdomain values to check overlap Jan 17, 2023
@michael-schwarz
Copy link
Copy Markdown
Member

It might be interesting to benchmark this to see how expensive computing the dual representation on demand every time is and whether it might be worth saving the offset together with the offset representation so it can be a simple lookup.

michael-schwarz and others added 4 commits January 22, 2023 13:42
This is for consistency with Normal (which implements printable) and NormalLat (which implements lattice operations).
jerhard and others added 2 commits February 27, 2023 16:04
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Not leaking this abstraction requires changing the signature of may_be_equal to elt -> elt -> bool (from t -> t -> bool).
This commit changes the implementation of the ProjectiveSetPairwiseMeet.meet to iterate also over the elements described by B: SetDomain that is passed.
@jerhard jerhard marked this pull request as ready for review March 20, 2023 09:33
@sim642 sim642 self-requested a review March 21, 2023 15:49
@sim642 sim642 added this to the v2.2.0 milestone Mar 27, 2023
@jerhard
Copy link
Copy Markdown
Member Author

jerhard commented Mar 28, 2023

There is still one issue remaining with addresses of arrays that were casted to a different type, and where then an offset is taken:

char* a_intoffset =(char*) (a + 1);
char* b_intoffset = b_char + sizeof(int);
__goblint_check(a_intoffset == b_intoffset);
__goblint_check((char*) (a + 1) == b_char + sizeof(int));
char* a_4intoffset = (char*) (a + 4);
__goblint_check(a_4intoffset == b_intoffset); // FAIL
__goblint_check((char*) (a + 4) == b_char + sizeof(int)); // FAIL

These four __goblint_checks are unsound on this branch: for the first two it claims it will fail, while for the last two it claims that they will succeed. On master the first two are unknown (instead of succeeding) and the last two are unsoundly claimed to succeed.

To handle this correctly, one would have to track the type of the pointer to the abstract address operations, so that one can compute the correct offset in bits from it.

@jerhard
Copy link
Copy Markdown
Member Author

jerhard commented Mar 28, 2023

As discussed with @michael-schwarz, one approach to handle the issue described in the previous comment, would be to introduce a separate constructor for broken / corrupted abstract offsets.

The idea is to set the offset to CorruptedOffset in Base, when an offset is added on a pointer, where the static pointer-target type is different from the type of the actual abstract target.
Then, abstract equality checks of two address that have the same base-address, and at least one of the addresses has a CorruptedOffset will result in Unknown. When reading from an address with corrupted offset, the top_value of the type would be read, while writing would set the whole base-value to the top_value of the type.

I started some work on this in a separate branch: array_cast_broken_offsets. There are some failing test cases with the changes from that branch, mostly caused by reduced precision.

@sim642
Copy link
Copy Markdown
Member

sim642 commented Mar 29, 2023

To handle this correctly, one would have to track the type of the pointer to the abstract address operations, so that one can compute the correct offset in bits from it.

I would've expected the abstract bitsOffset calculation to handle that already, does it not? I think Cil.bitsOffset correctly accounts for array element type sizes on index offsets.

@jerhard jerhard merged commit ddfdd2d into master Mar 31, 2023
@jerhard jerhard deleted the issue_564 branch March 31, 2023 11:27
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.

Unsound address set must-not-equality check in base

3 participants