uname: register roots for intermediate strings to avoid GC corruption#6880
Conversation
kit-ty-kate
left a comment
There was a problem hiding this comment.
LGTM, thanks! I have a couple of very minor comments but the core is of course good
| #include <sys/utsname.h> | ||
|
|
||
| CAMLprim value opam_uname(value _unit) { | ||
| CAMLparam1(_unit); |
There was a problem hiding this comment.
wouldn't CAMLparam0(); be sufficient here since it's unused?
|
|
||
| CAMLprim value opam_uname(value _unit) { | ||
| CAMLparam1(_unit); | ||
| CAMLlocal4(v_ret, v_sysname, v_release, v_machine); |
There was a problem hiding this comment.
very minor, but i feel like the v_ prefix change here isn't necessary
|
actually i'm curious, why add new intermediate cc @gasche |
|
I've chosen to simplify the change and only register the actually required GC root ( Unless someone raises a veto in the meantime, i'll merge this in a couple hours. |
dra27
left a comment
There was a problem hiding this comment.
Both versions absolutely fine - indeed, the problem with the original code was just that ret could move if any of the caml_copy_string calls resulted in a GC, the actual copied strings went straight into a block, so no issue.
…corruption Reported-by: Andrew Nesbitt Co-authored-by: Kate <kit-ty-kate@exn.st>
|
Rebased above master. Merging once CI is done. |
|
Thanks a lot! (ignoring the ocaml-benchmarks failure, opam update failed with error code 40 seems unrelated) |
|
Just catching up after some travels; mine was just conservative as each caml_copy_string could go into the GC, but only if someone else modified the code later. Thanks for the simpler revision. |
uname: register roots for intermediate strings to avoid GC corruption
Reported by @andrew
Backported to 2.5 in #6893