[PATCH 0/2] remote: fix object initialization in ACL helpers
Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Peter Krempa <pkrempa@redhat.com> CC: Roman Grigoriev <rgrigoriev@astralinux.ru>
Technically commit 2ecdf259299813c2c674377e22a0acbce5ccbbb2 does not really introduces a leak, but it is incorrect ideologically. Neither function accepting non-const pointer to virDomainDef does not provide any warrantee that the object will not be improved inside. Thus, keeping object model in mind, we must ensure that virDomainDefFree is called over virDomainDef object as a destructor. In order to achieve this we should change pointer declaration inside remoteRelayDomainEventCheckACL remoteRelayDomainQemuMonitorEventCheckACL and assign def->name via strdup. Fixes: 2ecdf259299813c2c674377e22a0acbce5ccbbb2 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 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index aaabd1e56c..3172a632df 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -154,14 +154,14 @@ static bool remoteRelayDomainEventCheckACL(virNetServerClient *client, virConnectPtr conn, virDomainPtr dom) { - g_autofree virDomainDef *def = g_new0(virDomainDef, 1); + g_autoptr(virDomainDef) def = g_new0(virDomainDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virDomainDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def->name = dom->name; + def->name = g_strdup(dom->name); memcpy(def->uuid, dom->uuid, VIR_UUID_BUFLEN); if (!(identity = virNetServerClientGetIdentity(client))) @@ -283,14 +283,14 @@ static bool remoteRelayDomainQemuMonitorEventCheckACL(virNetServerClient *client, virConnectPtr conn, virDomainPtr dom) { - g_autofree virDomainDef *def = g_new0(virDomainDef, 1); + g_autoptr(virDomainDef) def = g_new0(virDomainDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virDomainDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def->name = dom->name; + def->name = g_strdup(dom->name); memcpy(def->uuid, dom->uuid, VIR_UUID_BUFLEN); if (!(identity = virNetServerClientGetIdentity(client))) -- 2.40.1
On Sun, Mar 17, 2024 at 18:08:49 +0100, Denis V. Lunev wrote:
Technically commit 2ecdf259299813c2c674377e22a0acbce5ccbbb2 does not really introduces a leak, but it is incorrect ideologically. Neither function accepting non-const pointer to virDomainDef does not provide any warrantee that the object will not be improved inside.
I don't quite understand what the second sentence is supposed to mean, can you please elaborate the justification?
Thus, keeping object model in mind, we must ensure that virDomainDefFree is called over virDomainDef object as a destructor.
Using the object model as argument would require that you also use 'virDomainDefNew' as "constructor", which IMO in this case would be overkill same as using virDomainDefFree.
In order to achieve this we should change pointer declaration inside remoteRelayDomainEventCheckACL remoteRelayDomainQemuMonitorEventCheckACL and assign def->name via strdup.
Fixes: 2ecdf259299813c2c674377e22a0acbce5ccbbb2
The commit message doesn't really clarify what the actual problem is that this patch is fixing, which is needed in case you are mentioning that some commit is broken/being fixed.
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 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index aaabd1e56c..3172a632df 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -154,14 +154,14 @@ static bool remoteRelayDomainEventCheckACL(virNetServerClient *client, virConnectPtr conn, virDomainPtr dom) { - g_autofree virDomainDef *def = g_new0(virDomainDef, 1); + g_autoptr(virDomainDef) def = g_new0(virDomainDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false;
/* For now, we just create a virDomainDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def->name = dom->name; + def->name = g_strdup(dom->name); memcpy(def->uuid, dom->uuid, VIR_UUID_BUFLEN);
Based on the CC-list I assume that there's a false positive from the static analysis tool which we've got multiple fixes/patches form recently. In such case I think it should be enough to explicitly clear def->name after use before freeing to show that we're intended to just borrow the value without going overkill on fully allocating and freeing the domain object. Alternatively a warning comment can be added too. Either way, please describe the reason for the patch in the commit message as requested above.
On 3/18/24 09:37, Peter Krempa wrote:
On Sun, Mar 17, 2024 at 18:08:49 +0100, Denis V. Lunev wrote:
Technically commit 2ecdf259299813c2c674377e22a0acbce5ccbbb2 does not really introduces a leak, but it is incorrect ideologically. Neither function accepting non-const pointer to virDomainDef does not provide any warrantee that the object will not be improved inside. I don't quite understand what the second sentence is supposed to mean, can you please elaborate the justification?
Thus, keeping object model in mind, we must ensure that virDomainDefFree is called over virDomainDef object as a destructor. Using the object model as argument would require that you also use 'virDomainDefNew' as "constructor", which IMO in this case would be overkill same as using virDomainDefFree.
In order to achieve this we should change pointer declaration inside remoteRelayDomainEventCheckACL remoteRelayDomainQemuMonitorEventCheckACL and assign def->name via strdup.
Fixes: 2ecdf259299813c2c674377e22a0acbce5ccbbb2 The commit message doesn't really clarify what the actual problem is that this patch is fixing, which is needed in case you are mentioning that some commit is broken/being fixed.
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 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index aaabd1e56c..3172a632df 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -154,14 +154,14 @@ static bool remoteRelayDomainEventCheckACL(virNetServerClient *client, virConnectPtr conn, virDomainPtr dom) { - g_autofree virDomainDef *def = g_new0(virDomainDef, 1); + g_autoptr(virDomainDef) def = g_new0(virDomainDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false;
/* For now, we just create a virDomainDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def->name = dom->name; + def->name = g_strdup(dom->name); memcpy(def->uuid, dom->uuid, VIR_UUID_BUFLEN); Based on the CC-list I assume that there's a false positive from the static analysis tool which we've got multiple fixes/patches form recently. Nope. The code is written based on the manual code analysis. Sorry that commit message is not good enough.
In such case I think it should be enough to explicitly clear def->name after use before freeing to show that we're intended to just borrow the value without going overkill on fully allocating and freeing the domain object. Alternatively a warning comment can be added too.
Either way, please describe the reason for the patch in the commit message as requested above.
Please take a look into the attached disassembly of remoteRelayDomainEventCheckACL The key moment is that in the cleanup section we call g_autoptr_cleanup_generic_gfree to cleanup def pointer. That is not bad, but this is fragile. Right now this does not induce any leak, but once we will change something here (adding mode data) or plugins validating ACL will do something with the object (potentially they can) and we will have leaks with the dependent pointers. As far as I could talk about object model, if we call a constructor, we should call the destructor, namely glib_autoptr_cleanup_virDomainDef. That is all beyond my motivation in this patch. Den
On Mon, Mar 18, 2024 at 09:56:35 +0100, Denis V. Lunev wrote:
On 3/18/24 09:37, Peter Krempa wrote:
On Sun, Mar 17, 2024 at 18:08:49 +0100, Denis V. Lunev wrote:
Technically commit 2ecdf259299813c2c674377e22a0acbce5ccbbb2 does not really introduces a leak, but it is incorrect ideologically. Neither function accepting non-const pointer to virDomainDef does not provide any warrantee that the object will not be improved inside. I don't quite understand what the second sentence is supposed to mean, can you please elaborate the justification?
Thus, keeping object model in mind, we must ensure that virDomainDefFree is called over virDomainDef object as a destructor. Using the object model as argument would require that you also use 'virDomainDefNew' as "constructor", which IMO in this case would be overkill same as using virDomainDefFree.
In order to achieve this we should change pointer declaration inside remoteRelayDomainEventCheckACL remoteRelayDomainQemuMonitorEventCheckACL and assign def->name via strdup.
Fixes: 2ecdf259299813c2c674377e22a0acbce5ccbbb2 The commit message doesn't really clarify what the actual problem is that this patch is fixing, which is needed in case you are mentioning that some commit is broken/being fixed.
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 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index aaabd1e56c..3172a632df 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -154,14 +154,14 @@ static bool remoteRelayDomainEventCheckACL(virNetServerClient *client, virConnectPtr conn, virDomainPtr dom) { - g_autofree virDomainDef *def = g_new0(virDomainDef, 1); + g_autoptr(virDomainDef) def = g_new0(virDomainDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virDomainDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def->name = dom->name; + def->name = g_strdup(dom->name); memcpy(def->uuid, dom->uuid, VIR_UUID_BUFLEN); Based on the CC-list I assume that there's a false positive from the static analysis tool which we've got multiple fixes/patches form recently. Nope. The code is written based on the manual code analysis. Sorry that commit message is not good enough.
In such case I think it should be enough to explicitly clear def->name after use before freeing to show that we're intended to just borrow the value without going overkill on fully allocating and freeing the domain object. Alternatively a warning comment can be added too.
Either way, please describe the reason for the patch in the commit message as requested above.
Please take a look into the attached disassembly of remoteRelayDomainEventCheckACL
The key moment is that in the cleanup section we call g_autoptr_cleanup_generic_gfree to cleanup def pointer. That is not bad, but this is fragile. Right now this does not induce any leak, but once we will change something here (adding mode data) or plugins validating ACL will do something with the object (potentially they can) and we will have leaks with the dependent pointers.
Okay, so a comment explaining that we specifically want just the bare shell of the object should be fine, right? All of this weird thing is needed because 'virConnectDomainQemuMonitorEventRegisterCheckACL' needs the name and UUID wrapped in the domain object struct rather than passed directly due to the way the code is generated. Doing a full fake domain object is thus a bit overkill, which was why originally it was being passed in a stack-allocated version of it.
As far as I could talk about object model, if we call a constructor, we should call the destructor, namely glib_autoptr_cleanup_virDomainDef.
My point which I made above is that this does not call the constructor (virDomainDefNew) but just allocates the struct (g_new0(virDomainDef, 1)), thus calling the destructor is in fact wrong ideologically.
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
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(-)
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
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 :|
participants (4)
-
Daniel P. Berrangé -
Denis V. Lunev -
Denis V. Lunev -
Peter Krempa