On Mon, Sep 05, 2022 at 03:57:03PM +0200, Kristina Hanicova wrote:
> This patch adds the generalized job object into the domain object
> so that it can be used by all drivers without the need to extract
> it from the private data.
>
> Because of this, the job object needs to be created and set
> during the creation of the domain object. This patch also extends
> xmlopt with possible job config containing virDomainJobObj
> callbacks, its private data callbacks and one variable
> (maxQueuedJobs).
>
> This patch includes:
> * addition of virDomainJobObj into virDomainObj (used in the
> following patches)
> * extending xmlopt with job config structure
> * new function for freeing the virDomainJobObj
Since this series was pushed, virsh unit tests are failing on
some platforms:
# ./build/tools/virsh -c test:///default uri
test:///default
Segmentation fault (core dumped)
In fact after checking with valgrind, they should be failing on all
platforms, but most aren't crashing on the use-after-free
$ valgrind ./build/tools/virsh -c test:///default uri
test:///default
==2099625== Invalid read of size 8
==2099625== at 0x4A1FCCB: virDomainObjResetAsyncJob (virdomainjob.c:184)
==2099625== by 0x4A1FDF0: virDomainObjClearJob (virdomainjob.c:224)
==2099625== by 0x4A1FE8D: virDomainJobObjFree (virdomainjob.c:240)
==2099625== by 0x499017D: vir_object_finalize (virobject.c:323)
==2099625== by 0x52F8D31: g_object_unref (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x49907D7: virObjectUnref (virobject.c:377)
==2099625== by 0x4F357D2: ??? (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x4F39772: g_hash_table_unref (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x499017D: vir_object_finalize (virobject.c:323)
==2099625== by 0x52F8D31: g_object_unref (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x49907D7: virObjectUnref (virobject.c:377)
==2099625== by 0x4B6F18A: testDriverDispose (test_driver.c:158)
==2099625== Address 0x69aa1e8 is 392 bytes inside a block of size 464 free'd
==2099625== at 0x48480E4: free (vg_replace_malloc.c:872)
==2099625== by 0x4F4BB8C: g_free (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x4F67013: g_slice_free1 (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x530D7D3: g_type_free_instance (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x49907D7: virObjectUnref (virobject.c:377)
==2099625== by 0x4B6F17E: testDriverDispose (test_driver.c:157)
==2099625== by 0x499017D: vir_object_finalize (virobject.c:323)
==2099625== by 0x52F8D31: g_object_unref (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x49907D7: virObjectUnref (virobject.c:377)
==2099625== by 0x4B6EE62: testDriverCloseInternal (test_driver.c:1498)
==2099625== by 0x4B6EEE2: testConnectClose (test_driver.c:1543)
==2099625== by 0x4BBE495: virConnectDispose (datatypes.c:174)
==2099625== Block was alloc'd at
==2099625== at 0x484586F: malloc (vg_replace_malloc.c:381)
==2099625== by 0x4F4F278: g_malloc (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x4F67BA5: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x4F69BCC: g_slice_alloc0 (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x5313016: g_type_create_instance (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x52FAE37: ??? (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x52FC080: g_object_new_with_properties (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x52FCB20: g_object_new (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x4990723: virObjectNew (virobject.c:252)
==2099625== by 0x49E0DD6: virDomainXMLOptionNew (domain_conf.c:1616)
==2099625== by 0x4B6F098: testDriverNew (test_driver.c:463)
==2099625== by 0x4B7709C: testOpenDefault (test_driver.c:1397)
==2099625== by 0x4B7709C: testConnectOpen (test_driver.c:1522)
>
> Signed-off-by: Kristina Hanicova <khanicov@xxxxxxxxxx>
> ---
> src/bhyve/bhyve_domain.c | 2 +-
> src/ch/ch_conf.c | 2 +-
> src/conf/domain_conf.c | 13 ++++++++++++-
> src/conf/domain_conf.h | 16 +++++++++++++++-
> src/conf/virconftypes.h | 2 ++
> src/conf/virdomainjob.c | 11 +++++++++++
> src/conf/virdomainjob.h | 5 ++++-
> src/hyperv/hyperv_driver.c | 2 +-
> src/libvirt_private.syms | 1 +
> src/libxl/libxl_conf.c | 2 +-
> src/lxc/lxc_conf.c | 2 +-
> src/openvz/openvz_conf.c | 2 +-
> src/qemu/qemu_conf.c | 3 ++-
> src/qemu/qemu_process.c | 2 +-
> src/security/virt-aa-helper.c | 2 +-
> src/test/test_driver.c | 2 +-
> src/vbox/vbox_common.c | 2 +-
> src/vmware/vmware_driver.c | 2 +-
> src/vmx/vmx.c | 2 +-
> src/vz/vz_driver.c | 2 +-
> tests/bhyveargv2xmltest.c | 2 +-
> tests/testutils.c | 2 +-
> 22 files changed, 62 insertions(+), 19 deletions(-)
>
> diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
> index 69555a3efc..b7b2db57b8 100644
> --- a/src/bhyve/bhyve_domain.c
> +++ b/src/bhyve/bhyve_domain.c
> @@ -221,7 +221,7 @@ virBhyveDriverCreateXMLConf(struct _bhyveConn *driver)
> return virDomainXMLOptionNew(&virBhyveDriverDomainDefParserConfig,
> &virBhyveDriverPrivateDataCallbacks,
> &virBhyveDriverDomainXMLNamespace,
> - NULL, NULL);
> + NULL, NULL, NULL);
> }
>
>
> diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c
> index 775596e9f5..0d07fa270c 100644
> --- a/src/ch/ch_conf.c
> +++ b/src/ch/ch_conf.c
> @@ -110,7 +110,7 @@ chDomainXMLConfInit(virCHDriver *driver)
> virCHDriverDomainDefParserConfig.priv = driver;
> return virDomainXMLOptionNew(&virCHDriverDomainDefParserConfig,
> &virCHDriverPrivateDataCallbacks,
> - NULL, NULL, NULL);
> + NULL, NULL, NULL, NULL);
> }
>
> virCHDriverConfig *
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 237f1d6835..35ac2bb1c2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1604,7 +1604,8 @@ virDomainXMLOptionNew(virDomainDefParserConfig *config,
> virDomainXMLPrivateDataCallbacks *priv,
> virXMLNamespace *xmlns,
> virDomainABIStability *abi,
> - virSaveCookieCallbacks *saveCookie)
> + virSaveCookieCallbacks *saveCookie,
> + virDomainJobObjConfig *jobConfig)
> {
> virDomainXMLOption *xmlopt;
>
> @@ -1629,6 +1630,9 @@ virDomainXMLOptionNew(virDomainDefParserConfig *config,
> if (saveCookie)
> xmlopt->saveCookie = *saveCookie;
>
> + if (jobConfig)
> + xmlopt->jobObjConfig = *jobConfig;
> +
> /* Technically this forbids to use one of Xerox's MAC address prefixes in
> * our hypervisor drivers. This shouldn't ever be a problem.
> *
> @@ -3857,6 +3861,7 @@ static void virDomainObjDispose(void *obj)
> virDomainObjDeprecationFree(dom);
> virDomainSnapshotObjListFree(dom->snapshots);
> virDomainCheckpointObjListFree(dom->checkpoints);
> + virDomainJobObjFree(dom->job);
> }
>
> virDomainObj *
> @@ -3889,6 +3894,12 @@ virDomainObjNew(virDomainXMLOption *xmlopt)
> if (!(domain->checkpoints = virDomainCheckpointObjListNew()))
> goto error;
>
> + domain->job = g_new0(virDomainJobObj, 1);
> + if (virDomainObjInitJob(domain->job,
> + &xmlopt->jobObjConfig.cb,
> + &xmlopt->jobObjConfig.jobDataPrivateCb) < 0)
> + goto error;
> +
> virObjectLock(domain);
> virDomainObjSetState(domain, VIR_DOMAIN_SHUTOFF,
> VIR_DOMAIN_SHUTOFF_UNKNOWN);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 2b1497d78d..42fa2a4400 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -56,6 +56,7 @@
> #include "virsavecookie.h"
> #include "virresctrl.h"
> #include "virenum.h"
> +#include "virdomainjob.h"
>
> /* Flags for the 'type' field in virDomainDeviceDef */
> typedef enum {
> @@ -3093,6 +3094,8 @@ struct _virDomainObj {
> virObjectLockable parent;
> virCond cond;
>
> + virDomainJobObj *job;
> +
> pid_t pid; /* 0 for no PID, avoid negative values like -1 */
> virDomainStateReason state;
>
> @@ -3277,11 +3280,19 @@ struct _virDomainABIStability {
> virDomainABIStabilityDomain domain;
> };
>
> +
> +struct _virDomainJobObjConfig {
> + virDomainObjPrivateJobCallbacks cb;
> + virDomainJobDataPrivateDataCallbacks jobDataPrivateCb;
> + unsigned int maxQueuedJobs;
> +};
> +
> virDomainXMLOption *virDomainXMLOptionNew(virDomainDefParserConfig *config,
> virDomainXMLPrivateDataCallbacks *priv,
> virXMLNamespace *xmlns,
> virDomainABIStability *abi,
> - virSaveCookieCallbacks *saveCookie);
> + virSaveCookieCallbacks *saveCookie,
> + virDomainJobObjConfig *jobConfig);
>
> virSaveCookieCallbacks *
> virDomainXMLOptionGetSaveCookie(virDomainXMLOption *xmlopt);
> @@ -3321,6 +3332,9 @@ struct _virDomainXMLOption {
>
> /* Snapshot postparse callbacks */
> virDomainMomentPostParseCallback momentPostParse;
> +
> + /* virDomainJobObj callbacks, private data callbacks and defaults */
> + virDomainJobObjConfig jobObjConfig;
> };
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainXMLOption, virObjectUnref);
>
> diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
> index c3f1c5fa01..154805091a 100644
> --- a/src/conf/virconftypes.h
> +++ b/src/conf/virconftypes.h
> @@ -150,6 +150,8 @@ typedef struct _virDomainIdMapEntry virDomainIdMapEntry;
>
> typedef struct _virDomainInputDef virDomainInputDef;
>
> +typedef struct _virDomainJobObjConfig virDomainJobObjConfig;
> +
> typedef struct _virDomainKeyWrapDef virDomainKeyWrapDef;
>
> typedef struct _virDomainLeaseDef virDomainLeaseDef;
> diff --git a/src/conf/virdomainjob.c b/src/conf/virdomainjob.c
> index 5ab4bdc18b..a073ff08cd 100644
> --- a/src/conf/virdomainjob.c
> +++ b/src/conf/virdomainjob.c
> @@ -13,6 +13,7 @@
> #include "virthreadjob.h"
> #include "virlog.h"
> #include "virtime.h"
> +#include "domain_conf.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> @@ -230,6 +231,16 @@ virDomainObjClearJob(virDomainJobObj *job)
> g_clear_pointer(&job->privateData, job->cb->freeJobPrivate);
> }
>
> +void
> +virDomainJobObjFree(virDomainJobObj *job)
> +{
> + if (!job)
> + return;
> +
> + virDomainObjClearJob(job);
> + g_free(job);
> +}
> +
> bool
> virDomainTrackJob(virDomainJob job)
> {
> diff --git a/src/conf/virdomainjob.h b/src/conf/virdomainjob.h
> index bdfdc91935..091d951aa6 100644
> --- a/src/conf/virdomainjob.h
> +++ b/src/conf/virdomainjob.h
> @@ -11,7 +11,8 @@
> #include "virenum.h"
> #include "virthread.h"
> #include "virbuffer.h"
> -#include "domain_conf.h"
> +#include "virconftypes.h"
> +#include "virxml.h"
>
> #define JOB_MASK(job) (job == 0 ? 0 : 1 << (job - 1))
> #define VIR_JOB_DEFAULT_MASK \
> @@ -227,6 +228,8 @@ int virDomainObjPreserveJob(virDomainJobObj *currJob,
> void virDomainObjClearJob(virDomainJobObj *job);
> G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(virDomainJobObj, virDomainObjClearJob);
>
> +void virDomainJobObjFree(virDomainJobObj *job);
> +
> bool virDomainTrackJob(virDomainJob job);
>
> bool virDomainNestedJobAllowed(virDomainJobObj *jobs, virDomainJob newJob);
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 288c01ad14..3929e27e09 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -1766,7 +1766,7 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
> goto cleanup;
>
> /* init xmlopt for domain XML */
> - priv->xmlopt = virDomainXMLOptionNew(&hypervDomainDefParserConfig, NULL, NULL, NULL, NULL);
> + priv->xmlopt = virDomainXMLOptionNew(&hypervDomainDefParserConfig, NULL, NULL, NULL, NULL, NULL);
>
> if (hypervGetOperatingSystem(priv, &os) < 0)
> goto cleanup;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5077db9c6b..cd0b94297c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1182,6 +1182,7 @@ virDomainAsyncJobTypeToString;
> virDomainJobDataCopy;
> virDomainJobDataFree;
> virDomainJobDataInit;
> +virDomainJobObjFree;
> virDomainJobStatusToType;
> virDomainJobTypeFromString;
> virDomainJobTypeToString;
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 06f338ef90..918303c8d0 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -2486,5 +2486,5 @@ libxlCreateXMLConf(libxlDriverPrivate *driver)
> return virDomainXMLOptionNew(&libxlDomainDefParserConfig,
> &libxlDomainXMLPrivateDataCallbacks,
> &libxlDriverDomainXMLNamespace,
> - NULL, NULL);
> + NULL, NULL, NULL);
> }
> diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c
> index 7834801fb5..fefe63bf20 100644
> --- a/src/lxc/lxc_conf.c
> +++ b/src/lxc/lxc_conf.c
> @@ -189,7 +189,7 @@ lxcDomainXMLConfInit(virLXCDriver *driver, const char *defsecmodel)
> return virDomainXMLOptionNew(&virLXCDriverDomainDefParserConfig,
> &virLXCDriverPrivateDataCallbacks,
> &virLXCDriverDomainXMLNamespace,
> - NULL, NULL);
> + NULL, NULL, NULL);
> }
>
>
> diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
> index c94f9b8577..c28d0e9f43 100644
> --- a/src/openvz/openvz_conf.c
> +++ b/src/openvz/openvz_conf.c
> @@ -1062,5 +1062,5 @@ virDomainXMLOption *openvzXMLOption(struct openvz_driver *driver)
> {
> openvzDomainDefParserConfig.priv = driver;
> return virDomainXMLOptionNew(&openvzDomainDefParserConfig,
> - NULL, NULL, NULL, NULL);
> + NULL, NULL, NULL, NULL, NULL);
> }
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 3b75cdeb95..2809ae53b9 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1278,7 +1278,8 @@ virQEMUDriverCreateXMLConf(virQEMUDriver *driver,
> &virQEMUDriverPrivateDataCallbacks,
> &virQEMUDriverDomainXMLNamespace,
> &virQEMUDriverDomainABIStability,
> - &virQEMUDriverDomainSaveCookie);
> + &virQEMUDriverDomainSaveCookie,
> + NULL);
> }
>
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 32f03ff79a..251bca4bdf 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -9295,7 +9295,7 @@ qemuProcessQMPConnectMonitor(qemuProcessQMP *proc)
> monConfig.data.nix.path = proc->monpath;
> monConfig.data.nix.listen = false;
>
> - if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) ||
> + if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL, NULL)) ||
> !(proc->vm = virDomainObjNew(xmlopt)) ||
> !(proc->vm->def = virDomainDefNew(xmlopt)))
> return -1;
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 2d0bc99c73..f338488da3 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -632,7 +632,7 @@ get_definition(vahControl * ctl, const char *xmlStr)
> }
>
> if (!(ctl->xmlopt = virDomainXMLOptionNew(&virAAHelperDomainDefParserConfig,
> - NULL, NULL, NULL, NULL))) {
> + NULL, NULL, NULL, NULL, NULL))) {
> vah_error(ctl, 0, _("Failed to create XML config object"));
> return -1;
> }
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 641a141b6a..686ff051a8 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -460,7 +460,7 @@ testDriverNew(void)
> if (!(ret = virObjectLockableNew(testDriverClass)))
> return NULL;
>
> - if (!(ret->xmlopt = virDomainXMLOptionNew(&config, &privatecb, &ns, NULL, NULL)) ||
> + if (!(ret->xmlopt = virDomainXMLOptionNew(&config, &privatecb, &ns, NULL, NULL, NULL)) ||
> !(ret->eventState = virObjectEventStateNew()) ||
> !(ret->ifaces = virInterfaceObjListNew()) ||
> !(ret->domains = virDomainObjListNew()) ||
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index e249980195..bd77641d39 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -143,7 +143,7 @@ vboxDriverObjNew(void)
>
> if (!(driver->caps = vboxCapsInit()) ||
> !(driver->xmlopt = virDomainXMLOptionNew(&vboxDomainDefParserConfig,
> - NULL, NULL, NULL, NULL)))
> + NULL, NULL, NULL, NULL, NULL)))
> goto cleanup;
>
> return driver;
> diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
> index 2a18d73988..3c578434f3 100644
> --- a/src/vmware/vmware_driver.c
> +++ b/src/vmware/vmware_driver.c
> @@ -139,7 +139,7 @@ vmwareDomainXMLConfigInit(struct vmware_driver *driver)
> .free = vmwareDataFreeFunc };
> vmwareDomainDefParserConfig.priv = driver;
> return virDomainXMLOptionNew(&vmwareDomainDefParserConfig, &priv,
> - NULL, NULL, NULL);
> + NULL, NULL, NULL, NULL);
> }
>
> static virDrvOpenStatus
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index fa5cc56146..bf0dba17d8 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -696,7 +696,7 @@ virVMXDomainXMLConfInit(virCaps *caps)
> {
> virVMXDomainDefParserConfig.priv = caps;
> return virDomainXMLOptionNew(&virVMXDomainDefParserConfig, NULL,
> - &virVMXDomainXMLNamespace, NULL, NULL);
> + &virVMXDomainXMLNamespace, NULL, NULL, NULL);
> }
>
> char *
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index 017c084ede..571d895167 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -331,7 +331,7 @@ vzDriverObjNew(void)
> if (!(driver->caps = vzBuildCapabilities()) ||
> !(driver->xmlopt = virDomainXMLOptionNew(&vzDomainDefParserConfig,
> &vzDomainXMLPrivateDataCallbacksPtr,
> - NULL, NULL, NULL)) ||
> + NULL, NULL, NULL, NULL)) ||
> !(driver->domains = virDomainObjListNew()) ||
> !(driver->domainEventState = virObjectEventStateNew()) ||
> (vzInitVersion(driver) < 0) ||
> diff --git a/tests/bhyveargv2xmltest.c b/tests/bhyveargv2xmltest.c
> index 2ccc1379b9..92189a2e58 100644
> --- a/tests/bhyveargv2xmltest.c
> +++ b/tests/bhyveargv2xmltest.c
> @@ -113,7 +113,7 @@ mymain(void)
> if ((driver.caps = virBhyveCapsBuild()) == NULL)
> return EXIT_FAILURE;
>
> - if ((driver.xmlopt = virDomainXMLOptionNew(NULL, NULL,
> + if ((driver.xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL,
> NULL, NULL, NULL)) == NULL)
> return EXIT_FAILURE;
>
> diff --git a/tests/testutils.c b/tests/testutils.c
> index 30f91dcd28..8fe624f029 100644
> --- a/tests/testutils.c
> +++ b/tests/testutils.c
> @@ -989,7 +989,7 @@ static virDomainDefParserConfig virTestGenericDomainDefParserConfig = {
> virDomainXMLOption *virTestGenericDomainXMLConfInit(void)
> {
> return virDomainXMLOptionNew(&virTestGenericDomainDefParserConfig,
> - NULL, NULL, NULL, NULL);
> + NULL, NULL, NULL, NULL, NULL);
> }
>
>
> --
> 2.37.2
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
