Fix snprintf_os use in runtime_events_consumer.c#13089
Merged
gasche merged 2 commits intoocaml:trunkfrom Apr 10, 2024
Merged
Conversation
nojb
approved these changes
Apr 10, 2024
Contributor
nojb
left a comment
There was a problem hiding this comment.
Good catch! LGTM
I don't think a change entry is needed for this fix specifically
This is a bug fix, so I do think it deserves a Changes entry.
By the way, how did the bug manifest for you?
ghost
approved these changes
Apr 10, 2024
1455244 to
3f25ba5
Compare
Specifically, use the OS string for the `%s` parameter, as it's supposed to be used
3f25ba5 to
0d9d194
Compare
Contributor
Author
It's turning out to be harder than the bug fix itself...
I was trying to get https://github.com/tarides/runtime_events_tools/ working on Windows and when I fixed the PID issue I ran into it formatting the completely wrong path anyway. |
Contributor
|
Thanks for adding the Changes entry. I tweaked it to mention the terms that may be relevant for the ocassional |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes an incorrect use of
snprintf_osfor formatting the runtime events ring file path in the implementation ofotherlibs/runtime_events. Specifically, this changes the call to use the OS string rather than achar *copy, which both matches how it is constructed inruntime_events.c:ocaml/runtime/runtime_events.c
Line 96 in a7afade
ocaml/runtime/runtime_events.c
Lines 264 to 265 in a7afade
and is actually correct. It also has the benefit of removing a single alloc/free pair on all OSes.
This bug only manifests on Windows, where
char_osis notchar, and thus was unlikely to have been tested by anyone (before me, I suppose1). Windows has further issues with this API, in how theUnixlibrary handles PIDs on Windows (asHANDLEs casted toint, rather than real PIDs, see a similar issue in #11021), which makes some existing uses ofRuntime_events.create_cursorstill broken on Windows, with no unambiguous fix.2At a glance, there seem to be further bugs in this function too. For instance,
runtime_events_locis unconditionally initialised tocaml_stat_alloc_noexc(RING_FILE_NAME_MAX_LEN), but isn't freed on the code path withpid < 0, nor does it seem to be freed at all if the function completes unexceptionally. I'm also slightly weary of the use ofintfor the PID3.I don't think a change entry is needed for this fix specifically, but it could be linked to the change entry with a bug fix for the above.
Footnotes
I've successfully attached to another process using runtime events on a build of OCaml that includes this fix as well as another to fix the PID issue, but I could produce a program that attaches with just this fix, if desired. ↩
There are many different options: This function could perform the
HANDLEto PID conversion, however that would fail if the user does provide an actual PID. TheUnixlibrary could also be changed: apidfdtype; providing a way to get the real PID; returning actual PIDs even on Windows; etc. Another option would be to sidestep the use of PIDs altogether, and add something likeOCAML_RUNTIME_EVENTS_FILEto specify an exact file. ↩It should work fine on Linux (
PID_MAX_LIMITis 2^22 on 64-bit systems, according to proc(5)) and macOS (according to this SO post, PIDs don't go higher than 99998). However it could be an issue on Windows, either if we interpret it as aHANDLE(which is a pointer), or as an actual PID (Raymond Chen claims to have seen PIDs in the 4 bn. range in this SO comment). ↩