Libguestfs - Jan 2022 - [v2v PATCH] convert/libosinfo-c.c: turn caml_copy_*() return blocks into root values

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 <rjones at redhat.com>
Ref: https://listman.redhat.com/archives/libguestfs/2022-January/msg00037.html
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 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

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 <rjones at redhat.com>
> Ref:
https://listman.redhat.com/archives/libguestfs/2022-January/msg00037.html
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>  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