7:44 a.m.
Every security driver has domainGenSecurityLabel defined.
Coverity complains about a possible leak of seclabel if
!sec_managers[i]->drv->domainGenSecurityLabel is true
and the seclabel might be overwritten by the next iteration
of the loop.
---
src/security/security_manager.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index d68c7e9..24855db 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -512,24 +512,20 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
}
}
- if (!sec_managers[i]->drv->domainGenSecurityLabel) {
- virReportUnsupportedError();
- } else {
- /* The seclabel must be added to @vm prior calling domainGenSecurityLabel
- * which may require seclabel to be presented already */
- if (generated &&
- VIR_APPEND_ELEMENT(vm->seclabels, vm->nseclabels, seclabel) <
0)
- goto cleanup;
-
- if (sec_managers[i]->drv->domainGenSecurityLabel(sec_managers[i], vm)
< 0) {
- if (VIR_DELETE_ELEMENT(vm->seclabels,
- vm->nseclabels -1, vm->nseclabels) < 0)
- vm->nseclabels--;
- goto cleanup;
- }
+ /* The seclabel must be added to @vm prior calling domainGenSecurityLabel
+ * which may require seclabel to be presented already */
+ if (generated &&
+ VIR_APPEND_ELEMENT(vm->seclabels, vm->nseclabels, seclabel) < 0)
+ goto cleanup;
- seclabel = NULL;
+ if (sec_managers[i]->drv->domainGenSecurityLabel(sec_managers[i], vm) <
0) {
+ if (VIR_DELETE_ELEMENT(vm->seclabels,
+ vm->nseclabels -1, vm->nseclabels) < 0)
+ vm->nseclabels--;
+ goto cleanup;
}
+
+ seclabel = NULL;
}
ret = 0;
--
1.8.3.2
8 a.m.
On 04/02/2014 06:44 AM, Ján Tomko wrote:
Every security driver has domainGenSecurityLabel defined.
As currently written. But Dan wrote the manager to be flexible to
future drivers that omit obvious functions.
This patch makes sense for silencing Coverity, but I think it is
incomplete unless you also fix the registration with the manager to
forcefully require that all drivers supply callback functions that we
are going to blindly assume exist, rather than the current status quo of
allowing a driver to omit callbacks even if none of them do. That is,
virSecurityManagerNewDriver() should be taught to require
drv->domainGenSecurityLabel is non-NULL.
Coverity complains about a possible leak of seclabel if
!sec_managers[i]->drv->domainGenSecurityLabel is true
and the seclabel might be overwritten by the next iteration
of the loop.
---
src/security/security_manager.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:21 a.m.
On Wed, Apr 02, 2014 at 07:00:09AM -0600, Eric Blake wrote:
On 04/02/2014 06:44 AM, Ján Tomko wrote:
> Every security driver has domainGenSecurityLabel defined.
As currently written. But Dan wrote the manager to be flexible to
future drivers that omit obvious functions.
This patch makes sense for silencing Coverity, but I think it is
incomplete unless you also fix the registration with the manager to
forcefully require that all drivers supply callback functions that we
are going to blindly assume exist, rather than the current status quo of
allowing a driver to omit callbacks even if none of them do. That is,
virSecurityManagerNewDriver() should be taught to require
drv->domainGenSecurityLabel is non-NULL.
Further this change makes domainGenSecurityLabel an exception to
the rule now, since all others have checks for NULL. I don't object
to removing the checks for NULL and mandating non-NULL, but we should
do it for all the callbacks, or none of the callbacks.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:38 a.m.
On 04/02/2014 03:00 PM, Eric Blake wrote:
On 04/02/2014 06:44 AM, Ján Tomko wrote:
> Every security driver has domainGenSecurityLabel defined.
As currently written. But Dan wrote the manager to be flexible to
future drivers that omit obvious functions.
This patch makes sense for silencing Coverity, but I think it is
incomplete unless you also fix the registration with the manager to
forcefully require that all drivers supply callback functions that we
are going to blindly assume exist, rather than the current status quo of
allowing a driver to omit callbacks even if none of them do. That is,
virSecurityManagerNewDriver() should be taught to require
drv->domainGenSecurityLabel is non-NULL.
I don't feel that strongly about removing the NULL check.
I've sent v2 that keeps the check in place and frees seclabel instead.
Jan
3890
days inactive
3890
days old
3 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko