Fixed resource leaks in runtime events system by NickBarnes · Pull Request #13419 · ocaml/ocaml

NickBarnes · 2024-09-03T11:30:24Z

As @eutro is currently unavailable to shepherd #13091 to merge, here I have rebased it on to trunk, with some additional changes to reflect my review comments. The discussion on #13091 mostly applies here equally.

In email @eutro wrote: "In the interest of getting it merged in a timely manner, it's probably best if you do open a new PR, since I'm not sure when I would have gotten around to addressing the review myself. The code looks good to me."

gasche

Approved. Could you maybe move the Changes entry from the "Working version" section to the "5.3.0" section, so that I don't need to tweak it when cherry-picking?

There is a failure of tests/lib-runtime-events/test_create_cursor_failure in the i386 CI, which needs to be investigated before this can be merged.

 ... testing 'test_create_cursor_failures.ml' with line 6 (bytecode) => failed

@@ -1,6 +1,6 @@
 Runtime_events: no ring for current process.          Was runtime_events started?
 OK
 OK
-Runtime_events: could not create cursor for specified path.
-Runtime_events: could not create cursor for specified path.
-Runtime_events: could not create cursor for specified path.
+OK
+OK
+OK

ghost

Looking good.

NickBarnes · 2024-09-04T09:18:22Z

Approved. Could you maybe move the Changes entry from the "Working version" section to the "5.3.0" section, so that I don't need to tweak it when cherry-picking?

Done.

There is a failure of tests/lib-runtime-events/test_create_cursor_failure in the i386 CI, which needs to be investigated before this can be merged.

Curious - cursor creation isn't failing as it should. I'll look into it.

NickBarnes · 2024-09-04T16:17:15Z

There is a failure of tests/lib-runtime-events/test_create_cursor_failure in the i386 CI, which needs to be investigated before this can be merged.

Curious - cursor creation isn't failing as it should. I'll look into it.

I set up an i386 environment but couldn't reproduce this.

gasche · 2024-09-04T19:40:38Z

testsuite/tests/lib-runtime-events/test_create_cursor_failures.ml

