Re: [Qemu-devel] [PATCH 1/2] qga-win: prevent crash when executing fsinfo command

[Eric Blake]

On 06/26/2018 10:10 AM, Sameeh Jubran wrote:
> From: Sameeh Jubran <address@hidden>
>
> The fsinfo command is currently implemented for Windows only and it's disk
> parameter can be enabled by adding the define "CONFIG_QGA_NTDDSCSI" to the qga
> code. When enabled and executed the qemu-ga crashed with the following message:
>
> ------------------------------------------------
> File qapi/qapi-visit-core.c, Line 49
>
> Expression: !(v->type & VISITOR_OUTPUT) || *obj)
> ------------------------------------------------
>
> After some digging, turns out that the GuestPCIAddress is null and the
> qapi visitor doesn't like that, so we can always allocate it instead and
> initiate all it's members to -1.

Adding Markus for a qapi back-compat question.

Is faking an invalid address better than making the output optional instead?

> +++ b/qga/commands-win32.c
> @@ -485,6 +485,11 @@ static GuestPCIAddress *get_pci_info(char *guid, Error 
> **errp)
>       char *buffer = NULL;
>       GuestPCIAddress *pci = NULL;
>       char *name = g_strdup(&guid[4]);
> +    pci = g_malloc0(sizeof(*pci));
> +    pci->domain = -1;
> +    pci->slot = -1;
> +    pci->function = -1;
> +    pci->bus = -1;

Right now, we have:

##
# @GuestDiskAddress:
#
# @pci-controller: controller's PCI address
# @bus-type: bus type
# @bus: bus id
# @target: target id
# @unit: unit id
#
# Since: 2.2
##
{ 'struct': 'GuestDiskAddress',
  'data': {'pci-controller': 'GuestPCIAddress',
           'bus-type': 'GuestDiskBusType',
           'bus': 'int', 'target': 'int', 'unit': 'int'} }

and the problem you ran into is that under certain conditions, we have 
no idea what to populate in GuestPCIAddress.  Your patch populates 
garbage instead; but I'm wondering if it would be better to instead mark 
pci-controller as optional, where code that CAN populate it would set 
has_pci_controller=true, and the code that crashed will now work by 
leaving the struct NULL (and has_pci_controller=false).  But that 
removes output that the client may expect to be present, hence, this is 
a back-compat question of the best way to approach this.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org