On Mon, Jan 10, 2022 at 02:26:23PM +0100, Laszlo Ersek wrote: > 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.
Or was! Store_field is a trivial macro around caml_modify, and caml_modify _was_ documented as not calling the GC here: https://github.com/ocaml/ocaml/blob/b6163a49ee67acf17235b49c177a5f85b8c944db/runtime/memory.c#L609 But that comment is not present on the trunk. (I'm unclear when it was removed - the git history is complicated to follow.) > 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 ACK Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
