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

[Sameeh Jubran]

I've found another bug which is related to this one, where pci_controller
might also be null:
in the function "build_guest_disk_info" in "qga/commands-win32.c"

if (bus == BusTypeScsi || bus == BusTypeAta || bus == BusTypeRAID

#if (_WIN32_WINNT >= 0x0600)

/* This bus type is not supported before Windows Server 2003 SP1 */

|| bus == BusTypeSas

#endif

) {

/* We are able to use the same ioctls for different bus types

* according to Microsoft docs

* https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */

if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad,

sizeof(SCSI_ADDRESS), &len, NULL)) {

disk->unit = addr.Lun;

disk->target = addr.TargetId;

disk->bus = addr.PathId;

disk->pci_controller = get_pci_info(name, errp);

}

/* We do not set error in this case, because we still have enough

* information about volume. */

} else {

disk->pci_controller = NULL;

}

This can be easily prevented by always calling "get_pci_info(name, errp);",
is this a separate patch, or should be combined with this one? what do you
think?

On Tue, Jul 10, 2018 at 1:58 AM, Michael Roth <address@hidden>
wrote:

> 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.
>
>


-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*