caml_copy_string() and caml_copy_int64() create OCaml blocks <https://ocaml.org/manual/intfc.html#sss:c-simple-allocation> that as such are subject to movement or release by the garbage collector <https://ocaml.org/manual/intfc.html#ss:c-blocks>.
In order for the GC to see that a block is in use (and should not be collected, plus its inward references should be updated when it is moved), the block needs to be reachable (directly or indirectly) from a "root" value. In each of the following patterns: Store_field (parent_block_v, field_n, caml_copy_string (str)) Store_field (parent_block_v, field_n, caml_copy_int64 (i64)) the block produced by caml_copy_*() hangs in the air temporarily, before a reference to it is added, originating from the parent block. If Store_field() triggered a garbage collection just before creating the reference, the retval block of caml_copy_*() could be moved (or even released), and then the new reference inside the parent block would point to garbage. Store_field() is apparently documented to never invalidate its last argument by calling the garbage collector, so the code we currently have in "libosinfo-c.c" is safe. However, the following warning <https://ocaml.org/manual/intfc.html#ss:c-simple-gc-harmony> does apply to Store_field(): > The first argument of Store_field and Store_double_field must be a > variable declared by CAMLparam* or a parameter declared by CAMLlocal* to > ensure that a garbage collection triggered by the evaluation of the > other arguments will not invalidate the first argument after it is > computed. meaning that the *first* argument of Store_field() *could* be released or moved by a garbage collection that is triggered by the evaluation of the last argument. And, in order to protect the first arg from such a release (or, to be notified if the first arg is moved), the first arg must be (temporarily) registered as a "root" value (with the "rootness" substituting for other inward edge(s) from other blocks). Keeping in mind the first and last (block) args' *different* exposures to garbage collection is difficult. Therefore, for every Store_field() call where the last arg is not an unboxed integer (in practice: Val_bool()), but a block created with caml_copy_*(), track that block (at least temporarily -- "long enough") as a root value, via a new local CAML value called "copyv". For consistency, update the one CAMLreturn (caml_copy_string (id)) call we have as well. Suggested-by: Richard W.M. Jones <[email protected]> Ref: https://listman.redhat.com/archives/libguestfs/2022-January/msg00037.html Signed-off-by: Laszlo Ersek <[email protected]> --- convert/libosinfo-c.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/convert/libosinfo-c.c b/convert/libosinfo-c.c index b8e78bec5c1c..ec7c06d379e3 100644 --- a/convert/libosinfo-c.c +++ b/convert/libosinfo-c.c @@ -183,23 +183,26 @@ value v2v_osinfo_os_get_id (value osv) { CAMLparam1 (osv); + CAMLlocal1 (copyv); const gchar *id; id = osinfo_entity_get_id (OSINFO_ENTITY(OsinfoOs_t_val (osv))); - CAMLreturn (caml_copy_string (id)); + copyv = caml_copy_string (id); + CAMLreturn (copyv); } static value glist_to_value_list (GList *list) { CAMLparam0 (); - CAMLlocal2 (rv, v); + CAMLlocal3 (rv, v, copyv); GList *l; rv = Val_emptylist; for (l = list; l != NULL; l = l->next) { v = caml_alloc (2, 0); - Store_field (v, 0, caml_copy_string (l->data)); + copyv = caml_copy_string (l->data); + Store_field (v, 0, copyv); Store_field (v, 1, rv); rv = v; } @@ -211,7 +214,7 @@ value v2v_osinfo_os_get_device_drivers (value osv) { CAMLparam1 (osv); - CAMLlocal3 (rv, v, vi); + CAMLlocal4 (rv, v, vi, copyv); OsinfoDeviceDriverList *list; gint i, len; @@ -230,9 +233,11 @@ v2v_osinfo_os_get_device_drivers (value osv) vi = caml_alloc (6, 0); str = osinfo_device_driver_get_architecture (driver); - Store_field (vi, 0, caml_copy_string (str)); + copyv = caml_copy_string (str); + Store_field (vi, 0, copyv); str = osinfo_device_driver_get_location (driver); - Store_field (vi, 1, caml_copy_string (str)); + copyv = caml_copy_string (str); + Store_field (vi, 1, copyv); b = osinfo_device_driver_get_pre_installable (driver); Store_field (vi, 2, Val_bool (b)); b = osinfo_device_driver_get_signed (driver); @@ -243,7 +248,8 @@ v2v_osinfo_os_get_device_drivers (value osv) /* Same as OSINFO_DEVICE_DRIVER_DEFAULT_PRIORITY in libosinfo 1.7.0+. */ i64 = 50; #endif - Store_field (vi, 4, caml_copy_int64 (i64)); + copyv = caml_copy_int64 (i64); + Store_field (vi, 4, copyv); l = osinfo_device_driver_get_files (driver); Store_field (vi, 5, glist_to_value_list (l)); g_list_free (l); @@ -286,7 +292,7 @@ value v2v_osinfo_os_get_all_devices (value osv) { CAMLparam1 (osv); - CAMLlocal3 (retvalv, linkv, propsv); + CAMLlocal4 (retvalv, linkv, propsv, copyv); g_autoptr (OsinfoDeviceList) dev_list = NULL; OsinfoList *ent_list; gint ent_nr; @@ -310,7 +316,8 @@ v2v_osinfo_os_get_all_devices (value osv) prop_val = osinfo_entity_get_param_value (ent, device_prop[prop_nr]); if (prop_val == NULL) prop_val = ""; - Store_field (propsv, prop_nr, caml_copy_string (prop_val)); + copyv = caml_copy_string (prop_val); + Store_field (propsv, prop_nr, copyv); } linkv = caml_alloc (2, 0); base-commit: f0cea012d0183edf6f7b769c28d5038593f3fe6a -- 2.19.1.3.g30247aa5d201 _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
