Skip to content

Fix caml_obj_truncate#5

Merged
damiendoligez merged 1 commit intodamiendoligez:4022-gc-patchesfrom
lpw25:4022-gc-patches-fix4
Nov 6, 2015
Merged

Fix caml_obj_truncate#5
damiendoligez merged 1 commit intodamiendoligez:4022-gc-patchesfrom
lpw25:4022-gc-patches-fix4

Conversation

@lpw25
Copy link
Copy Markdown

@lpw25 lpw25 commented Jul 29, 2015

Since sweeping can now occur before the ref table is emptied,
overflow objects created by Obj.truncate must be marked Caml_black
to avoid them being reallocated whilst still in the ref table.
Using Caml_black means that Abstract_tag should be used instead
of 1 in case the contents of the block are not safe to scan.

@lpw25
Fix caml_obj_truncate
070a74c 
Since sweeping can now occur before the ref table is emptied,
overflow objects created by Obj.truncate must be marked Caml_black
to avoid them being reallocated whilst still in the ref table.
Using Caml_black means that Abstract_tag should be used instead
of 1 in case the contents of the block are not safe to scan.
@lpw25
Copy link
Copy Markdown
Author

lpw25 commented Jul 30, 2015

This patch fixes a segfault which would occasionally happen during the tests for Core. The sequence of events that leads to a segfault is as follows:

  1. Modify a value in some object (an array in this case) to point to a value on the minor heap -- which adds the modified address to the ref table (i.e. the remembered set).
  2. Call Obj.truncate on the modified object. This creates a new "overflow" object containing the discarded part of the old object. This new object is marked white. Note that there is still a reference to this new object in the ref table.
  3. The GC decides to do some sweeping (without performing a minor collection). This adds the overflow object to the free list because it is marked white.
  4. The GC decides to do a minor collection. It starts by allocating objects on the major heap for each object being promoted from the minor heap, and adding a pointer from the minor heap object to the fresh major heap object. It also makes a linked list of these new values (called the todo list) by adding a pointer from each fresh major heap object to the previous minor heap object. Unfortunately, one of the fresh allocated major heap objects was made from part of the overflow object. This means there is a reference in the ref table to an item on the todo list.
  5. As part of the minor collection, the references in the ref table are forwarded from the old minor heap objects to the corresponding major heap objects. Since one entry in the ref table refers not to a real pointer but to one of the pointers used to implement the todo list, it incorrectly forwards this pointer breaking the structure of the todo list.
  6. Whilst attempting to finish the minor collection by traversing the todo list, the GC veers off into uninitialised bits of memory and segfaults.

This sequence of events was not possible in the old GC as the collector would never sweep until the ref table was empty. The solution is to mark the overflow object black. This forces it to stay around for a full cycle -- which ensures that we will have performed at least one minor collection.

@damiendoligez
Copy link
Copy Markdown
Owner

damiendoligez commented Nov 6, 2015

Note that if you make it black, the GC will not scan it, but yes Abstract_tag looks like a good idea anyway.

damiendoligez added a commit that referenced this pull request Nov 6, 2015
@damiendoligez
Merge pull request #5 from lpw25/4022-gc-patches-fix4
45537f9 
Fix caml_obj_truncate
@damiendoligez damiendoligez merged commit 45537f9 into damiendoligez:4022-gc-patches Nov 6, 2015
@lpw25
Copy link
Copy Markdown
Author

lpw25 commented Nov 6, 2015

Note that if you make it black, the GC will not scan it, but yes Abstract_tag looks like a good idea anyway.

Won't the sweep change it from black to white before it is finally collected?

@damiendoligez
Copy link
Copy Markdown
Owner

damiendoligez commented Nov 6, 2015

Yes but it's already unreachable at that point.

@lpw25
Copy link
Copy Markdown
Author

lpw25 commented Nov 6, 2015

I really thought there was a way for it to get scanned if the tag was 1. Perhaps it was compaction I was worried about. If the collection which turns it from black to white decides to do a compaction straight afterwards, would that cause the block to be scanned for pointers?

@damiendoligez
Copy link
Copy Markdown
Owner

damiendoligez commented Nov 10, 2015

No because compaction is always preceeded by a full major GC anyway.

@lpw25
Copy link
Copy Markdown
Author

lpw25 commented Nov 10, 2015

No because compaction is always preceeded by a full major GC anyway.

