qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael Roth
Subject: Re: [Qemu-devel] [PATCH 1/2] qga-win: prevent crash when executing fsinfo command
Date: Mon, 09 Jul 2018 17:58:45 -0500
User-agent: alot/0.6

Quoting Sameeh Jubran (2018-06-28 18:33:58)
> 
> 
> On Fri, Jun 29, 2018 at 12:44 AM, Eric Blake <address@hidden> wrote:
> 
>     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.
> 
> Since all of the fields are ints, I believe that "-1" is very informative even
> though I don't like this approach but it does preserve backward compatibility 
> as you've already clarified.
> 
> I don't want to assume anything but this bug has been laying around for quite 
> a
> while and no one complained, moreover,  the disk option is not enabled by
> default in upstream code so I don't think that backward compatibility is
> actually disturbed but then again this is just an assumption.
> 
> One more thing to consider is the second patch of this series which actually
> was the root cause of why we didn't allocate the " GuestDiskAddress" struct
> which is some registry keys being absent for the specific device which would
> leave us with two options:
> 1. Don not return anything for all of the fields (leave it null)
> 2. Return partial information
> I think that the second option is preferable for obvious reasons.
> 
> I am not that familiar with schema files, but it is possible to make int 
> fields
> optional too?
> 
> I can live with either approaches, maintainers, it's your call :)

IMO both approaches potentially break clients, but making the field
optional in this case would be likely to trigger a segfault for any code
that relies on GuestPCIAddress being populated, whereas -1 would likely
just propagate through the stack as a signed integer as the schema calls
for. Lack of -1/undefined handling further up the stack could still
cause breakage but the severity seems lesser. I'm inclined to go this
route for 2.13. If there are no objections I plan to include this in a
pull for 2.13-rc1.

> 
> Thanks for the review!
> 
>    
>     --
>     Eric Blake, Principal Software Engineer
>     Red Hat, Inc.           +1-919-301-3266
>     Virtualization:  qemu.org | libvirt.org
> 
> 
> 
> 
> --
> Respectfully,
> Sameeh Jubran
> Linkedin
> Software Engineer @ Daynix.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]