[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: |
Sameeh Jubran |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] qga-win: prevent crash when executing fsinfo command |
Date: |
Mon, 16 Jul 2018 14:42:14 +0300 |
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>.*