Passing an uninitialised list to visit_start_list() happens to work for
the QObject output visitor because it treats the pointer as an opaque
value and never dereferences it, but the string output visitor expects a
valid list to check if it has more than one element.

The existing code crashes with the string output visitor if the
uninitialised value is non-NULL. Passing an explicit NULL would fix the
crash, but still result in wrong output.

Rework get_prop_array() so that it conforms to the expectations that the
string output visitor has. This includes building a real list first and
using visit_next_list() to iterate it.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1993
Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/core/qdev-properties.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 91632f7be9..840006e953 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -689,23 +689,36 @@ static void get_prop_array(Object *obj, Visitor *v, const char *name,
     Property *prop = opaque;
     uint32_t *alenptr = object_field_prop_ptr(obj, prop);
     void **arrayptr = (void *)obj + prop->arrayoffset;
-    char *elem = *arrayptr;
-    GenericList *list;
-    const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
+    char *elemptr = *arrayptr;
+    ArrayElementList *list = NULL, *elem;
+    ArrayElementList **tail = &list;
+    const size_t size = sizeof(*list);
     int i;
     bool ok;
 
-    if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
+    /* At least the string output visitor needs a real list */
+    for (i = 0; i < *alenptr; i++) {
+        elem = g_new0(ArrayElementList, 1);
+        elem->value = elemptr;
+        elemptr += prop->arrayfieldsize;
+
+        *tail = elem;
+        tail = &elem->next;
+    }
+
+    if (!visit_start_list(v, name, (GenericList **) &list, size, errp)) {
         return;
     }
 
-    for (i = 0; i < *alenptr; i++) {
-        Property elem_prop = array_elem_prop(obj, prop, name, elem);
+    elem = list;
+    while (elem) {
+        Property elem_prop = array_elem_prop(obj, prop, name, elem->value);
         prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
         if (*errp) {
             goto out_obj;
         }
-        elem += prop->arrayfieldsize;
+        elem = (ArrayElementList *) visit_next_list(v, (GenericList*) elem,
+                                                    size);
     }
 
     /* visit_check_list() can only fail for input visitors */
@@ -714,6 +727,12 @@ static void get_prop_array(Object *obj, Visitor *v, const char *name,
 
 out_obj:
     visit_end_list(v, (void**) &list);
+
+    while (list) {
+        elem = list;
+        list = elem->next;
+        g_free(elem);
+    }
 }
 
 static void default_prop_array(ObjectProperty *op, const Property *prop)
-- 
2.42.0

On 21/11/23 18:34, Kevin Wolf wrote:
> Passing an uninitialised list to visit_start_list() happens to work for
> the QObject output visitor because it treats the pointer as an opaque
> value and never dereferences it, but the string output visitor expects a
> valid list to check if it has more than one element.
> 
> The existing code crashes with the string output visitor if the
> uninitialised value is non-NULL. Passing an explicit NULL would fix the
> crash, but still result in wrong output.
> 
> Rework get_prop_array() so that it conforms to the expectations that the
> string output visitor has. This includes building a real list first and
> using visit_next_list() to iterate it.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1993
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   hw/core/qdev-properties.c | 33 ++++++++++++++++++++++++++-------
>   1 file changed, 26 insertions(+), 7 deletions(-)

Per 
https://lore.kernel.org/qemu-devel/CAFXChKJ+OoxXH0Krvvc0-84VwTkat1CciOL=59+gyH+WYWEV_A@mail.gmail.com/
Tested-by: Dan Hoffman <dhoff749@gmail.com>