Ok, I see where I was confused now. caml_compact_heap_maybe only calls caml_finish_major_cycle which finishes the current cycle, but caml_compact_heap_maybe is only called by caml_major_collection_slice after a cycle has just completed so there has indeed been a full major GC.

damiendoligez pushed a commit that referenced this pull request Feb 23, 2016
@mshinwell
Merge pull request #5 from chambart/flambda_prereq-cmm_ifthenelse
b526d25 
Avoid pushing test deep inside conditions
damiendoligez pushed a commit that referenced this pull request Oct 11, 2024
@gasche @edwintorok
fix(toplevel): avoid hashing uninitialized values when printing (ocam…
d33d999 
…l#13294)

The toplevel printer detects cycles by keeping a hashtable of values
that it has already traversed.

However, some OCaml runtime types (at least bigarrays) may be
partially uninitialized, and hashing them at arbitrary program points
may read uninitialized memory. In particular, the OCaml testsuite
fails when running with a memory-sanitizer enabled, as bigarray
printing results in reads to uninitialized memory:

```
==133712==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x4e6d11 in caml_ba_hash /var/home/edwin/git/ocaml/runtime/bigarray.c:486:45
    #1 0x52474a in caml_hash /var/home/edwin/git/ocaml/runtime/hash.c:251:35
    #2 0x599ebf in caml_interprete /var/home/edwin/git/ocaml/runtime/interp.c:1065:14
    #3 0x5a909a in caml_main /var/home/edwin/git/ocaml/runtime/startup_byt.c:575:9
    #4 0x540ccb in main /var/home/edwin/git/ocaml/runtime/main.c:37:3
    #5 0x7f0910abb087 in __libc_start_call_main (/lib64/libc.so.6+0x2a087) (BuildId: 8f53abaad945a669f2bdcd25f471d80e077568ef)
    #6 0x7f0910abb14a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a14a) (BuildId: 8f53abaad945a669f2bdcd25f471d80e077568ef)
    #7 0x441804 in _start (/var/home/edwin/git/ocaml/runtime/ocamlrun+0x441804) (BuildId: 7a60eef57e1c2baf770bc38d10d6c227e60ead37)

  Uninitialized value was created by a heap allocation
    #0 0x47d306 in malloc (/var/home/edwin/git/ocaml/runtime/ocamlrun+0x47d306) (BuildId: 7a60eef57e1c2baf770bc38d10d6c227e60ead37)
    #1 0x4e7960 in caml_ba_alloc /var/home/edwin/git/ocaml/runtime/bigarray.c:246:12
    #2 0x4e801f in caml_ba_create /var/home/edwin/git/ocaml/runtime/bigarray.c:673:10
    #3 0x59b8fc in caml_interprete /var/home/edwin/git/ocaml/runtime/interp.c:1058:14
    #4 0x5a909a in caml_main /var/home/edwin/git/ocaml/runtime/startup_byt.c:575:9
    #5 0x540ccb in main /var/home/edwin/git/ocaml/runtime/main.c:37:3
    #6 0x7f0910abb087 in __libc_start_call_main (/lib64/libc.so.6+0x2a087) (BuildId: 8f53abaad945a669f2bdcd25f471d80e077568ef)
    #7 0x7f0910abb14a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a14a) (BuildId: 8f53abaad945a669f2bdcd25f471d80e077568ef)
    ocaml#8 0x441804 in _start (/var/home/edwin/git/ocaml/runtime/ocamlrun+0x441804) (BuildId: 7a60eef57e1c2baf770bc38d10d6c227e60ead37)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /var/home/edwin/git/ocaml/runtime/bigarray.c:486:45 in caml_ba_hash
```

The only use of hashing in genprintval is to avoid cycles, that is, it
is only useful for OCaml values that contain other OCaml values
(including possibly themselves). Bigarrays cannot introduce cycles,
and they are always printed as "<abstr>" anyway.

The present commit proposes to be more conservative in which values
are hashed by the cycle detector to avoid this issue: we skip hashing
any value with tag above No_scan_tag -- which may not contain any
OCaml values.

Suggested-by: Gabriel Scherer <gabriel.scherer@gmail.com>

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Co-authored-by: Edwin Török <edwin.torok@cloud.com>
damiendoligez pushed a commit that referenced this pull request Aug 22, 2025
@NickBarnes
Merge pull request #5 from kayceesrk/markdelay-assertion-fix
2bcb009 
Move the orphaned ephemerons GC colour check inside the barrier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lpw25 @damiendoligez