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

[Arun Menon <armenon@redhat.com>]

Hi Peter,

On Tue, Apr 14, 2026 at 06:28:52PM -0400, Peter Xu wrote:
> 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.

Thank you for the detailed explanation! I completely see the design flaw
now regarding the nullptr marker.

I am more inclined to make the one liner change of adding asserts,
because I am not sure where VMStateDescription of GByteArray should
live. Since it is an external GLib type, it does not have a dedicated
QEMU wrapper file in util/ directory. Placing it inside
migration/vmstate-types.c feels a bit out of place as that file is,
AFAICT, reserved for low-level VMStateInfo primitives.

I want to use the same GByteArray VMStateInfo to migrate TPM CRBState
struct in another change.

> 
> Thanks,
> 
> -- 
> Peter Xu
> 

Regards,
Arun Menon