ocaml / ocaml

The core OCaml system: compilers, runtime system, base libraries
https://ocaml.org
Other
5.19k stars 1.06k forks source link

Fix some memory bugs in `runtime_events_consumer.c` #13091

Open eutro opened 1 month ago

eutro commented 1 month ago

This PR fixes a few memory bugs surrounding runtime_events_loc in runtime_events_consumer.c, as mentioned in #13089, specifically in caml_runtime_events_create_cursor.

These bugs are:

https://github.com/ocaml/ocaml/blob/4c6a3849022ba19c23fb1860095f65eb09da157c/otherlibs/runtime_events/runtime_events_consumer.c#L108-L124

This PR:

https://github.com/ocaml/ocaml/blob/1c337f6d56604663f8b2eb839fe14abfb8230856/otherlibs/runtime_events/runtime_events_consumer.c#L237-L252

[^1]: This is the correct behaviour for cursor, since the caller takes ownership of the returned cursor iff the function returns successfully

eutro commented 1 month ago

It looks like, perhaps unsurprisingly, not all platforms work with the new test (though MSVC just had a quite disappointing network error), so I intend to remove it later, since I can't think of a better way to force failure.

eutro commented 4 weeks ago

I've spotted and fixed another bug, notably Runtime_events.create_cursor None never closing the file descriptor on Unix.

On trunk this crashes after opening too many file descriptors, causing create_cursor None to fail and trigger the existing double-free bug:

let () =
  Runtime_events.start ();
  try
    for _ = 1 to 1024 (* or whatever your [ulimit -n] is *) do
      Runtime_events.(create_cursor None |> free_cursor)
    done
  with _ ->
    Runtime_events.(create_cursor None |> free_cursor)

I also perform cleanup on the Windows handles if the mapping fails.

NickBarnes commented 4 weeks ago

I'll review this.

MisterDA commented 3 weeks ago

Should the ring file be also marked non-inheritable on Windows and close-on-exec on Unix?