Dune corrupts ~10400 bytes of binary during artifact version substitution causing segfault

[edwintorok]

Expected Behavior

No segfault

Actual Behavior

Segfault, and a hex comparison of before/after artifact substitution shows it has changed ~10400 bytes after the end of the placeholder:

./repro.sh
0.0.0
0.0.0
Fatal error: exception Failure("Couldn't determine xapi version - got unexpected version from dune: 'fbe3031'")
Found placeholder in _build/default/gen_lifecycle.bc:
- placeholder: Vcs_describe "."
- evaluates to: "fbe3031"
Found placeholder in _build/default/gen_lifecycle.bc:
- placeholder: Vcs_describe "."
- evaluates to: "fbe3031"
Found placeholder in _build/default/gen_lifecycle.exe:
- placeholder: Vcs_describe "."
- evaluates to: "fbe3031"
Segmentation fault (core dumped)

$ xxd _build/default/gen_lifecycle.exe >good
$ xxd gen_lifecycle.exe >bad
$ diff -wu good bad
--- good        2022-12-09 17:23:07.590138110 +0000
+++ bad 2022-12-09 17:23:01.857971513 +0000
@@ -48493,663 +48493,663 @@
 000bd6c0: fc07 0000 0000 0000 332e 322e 3000 0002  ........3.2.0...
 000bd6d0: fc0b 0000 0000 0000 6475 6e65 2d62 7569  ........dune-bui
 000bd6e0: 6c64 2d69 6e66 6f00 fc27 0000 0000 0000  ld-info..'......
