Re: [PATCH v2 1/2] migration/vmstate: Add VMState support for GByteArray

[Peter Xu <peterx@redhat.com>]

On Thu, Apr 09, 2026 at 11:21:24PM +0530, Arun Menon wrote:
> From: Arun Menon <armenon@redhat.com>
> 
> In GLib, GByteArray is an object managed by the library. Currently,
> migrating a GByteArray requires treating it as a raw C struct and using
> VMSTATE_VBUFFER_ALLOC_UINT32. For example, see vmstate_vdba in
> ui/vdagent.c
> 
> QEMU cannot pretend that GByteArray is a C struct and simply use
> VMS_ALLOC to g_malloc() the buffer. This is because, VMS_ALLOC blindly
> overwrites the data pointer with a newly allocated buffer, thereby
> leaking the previous memory. Besides, GLib tracks the array's capacity
> in a hidden alloc field. Bypassing GLib APIs leave this capacity out of
> sync with the newly allocated buffer, potentially leading to heap buffer
> overflows during subsequent g_byte_array_append() calls.
> 
> This commit introduces VMSTATE_GBYTEARRAY which uses specific library
> API calls (g_byte_array_set_size()) to safely resize and populate the
> buffer.
> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> Fix-Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  include/migration/vmstate.h | 10 ++++++++++
>  migration/vmstate-types.c   | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 62c2abd0c4..f503a40ec0 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -265,6 +265,7 @@ extern const VMStateInfo vmstate_info_bitmap;
>  extern const VMStateInfo vmstate_info_qtailq;
>  extern const VMStateInfo vmstate_info_gtree;
>  extern const VMStateInfo vmstate_info_qlist;
> +extern const VMStateInfo vmstate_info_g_byte_array;
>  
>  #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
>  /*
> @@ -892,6 +893,15 @@ extern const VMStateInfo vmstate_info_qlist;
>      .start        = offsetof(_type, _next),                              \
>  }
>  
> +#define VMSTATE_GBYTEARRAY(_field, _state, _version) {                   \
> +    .name         = (stringify(_field)),                                 \
> +    .version_id   = (_version),                                          \
> +    .size         = sizeof(GByteArray),                                  \
> +    .info         = &vmstate_info_g_byte_array,                          \
> +    .flags        = VMS_SINGLE,                                          \
> +    .offset       = vmstate_offset_pointer(_state, _field, GByteArray),  \
> +}
> +
>  /* _f : field name
>     _f_n : num of elements field_name
>     _n : num of elements
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 89cb211472..743c2092e9 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -942,3 +942,40 @@ const VMStateInfo vmstate_info_qlist = {
>      .get  = get_qlist,
>      .put  = put_qlist,
>  };
> +
> +static int get_g_byte_array(QEMUFile *f, void *pv, size_t size,
> +                            const VMStateField *field)
> +{
> +    GByteArray **byte_array = (GByteArray **)pv;
> +    uint32_t len = qemu_get_be32(f);
> +
> +    if (*byte_array == NULL) {
> +        *byte_array = g_byte_array_new();
> +    }

Not an immediate problem when GByteArray* will always be pre-allocated
(even if with len=0) in the vdagent use case, but just to mention..

This is kind of over-taking the VMS_ALLOC semantics, and it will be error
prone too as vmstate core has special handling of NULL pointers, you can
see e.g. vmstate_save_state_v() has this piece of code:

                if (!curr_elem && size) {
                    /*
                     * If null pointer found (which should only happen in
                     * an array of pointers), use null placeholder and do
                     * not follow.
                     */
                    inner_field = vmsd_create_fake_nullptr_field(field);
                    is_null = true;
                } else {
                    inner_field = field;
                    is_null = false;
                }

IIUC it'll start to generate nullptr markers if someone by accident removes
the initialization of VDAgentChardev.outbuf someday, expecting it to work
reading this code.

Maybe it's easier we stick with non-NULL for most of vmstates, assert that
the GByteArray* pointer to be valid while get() it.  Same to put().

> +
> +    g_byte_array_set_size(*byte_array, len);
> +    if (len > 0) {
> +        qemu_get_buffer(f, (*byte_array)->data, len);
> +    }
> +    return 0;
> +}
> +
> +static int put_g_byte_array(QEMUFile *f, void *pv, size_t size,
> +                            const VMStateField *field, JSONWriter *vmdesc)
> +{
> +    GByteArray *byte_array = *(GByteArray **)pv;
> +    uint32_t len = byte_array ? byte_array->len : 0;
> +
> +    qemu_put_be32(f, len);
> +    if (len > 0) {
> +        qemu_put_buffer(f, byte_array->data, len);
> +    }
> +
> +    return 0;
> +}
> +
> +const VMStateInfo vmstate_info_g_byte_array = {
> +    .name = "GByteArray",
> +    .get  = get_g_byte_array,
> +    .put  = put_g_byte_array,
> +};

The other thing to mention is, if it's only about one
g_byte_array_set_size() call after getting LEN and before getting the byte
array, we could also use tricks like pre_load(), e.g., instead of the
current VMSD defined as this:

static const VMStateDescription vmstate_vdba = {
    .name = "vdagent/bytearray",
    .version_id = 0,
    .minimum_version_id = 0,
    .fields = (const VMStateField[]) {
        VMSTATE_UINT32(len, GByteArray),
        VMSTATE_VBUFFER_ALLOC_UINT32(data, GByteArray, 0, 0, len),
        VMSTATE_END_OF_LIST()
    }
};

We could change VMSTATE_VBUFFER_ALLOC_UINT32() to be a
VMSTATE_STRUCT_POINTER, then within that the internal vmsd can provide a
pre_load() hook doing g_byte_array_set_size(): since all fields will be
loaded in order, when reaching there it's guaranteed LEN is loaded.

It'll avoid introducing get()/put() completely, IIUC.  But no strong
feelings on either approach.

Thanks,

-- 
Peter Xu