[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 07/10] selinux: Resolve resource leak using the default disk label



On 01/17/13 20:17, John Ferlan wrote:
Commit id a994ef2d1 changed the mechanism to store/update the default
security label from using disk->seclabels[0] to allocating one on the
fly. That change allocated the label, but never saved it.  This patch
will save the label.
---
  src/conf/domain_conf.c          | 29 +++++++++++++++++++++++++++++
  src/conf/domain_conf.h          |  3 +++
  src/security/security_selinux.c |  6 +++---
  3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0b9ba13..0b3427f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16064,3 +16064,32 @@ virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model)

      return seclabel;
  }
+
+virSecurityDeviceLabelDefPtr
+virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model)
+{
+    virSecurityDeviceLabelDefPtr seclabel = NULL;
+
+    if (VIR_ALLOC(seclabel) < 0) {
+        virReportOOMError();

all ...

+        return NULL;
+    }
+
+    if (model) {
+        seclabel->model = strdup(model);
+        if (seclabel->model == NULL) {
+            virReportOOMError();
+            virSecurityDeviceLabelDefFree(seclabel);

.. these ..

+            return NULL;
+        }
+    }
+
+    if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) {
+        virReportOOMError();
+        virSecurityDeviceLabelDefFree(seclabel);

... error paths can be merged under a no_memory label saving a few lines of code. (virSecurityDeviceLabelDefFree handles NULL arguments gracefully)

+        return NULL;
+    }
+    def->seclabels[def->nseclabels - 1] = seclabel;
+
+    return seclabel;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ce36003..9770ffb 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2222,6 +2222,9 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model);
  virSecurityLabelDefPtr
  virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model);

+virSecurityDeviceLabelDefPtr
+virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model);
+
  typedef const char* (*virEventActionToStringFunc)(int type);
  typedef int (*virEventActionFromStringFunc)(const char *type);

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index b5e1a9a..511e923 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1095,10 +1095,10 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk,
      if (ret == 1 && !disk_seclabel) {
          /* If we failed to set a label, but virt_use_nfs let us
           * proceed anyway, then we don't need to relabel later.  */
-        if (VIR_ALLOC(disk_seclabel) < 0) {
-            virReportOOMError();
+        disk_seclabel =
+            virDomainDiskDefAddSecurityLabelDef(disk, SECURITY_SELINUX_NAME);
+        if (!disk_seclabel)
              return -1;
-        }
          disk_seclabel->norelabel = true;
          ret = 0;
      }


Otherwise this seems to me be a good approach how to fix it, but I'm not that familiar with the selinux code. Hopefully, if this fix is wrong somebody will chime in on the v2.

Peter


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]