On 9/21/18 5:29 AM, Michal Privoznik wrote:
This shouldn't be needed per-se. Security manager shouldn't disappear during transactions - it's immutable. However, it doesn't hurt to grab a reference either - transaction code uses it after all.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 5 +++-- src/security/security_selinux.c | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-)
FWIW: I agree w/ Marc's assessment. You need a patch 0.5 ;-) to add the VIR_FREE(list->items) for selinux. It should reference commit ca25026
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2dbaf29ff5..5aea386e7c 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -141,6 +141,7 @@ virSecurityDACChownListFree(void *opaque) VIR_FREE(list->items[i]); } VIR_FREE(list->items); + virObjectUnref(list->manager); VIR_FREE(list); }
@@ -511,12 +512,12 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr) if (VIR_ALLOC(list) < 0) return -1;
- list->manager = mgr; + list->manager = virObjectRef(mgr);
If you move ^^^ to below vvv, then I think the VIR_FREE could still apply. Realistically all that's happening by calling ListFree is the Unref of list->manager. Same for _selinux. In fact, that'd probably be the more proper order with the Ref(mgr) being done. With all that, Reviewed-by: John Ferlan <jferlan@redhat.com> John I also assume you can add the VIR_FREE(list->items) to _selinux with an appropriate commit message as a pre-patch and that too has my R-By.
if (virThreadLocalSet(&chownList, list) < 0) { virReportSystemError(errno, "%s", _("Unable to set thread local variable")); - VIR_FREE(list); + virSecurityDACChownListFree(list); return -1; }
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 056637e4cb..31e42afee7 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -156,6 +156,7 @@ virSecuritySELinuxContextListFree(void *opaque) for (i = 0; i < list->nItems; i++) virSecuritySELinuxContextItemFree(list->items[i]);
+ virObjectUnref(list->manager); VIR_FREE(list); }
@@ -1054,12 +1055,12 @@ virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr) if (VIR_ALLOC(list) < 0) return -1;
- list->manager = mgr; + list->manager = virObjectRef(mgr);
if (virThreadLocalSet(&contextList, list) < 0) { virReportSystemError(errno, "%s", _("Unable to set thread local variable")); - VIR_FREE(list); + virSecuritySELinuxContextListFree(list); return -1; }