Re: [PATCH] conf: Fix rawio/sgio checks for non-scsi hostdev devices

On Tue, Jul 02, 2024 at 18:49:13 +0530, Rayhan Faizel wrote: > The current hostdev parsing logic sets rawio or sgio even if the hostdev type > is not 'scsi'. The rawio field in virDomainHostdevSubsysSCSI overlaps with > wwpn field in virDomainHostdevSubsysSCSIVHost, consequently setting a bogus > pointer value such as 0x1 or 0x2 from virDomainHostdevSubsysSCSIVHost's > point of view. This leads to a segmentation fault when it attempts to free > wwpn. > > While setting sgio does not appear to crash, it shares the same flawed logic > as setting rawio. > > Instead, we test the presence of their xpaths along with the hostdev type > before setting either attributes. This patch also adds two test cases to > exercise both scenarios. Fixes: bdb95b520c53f9bacc6504fc51381bac4813be38 > Signed-off-by: Rayhan Faizel <rayhan.faizel(a)gmail.com&gt; > --- > src/conf/domain_conf.c | 33 ++++++++------- > ...scsi-vhost-rawio-invalid.x86_64-latest.err | 1 + > .../hostdev-scsi-vhost-rawio-invalid.xml | 41 +++++++++++++++++++ > ...-scsi-vhost-sgio-invalid.x86_64-latest.err | 1 + > .../hostdev-scsi-vhost-sgio-invalid.xml | 41 +++++++++++++++++++ > tests/qemuxmlconftest.c | 2 + > 6 files changed, 102 insertions(+), 17 deletions(-) > create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.x86_64-latest.err > create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.xml > create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.x86_64-latest.err > create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.xml > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 334152c4ff..5f8c6f2672 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -6220,7 +6220,6 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, > virDomainHostdevSubsysMediatedDev *mdevsrc = &def->source.subsys.u.mdev; > virTristateBool managed; > g_autofree char *model = NULL; > - int rv; > > /* @managed can be read from the xml document - it is always an > * attribute of the toplevel element, no matter what type of > @@ -6256,33 +6255,33 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, > return -1; > } > > - if ((rv = virXMLPropEnum(node, "sgio", > - virDomainDeviceSGIOTypeFromString, > - VIR_XML_PROP_NONZERO, > - &scsisrc->sgio)) < 0) { > + if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && > + virXPathBoolean("boolean(./@sgio)", ctxt)) { Rather than doing an extra XPath query I'd prefer if you fix it by introducing temporary variables which are filled instead of '&scsisrc->sgio' and then the value is transferred when the check passes.