+  Runtime_events.pause ()
+
+(* workaround for finding the events file even on Windows, where
+   [Unix.getpid] doesn't match the one used to open the file *)


This is, ahem, horrible. It sounds like something that could be an issue for our users as well. Could we fix this by ensuring that there is a programmatic way, from OCaml, to guess the correct event file for the current program? We could change the runtime-events code to use the same pid as Unix.getpid, or expose a way (maybe a new function in the runtime_events module?) to get whatever other notion of pid was used.

Note: another option would be to not work on this problem at all on this PR, and restrict the test to not-windows and write the simple solution to get the pid file. (I'm curious to know whether the testsuite failure would go away in this case.)

gasche · 2024-09-04T19:44:28Z

testsuite/tests/lib-runtime-events/test_create_cursor_failures.ml

+  let startup_events_files = list_events_files () in
+  fun () ->
+  let is_new_event_file f = not (List.mem f startup_events_files) in
+  List.find is_new_event_file (list_events_files ())


The code here looks fairly optimistic in terms of the expected semantics of the filesystem. It assumes that all .events file from past test runs are visible in the filesystem when this program starts (but, as far as I know, there is no fsync call anywhere in the runtime that would give this guarantee), and then at call-time it assumes that the .events file created by the current process at runtime-events startup time is already visible in the filesystem. (The latter is not the cause of the weird test failure in this PR I guess, otherwise we would see a Not_found exception.)

Isn't there a guarantee that files created by a process are subsequently visible to that process? Huh.

Hm, I must misremember those things and your interpretation is plausible. But that would only cover the second issue I mentioned, not the accuracy of the first call to list_event_files that happens at program startup and lists "startup files" from other processes, right?

I quite agree: Runtime_events should have a function to return the path of the current process's events file (possibly a function on a cursor, returning the file name, as one can readily make a cursor for the current process).

On reflection, there's no reason for it to be a function on a cursor.

Incidentally, while I'm looking at resource safety in the runtime events system, I think there is a lack of data hygiene in caml_runtime_events_current_location, which hands out the value of the module-local string pointer current_ring_loc - so one domain might use it after another domain frees it. I'm changing it to always hand out a copy.

OK, I've tidied that up and added Runtime_events.path to address your points about finding the events file.

gasche · 2024-09-05T17:05:55Z

otherlibs/runtime_events/runtime_events.mli


+val path : unit -> string
+(** [path ()] returns the path to the runtime events file, if runtime
+  events are being collected.  *)


And otherwise, what? (Answer: it fails with a Failure _ exception, which could be documented.) I would prefer to have the function return a string option.

I considered that, but thought this was "good enough" for an interface in which other exceptional cases are not documented (e.g. Runtime_events.create_cursor) and other functions (e.g. pause) silently do nothing if circumstances aren't right (e.g. events aren't enabled). The whole API is perhaps a bit shonky. But that's no excuse to add more shonkiness; I'll tidy this up.

I see; with github reviews we don't really see the diff in context, so I tend to be maximally nitpicky by default, apologies.

gasche · 2024-09-05T17:07:06Z

Thanks for the simplification to the failing test. Unfortunately it looks like this still fails for i386, so we probably need to think about this further.

NickBarnes · 2024-09-06T13:50:44Z

Thanks for the simplification to the failing test. Unfortunately it looks like this still fails for i386, so we probably need to think about this further.

Can't reproduce.

$ docker run -it --platform linux/i386 ocaml-i386
root@b29ea0b89219:/# su - dixon
dixon@b29ea0b89219:~$ cd ocaml
dixon@b29ea0b89219:~/ocaml$ git checkout pr-13091
[...]
dixon@b29ea0b89219:~/ocaml$ make distclean
[...]
dixon@b29ea0b89219:~/ocaml$ ./configure --prefix=`opam var prefix`
[...]
dixon@b29ea0b89219:~/ocaml$ make -j 4
[...]
dixon@b29ea0b89219:~/ocaml$ make install
[...]
dixon@b29ea0b89219:~/ocaml$ make -C testsuite one TEST=tests/lib-runtime-events/test_create_cursor_failures.ml
make: Entering directory '/home/dixon/ocaml/testsuite'
 ... testing 'test_create_cursor_failures.ml' => passed
make[1]: Entering directory '/home/dixon/ocaml/testsuite'
make[1]: Leaving directory '/home/dixon/ocaml/testsuite'
make[1]: Entering directory '/home/dixon/ocaml/testsuite'


Summary:
     1 tests passed
     0 tests skipped
     0 tests failed
     0 tests not started (parent test skipped or failed)
     0 unexpected errors
     1 tests considered
make[1]: Leaving directory '/home/dixon/ocaml/testsuite'
make: Leaving directory '/home/dixon/ocaml/testsuite'
dixon@b29ea0b89219:~/ocaml$ lscpu
Architecture:        i686
CPU op-mode(s):      32-bit
Byte Order:          Little Endian
CPU(s):              4
On-line CPU(s) list: 0-3
Thread(s) per core:  1
Core(s) per socket:  4
Socket(s):           1
Vendor ID:           0x00
Model:               0
Stepping:            0x0
BogoMIPS:            48.00
Flags:               fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 asimddp sha512 asimdfhm dit uscat ilrcpc flagm ssbs sb paca pacg dcpodp flagm2 frint
dixon@b29ea0b89219:~/ocaml$

gasche · 2024-09-06T13:52:59Z

Ah, there is also a MSVC 64bits build failure in the testsuite:

runtime/runtime_events.c(289): error C2143: syntax error: missing ';' before '}'
runtime/runtime_events.c(282): warning C4133: 'initializing': incompatible types - from 'wchar_t *' to 'char *'

(The wchar_t thing might suggest that char_os should be used somewhere instead of char, I don't know.)

gasche · 2024-09-06T13:55:44Z

The fact that you can't reproduce does not immediately tell us how to deal with the issue, unfortunately. We cannot just merge the PR as-is and break the i386 CI, so we have to decide between:

heroic attempts to fix the issue without being able to reproduce it easily, or to do CI-golfing to figure out what is going on
disabling the test on i386 to make the issue go away, with a comment explaining why we did that

Which route is best depends on whether it is a real issue that does not occur under your particular setup (but does on the CI), or whether this is not a real issue and just an artifact of some weird setup on the CI machine. It's tempting to bet on the second option, but I really have no idea myself. This is also not the focus of this PR, so I'm happy to cut our losses and silence this one CI configuration for that one test.

nojb · 2024-09-06T14:02:29Z

runtime/runtime_events.c

-      caml_fatal_error("Couldn't open ring buffer loc: %s",
-                        ring_loc_u8);
-      caml_stat_free(ring_loc_u8);
+      char* ring_loc_u8 = caml_stat_strdup_noexc_os(current_ring_loc);


The function caml_stat_strdup_noexc_os does a "os" -> "os" copy. Here you would need a "os" -> "utf-8" copy instead (like the original caml_stat_strdup_of_os).

My mistake. I was tripped up by the similarity between the function names caml_stat_strdup_of_os (which returns a char * and can't raise an exception) and caml_stat_strdup_os (which returns a char_os * and can). I guess from the previous state of the code that I want MBCS for the caml_fatal_error, so I should use caml_stat_strdup_of_os. But my change was intended to avoid the (in fact impossible) exception, surviving a failure of the strdup. So looking at the implementation of caml_stat_strdup_of_os, it does a straight malloc under the hood, then calls WideCharToMultiByte with the un-validated return value. So that still seems unsafe: are we going to get NULL, or a SEGV?
This is getting quite bikesheddy: producing a useful caml_fatal_error message despite the failure of this particular malloc might not be a very high priority. But I can tweak caml_stat_strdup_of_os.

So looking at the implementation of caml_stat_strdup_of_os, it does a straight malloc under the hood, then calls WideCharToMultiByte with the un-validated return value.

Sorry, but caml_stat_strdup_of_utf16 uses caml_stat_alloc, which (if memory serves) can raise "out of memory" exception; am I missing something?

ocaml/runtime/win32.c

Lines 967 to 977 in fc98dd1

CAMLexport caml_stat_string caml_stat_strdup_of_utf16(const wchar_t *s)

{

caml_stat_string out;

int retcode;

retcode = caml_win32_wide_char_to_multi_byte(s, -1, NULL, 0);

out = caml_stat_alloc(retcode);

caml_win32_wide_char_to_multi_byte(s, -1, out, retcode);

return out;

}

Argh, I'm touching the implementation in yacc/wstr.c, rather than the one in runtime/win32.c. PEBKAC. I'm going home.

OK, I've cleaned this up with a new caml_stat_strdup_noexc_of_os.

gasche · 2024-09-06T14:20:41Z

Regarding disabling the test on i386, I have bad news and good news:

ocamltest has a arch_i386 option, but no arch_not_i386 option, so right now you cannot do this
someone implemented negation in ocamltest, and it's juuust waiting for a review from @shindere and @OlivierNicole

NickBarnes · 2024-09-06T14:48:29Z

The fact that you can't reproduce does not immediately tell us how to deal with the issue, unfortunately. We cannot just merge the PR as-is and break the i386 CI, so we have to decide between:
* heroic attempts to fix the issue without being able to reproduce it easily, or to do CI-golfing to figure out what is going on

* disabling the test on i386 to make the issue go away, with a comment explaining why we did that

I am suspicious of the i386 CI setup, but have wildly exceeded my effort budget for this PR. I'm going to ask @dra27 to poke at it with me when he's back from ICFP.

…umer.c` Co-authored-by: Nick Barnes <nick@tarides.com>

Test cases for some recent bugs in lib-runtime-events Co-authored-by: Nick Barnes <nick@tarides.com>

NickBarnes · 2024-09-09T14:08:25Z

@dra27 spots that the i386 CI at present runs everything as root, so the events file is readable to the test despite the permission bits all being cleared by chmod. So this test, as it stands, will fail when running as root.

dra27 · 2024-09-09T15:25:30Z

CI fixed in dra27@1434c45 - I don't seem to be able to push to the PR directly.

gasche · 2024-09-10T04:33:08Z

Oof! Congratulations on the CI fixes. Merging.

Octachron · 2024-10-31T09:53:31Z

@gasche, it looks like you never cherry-picked this PR to 5.3 . Any specific reasons? If there are no specific reasons, I am inclined to do so to give more freeway to the runtime events library.

Fixed resource leaks in runtime events system (cherry picked from commit 05f7e7a)

Octachron · 2024-10-31T12:20:04Z

Cherry-picked to 5.3 as planned initially in 64f6c2e .

NickBarnes force-pushed the pr-13091 branch from 8f2fdba to bf60b45 Compare September 3, 2024 11:31

gasche approved these changes Sep 3, 2024

View reviewed changes

ghost approved these changes Sep 4, 2024

View reviewed changes

NickBarnes force-pushed the pr-13091 branch from bf60b45 to e85972b Compare September 4, 2024 09:17

NickBarnes force-pushed the pr-13091 branch from e85972b to 5cd2a1f Compare September 4, 2024 16:31

gasche reviewed Sep 4, 2024

View reviewed changes

NickBarnes force-pushed the pr-13091 branch from 5cd2a1f to 49462e1 Compare September 5, 2024 16:47

gasche reviewed Sep 5, 2024

View reviewed changes

NickBarnes force-pushed the pr-13091 branch 2 times, most recently from 8f3e69d to c6f83d9 Compare September 6, 2024 13:43

nojb reviewed Sep 6, 2024

View reviewed changes

NickBarnes force-pushed the pr-13091 branch from c6f83d9 to b4a175d Compare September 6, 2024 14:18

NickBarnes force-pushed the pr-13091 branch from b4a175d to 71f17d6 Compare September 6, 2024 14:42

eutro and others added 2 commits September 9, 2024 14:32

Clarify and fix caml_stat memory management in `runtime_events_cons…

3ca771e

…umer.c` Co-authored-by: Nick Barnes <nick@tarides.com>

Add tests/lib-runtime-events/test_create_cursor_failures.ml

8d7995f

Test cases for some recent bugs in lib-runtime-events Co-authored-by: Nick Barnes <nick@tarides.com>

NickBarnes force-pushed the pr-13091 branch from 71f17d6 to 0d35743 Compare September 9, 2024 13:34

GHA i386: run as a normal user rather than root

1434c45

NickBarnes force-pushed the pr-13091 branch from 0d35743 to 1434c45 Compare September 9, 2024 20:51

gasche merged commit 05f7e7a into ocaml:trunk Sep 10, 2024

gasche mentioned this pull request Sep 10, 2024

Fix some memory bugs in runtime_events_consumer.c #13091

Closed

talex5 mentioned this pull request Sep 16, 2024

Add Runtime_events.EV_EMPTY_MINOR #13407

Merged

dra27 mentioned this pull request Sep 29, 2024

Add caml_stat_char_array_{to,of}_os #13497

Merged

MisterDA mentioned this pull request Oct 22, 2024

Fix caml_stat_wcsdup_noexc raising in out-of-memory #13567

Merged

Octachron pushed a commit that referenced this pull request Oct 31, 2024

Merge pull request #13419 from NickBarnes/pr-13091

64f6c2e

Fixed resource leaks in runtime events system (cherry picked from commit 05f7e7a)

This was referenced Feb 19, 2025

A series of runtime-events fixes ported from upstream oxcaml/oxcaml#3596

Merged

Fix field initialisation bug in runtime_events.c. #13819

Merged

dra27 mentioned this pull request May 4, 2025

GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) #14013

Merged

	CAMLexport caml_stat_string caml_stat_strdup_of_utf16(const wchar_t *s)
	{
	caml_stat_string out;
	int retcode;

	retcode = caml_win32_wide_char_to_multi_byte(s, -1, NULL, 0);
	out = caml_stat_alloc(retcode);
	caml_win32_wide_char_to_multi_byte(s, -1, out, retcode);

	return out;
	}

Conversation

NickBarnes commented Sep 3, 2024

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

NickBarnes commented Sep 4, 2024

Uh oh!

NickBarnes commented Sep 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasche Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasche commented Sep 5, 2024

Uh oh!

NickBarnes commented Sep 6, 2024

Uh oh!

gasche commented Sep 6, 2024

Uh oh!

gasche commented Sep 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasche commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickBarnes commented Sep 6, 2024

Uh oh!

NickBarnes commented Sep 9, 2024

Uh oh!

dra27 commented Sep 9, 2024

Uh oh!

gasche commented Sep 10, 2024

Uh oh!

Octachron commented Oct 31, 2024

Uh oh!

Octachron commented Oct 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gasche Sep 4, 2024 •

edited

Loading

gasche commented Sep 6, 2024 •

edited

Loading