-000bd6f0: 2525 4455 4e45 5f50 4c41 4345 484f 4c44  %%DUNE_PLACEHOLD
-000bd700: 4552 3a36 343a 7663 732d 6465 7363 7269  ER:64:vcs-descri
-000bd710: 6265 3a31 3a2e 2525 2525 2525 2525 2525  be:1:.%%%%%%%%%%
-000bd720: 2525 2525 2525 2525 2525 2525 2525 2525  %%%%%%%%%%%%%%%%
-000bd730: 0000 0000 0000 0007 0000 0000 0000 0000  ................
-000bd740: 0000 0000 0000 0000 0700 0000 0000 0000  ................
-000bd750: f284 0400 0000 0000 1100 0000 8c00 0000  ................
-000bd760: bf84 0400 0000 0000 2200 0100 0100 0100  ........".......
-000bd770: 8e84 0400 0000 0000 2100 0000 7c00 0000  ........!...|...
-000bd780: 7884 0400 0000 0000 2100 0200 0800 1000  x.......!.......
-000bd790: 7000 0000 0000 0000 6084 0400 0000 0000  p.......`.......
....
+000bffd0: 0133 b801 33ef 4040 a105 17cb 0056 a0e0  .3..3.@@.....V..
+000bffe0: a027 426f 6f6c 5f74 7900 5b90 a0c0 b390  .'Bool_ty.[.....
+000bfff0: 0509 05a0 c096 c090 9022 6131 0205 f5e1  ........."a1....
 000c0000: a3a2 0400 0000 0000 1300 0100 0100 0104  ................
 000c0010: 8006 0000 0000 0000 2ba2 0400 0000 0000  ........+.......
 000c0020: 2100 0100 1000 0000 d004 0000 0000 0000  !...............

Full output is here:
good.txt
bad.txt

Note that this isn't always a segfault, sometimes you get a fatal out of memory error, or it just hangs forever
(depending how exactly the binary is corrupted), but on a given dune+compiler version it always seems to corrupt the binary the same way.

This is pure OCaml code, no C stubs or Obj.magic and if you look at where the crash actually it, it is a NULL pointer deref in the C runtime startup code, which obviously became corrupted:

==179488== Invalid read of size 8
==179488==    at 0x176B74: fill_hashtable (roots_nat.c:115)
==179488==    by 0x176CEE: init_frame_descriptors (roots_nat.c:158)
==179488==    by 0x1930DC: caml_startup_common (startup_nat.c:127)
==179488==    by 0x1932CE: caml_startup_exn (startup_nat.c:163)
==179488==    by 0x1932CE: caml_startup (startup_nat.c:168)
==179488==    by 0x1466E1: main (main.c:41)
==179488==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

Reproduction

1. git clone https://github.com/edwintorok/segfault
2. cd segfault
3. ./repro.sh

Specifications

• Version of dune (output of dune --version):
  multiple
  3.2.0
  3.6.1

• Version of ocaml (output of ocamlc --version)
  4.13.1
  4.14.0

• Operating system (distribution and version):
  Ubuntu 22.04.1 LTS
  Centos Stream release 8

Additional information

I have reproduced this on 2 different machines running different Linux distributions, and also tried various dune and OCaml versions on the same machine too (see above for versions), they all fail:
Intel(R) Xeon(R) Silver 4108 CPU @ 1.80GHz
Intel(R) Xeon(R) CPU E5-2650 v3 @ 2.30GHz

• Link to gist with verbose output (run dune with the --verbose flag):
  output.txt

[edwintorok]

(note that the binary installed by 'dune install' is not corrupted, just the executable in the build directory, but if there are any bugs with artifact substitution it'd be good to find out why the one in the build dir is getting corrupted)

[bobot]

Normally the binaries in the _build directory are not substituted. The promoted one in the source directory are substituted. Thank you for the testcase, still can you precise if the bug occurs without promoted binary?

[edwintorok]

We've been using dune-build-info since ~Oct 4 and looking through our internal testsuite it hasn't found any crashes, but it might be that it is just extremely rare to hit the codepath that happened to be corrupted during normal usage (all the unit tests run prior to artifact substitution usually, and our system-level tests which use installed binaries don't have that much coverage).

I'll try to see whether I can modify the testcase to detect corruption without promotion, e.g. diff (the hex transformed) binary pre/post promotion and assert that the only change is really just around the version number placeholders
(I see there are unit tests for this too, but there is obviously some corner case they don't cover).

[bobot]

Perhaps no need to test it, just does "just the executable in the build directory" was talking about binaries in _build or binaries promoted in the source directory?

[edwintorok]

The corruption is for the binary in the source directory (the promoted one).

I have a suspicion this is due to the use of a single global 'buf', and multiple fibers corrupting/overwriting each-other's data, which is why with a single binary's 'dune install' it doesn't reproduce, but it does with promote until clean which promotes both the .bc and .exe in quick succession, quick enough that they're probably both active at the same time.

[edwintorok]

Yes, moving the place that 'buf' is allocated seems to fix the testcase (lets see what the CI says about the rest of the tests): 48cc447

In general it is probably a bad idea to use global state if you can run multiple builds in parallel (I haven't looked in detail how fibers work in dune or when they're allowed to switch from one fiber to another, but it is probbaly safe to assume that it can switch to another fiber and global mutable state should be avoided).

[edwintorok]

However if I'm looking at this correctly this means that Dune can corrupt any binaries that are being promoted/installed in parallel, not just those that use artifact substitution (since there is no way to turn subsitution off, even if unused it'll still copy everything through that shared global 'buf', which may or may not corrupt other binaries too?).
I'll see whether I can come up with a 2nd testcase that shows corruption, but it'd probably be good to backport this fix to older versions of Dune too.

[edwintorok]

The only 'scheduling point' I see is let* s = eval t ~conf in artifact_subsitution.ml, which happens only when decoding placeholders, so binaries that have no placeholders are probably safe against this corruption.

[edwintorok]

I've added a few more testcases, I can't make it corrupt the binaries on 'dune install -j16', but it reliably corrupts the binaries with 'promote until-clean'. I've also added a hex diff of the binaries to check that the diff is constrained to the expected small area, and in the buggy case it can be seen that the diff is much bigger (this should make the test more robust against future changes to the compiler/runtime that might cause the testcase to not segfault anymore)

[rgrinberg]

@edwintorok would you like to submit a PR to unshare the buffer?

[edwintorok]

Yes, see the testcase PR , I've updated it to also contain the fix, not just the testcase.