Commit 2ecdf259299813c2c674377e22a0acbce5ccbbb2 is idealogically corrent,
but unfortunately is incomplete. There are other similar objects in the
module which are used also without proper initialization.
This patch adds proper clauses to
remoteRelayNetworkEventCheckACL
remoteRelayStoragePoolEventCheckACL
remoteRelayNodeDeviceEventCheckACL
remoteRelaySecretEventCheckACL
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Peter Krempa <pkrempa@redhat.com>
CC: Roman Grigoriev <rgrigoriev@astralinux.ru>
---
src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++---------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 3172a632df..01f97bb345 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -180,21 +180,21 @@ static bool
remoteRelayNetworkEventCheckACL(virNetServerClient *client,
virConnectPtr conn, virNetworkPtr net)
{
- virNetworkDef def;
+ g_autoptr(virNetworkDef) def = g_new0(virNetworkDef, 1);
g_autoptr(virIdentity) identity = NULL;
bool ret = false;
/* For now, we just create a virNetworkDef with enough contents to
* satisfy what viraccessdriverpolkit.c references. This is a bit
* fragile, but I don't know of anything better. */
- def.name = net->name;
- memcpy(def.uuid, net->uuid, VIR_UUID_BUFLEN);
+ def->name = g_strdup(net->name);
+ memcpy(def->uuid, net->uuid, VIR_UUID_BUFLEN);
if (!(identity = virNetServerClientGetIdentity(client)))
goto cleanup;
if (virIdentitySetCurrent(identity) < 0)
goto cleanup;
- ret = virConnectNetworkEventRegisterAnyCheckACL(conn, &def);
+ ret = virConnectNetworkEventRegisterAnyCheckACL(conn, def);
cleanup:
ignore_value(virIdentitySetCurrent(NULL));
@@ -206,21 +206,21 @@ remoteRelayStoragePoolEventCheckACL(virNetServerClient *client,
virConnectPtr conn,
virStoragePoolPtr pool)
{
- virStoragePoolDef def;
+ g_autoptr(virStoragePoolDef) def = g_new0(virStoragePoolDef, 1);
g_autoptr(virIdentity) identity = NULL;
bool ret = false;
/* For now, we just create a virStoragePoolDef with enough contents to
* satisfy what viraccessdriverpolkit.c references. This is a bit
* fragile, but I don't know of anything better. */
- def.name = pool->name;
- memcpy(def.uuid, pool->uuid, VIR_UUID_BUFLEN);
+ def->name = g_strdup(pool->name);
+ memcpy(def->uuid, pool->uuid, VIR_UUID_BUFLEN);
if (!(identity = virNetServerClientGetIdentity(client)))
goto cleanup;
if (virIdentitySetCurrent(identity) < 0)
goto cleanup;
- ret = virConnectStoragePoolEventRegisterAnyCheckACL(conn, &def);
+ ret = virConnectStoragePoolEventRegisterAnyCheckACL(conn, def);
cleanup:
ignore_value(virIdentitySetCurrent(NULL));
@@ -232,20 +232,20 @@ remoteRelayNodeDeviceEventCheckACL(virNetServerClient *client,
virConnectPtr conn,
virNodeDevicePtr dev)
{
- virNodeDeviceDef def;
+ g_autoptr(virNodeDeviceDef) def = g_new0(virNodeDeviceDef, 1);
g_autoptr(virIdentity) identity = NULL;
bool ret = false;
/* For now, we just create a virNodeDeviceDef with enough contents to
* satisfy what viraccessdriverpolkit.c references. This is a bit
* fragile, but I don't know of anything better. */
- def.name = dev->name;
+ def->name = g_strdup(dev->name);
if (!(identity = virNetServerClientGetIdentity(client)))
goto cleanup;
if (virIdentitySetCurrent(identity) < 0)
goto cleanup;
- ret = virConnectNodeDeviceEventRegisterAnyCheckACL(conn, &def);
+ ret = virConnectNodeDeviceEventRegisterAnyCheckACL(conn, def);
cleanup:
ignore_value(virIdentitySetCurrent(NULL));
@@ -257,22 +257,22 @@ remoteRelaySecretEventCheckACL(virNetServerClient *client,
virConnectPtr conn,
virSecretPtr secret)
{
- virSecretDef def;
+ g_autoptr(virSecretDef) def = g_new0(virSecretDef, 1);
g_autoptr(virIdentity) identity = NULL;
bool ret = false;
/* For now, we just create a virSecretDef with enough contents to
* satisfy what viraccessdriverpolkit.c references. This is a bit
* fragile, but I don't know of anything better. */
- memcpy(def.uuid, secret->uuid, VIR_UUID_BUFLEN);
- def.usage_type = secret->usageType;
- def.usage_id = secret->usageID;
+ memcpy(def->uuid, secret->uuid, VIR_UUID_BUFLEN);
+ def->usage_type = secret->usageType;
+ def->usage_id = secret->usageID;
if (!(identity = virNetServerClientGetIdentity(client)))
goto cleanup;
if (virIdentitySetCurrent(identity) < 0)
goto cleanup;
- ret = virConnectSecretEventRegisterAnyCheckACL(conn, &def);
+ ret = virConnectSecretEventRegisterAnyCheckACL(conn, def);
cleanup:
ignore_value(virIdentitySetCurrent(NULL));
--
2.40.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
FYI: Gmail decided that your whole series is spam. I'm not sure whether it's just gmail's spam filter being silly or your mail infra has something wrong, but just so you know. On Sun, Mar 17, 2024 at 18:08:50 +0100, Denis V. Lunev wrote: > Commit 2ecdf259299813c2c674377e22a0acbce5ccbbb2 is idealogically corrent, > but unfortunately is incomplete. There are other similar objects in the > module which are used also without proper initialization. The commit you mention was justifying the change from stack-allocated to heap allocated in order to decrease the stack frame size, which was too big due to the fact that 'virDomainDef' is too big. With the other structs that isn't that much of a problem, so I don't think the justification of "Commit 2ecdf259299813 being incomplete" is relevant and thus please add your own justification or adapt what I wrote there as a separate justification, but IMO commit 2ecdf259299813 is not incomplete based on what it tried to do. > > This patch adds proper clauses to > remoteRelayNetworkEventCheckACL > remoteRelayStoragePoolEventCheckACL > remoteRelayNodeDeviceEventCheckACL > remoteRelaySecretEventCheckACL > > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Peter Krempa <pkrempa@redhat.com> > CC: Roman Grigoriev <rgrigoriev@astralinux.ru> > --- > src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++--------------- > 1 file changed, 16 insertions(+), 16 deletions(-) _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On 3/18/24 09:56, Peter Krempa wrote: > FYI: Gmail decided that your whole series is spam. I'm not sure whether > it's just gmail's spam filter being silly or your mail infra has > something wrong, but just so you know. > > On Sun, Mar 17, 2024 at 18:08:50 +0100, Denis V. Lunev wrote: >> Commit 2ecdf259299813c2c674377e22a0acbce5ccbbb2 is idealogically corrent, >> but unfortunately is incomplete. There are other similar objects in the >> module which are used also without proper initialization. > The commit you mention was justifying the change from stack-allocated to > heap allocated in order to decrease the stack frame size, which was too > big due to the fact that 'virDomainDef' is too big. > > With the other structs that isn't that much of a problem, so I don't > think the justification of "Commit 2ecdf259299813 being incomplete" is > relevant and thus please add your own justification or adapt what I > wrote there as a separate justification, but IMO commit 2ecdf259299813 > is not incomplete based on what it tried to do. Original commit has 2 motivations: * reduce stack size * properly initialize the object Right now we pass half-initialized object into the engine. We should either memset() or allocate/free the object. I thought that allocation is better as we could have object filled with more data inside validators. Den _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On Mon, Mar 18, 2024 at 10:00:25AM +0100, Denis V. Lunev wrote: > On 3/18/24 09:56, Peter Krempa wrote: > > FYI: Gmail decided that your whole series is spam. I'm not sure whether > > it's just gmail's spam filter being silly or your mail infra has > > something wrong, but just so you know. > > > > On Sun, Mar 17, 2024 at 18:08:50 +0100, Denis V. Lunev wrote: > > > Commit 2ecdf259299813c2c674377e22a0acbce5ccbbb2 is idealogically corrent, > > > but unfortunately is incomplete. There are other similar objects in the > > > module which are used also without proper initialization. > > The commit you mention was justifying the change from stack-allocated to > > heap allocated in order to decrease the stack frame size, which was too > > big due to the fact that 'virDomainDef' is too big. > > > > With the other structs that isn't that much of a problem, so I don't > > think the justification of "Commit 2ecdf259299813 being incomplete" is > > relevant and thus please add your own justification or adapt what I > > wrote there as a separate justification, but IMO commit 2ecdf259299813 > > is not incomplete based on what it tried to do. > Original commit has 2 motivations: > * reduce stack size > * properly initialize the object > > Right now we pass half-initialized object into the > engine. We should either memset() or allocate/free > the object. I thought that allocation is better as > we could have object filled with more data inside > validators. Yes, stack allocating this struct is gross :-( 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 :| _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2026 Red Hat, Inc.