irmin-pack 3.6: fix mmap keeps a file descriptor

[art-w]

I'm not too happy about this quickfix. The mapping file uses mmap to lazy load its content, but this keeps the underlying file descriptor alive (until the OCaml GC calls the finalizer and releases it, but later than Irmin close)

I've included two commits:

• The first one shows that calling Stdlib.Gc.full_major () after close is good enough to release the file descriptor
• The second steals C stubs from https://github.com/ocaml/ocaml/pull/389 and munmap specific update from https://github.com/janestreet/core/blob/4b6635d206f7adcfac8324820d246299d6f572fe/core/src/bigstring_stubs.c#L114 to release the file descriptor early, without full_major. The linked PR explains why this is considered unsafe, but it's very easy to confirm that we are in the "no use after free".

While the updated C stub seems to work, it would be nice if someone knowledgeable could take a look at the changes (cc @talex5 ? for some reason caml_ba_finalize isn't calling munmap anymore https://github.com/ocaml/ocaml/blob/0e09dc477c131a404f28be373effc3508c99ff6f/runtime/bigarray.c#L164)

With the sparse file refactoring, our use of mmap has significantly dropped to the point that we could implement mmap in userland (since the file is never updated, we only need lazy reading.) This would allow removing the C stubs and I would feel a lot better.

[talex5]

I haven't kept up with what happened with this code. I would guess that the Jane Street version is well tested, though. Multicore probably introduces some more ways for use-after-free to be unsafe (not just compiler optimisations). I made yet another version of this at https://github.com/talex5/wayland-proxy-virtwl/blob/a99bba0a802940ce637b23e072e3d7555c5830ad/virtio_gpu/utils.mli#L15, though I don't remember what was wrong with the original.

[codecov-commenter]

Codecov Report

:exclamation: No coverage uploaded for pull request base (3.6@6c708ee). Click here to learn what that means. The diff coverage is n/a.

:exclamation: Current head bd7e130 differs from pull request most recent head 4f924ab. Consider uploading reports for the commit 4f924ab to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@          Coverage Diff           @@
##             3.6    #2196   +/-   ##
======================================
  Coverage       ?   68.08%           
======================================
  Files          ?      134           
  Lines          ?    16166           
  Branches       ?        0           
======================================
  Hits           ?    11007           
  Misses         ?     5159           
  Partials       ?        0           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[vect0r-vicall]

I think next steps would be asking Victor to test your branch before we do a proper release.

@metanivek I confirm that is solves the issue. However, no need to do a release for that, it is really a minor leak that only impacts the very long tests.

Thanks for the fix!

[art-w]

Thank you all for the pointers and the feedback! It's better if we can avoid depending on the bigarray internals, so we'll just remove mmap in the next release after checking for performance regression :)

(But this PR is ready if it turns out that the issue is more urgent than we thought)