>From 2bc32f089b76bb519d77d7ff687f0d158d452c28 Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzidek@redhat.com>
Date: Wed, 6 Mar 2013 16:46:32 +0100
Subject: [PATCH 1/2] File descriptor leak in nss responder.

File descriptors leaked every time sss_mmap_cache_reinit was
called and also the old memory cache was still maped in memory
(munmap was not called). This patch adds destructor for memory
cache context to call close() and munmap() automaticly.

https://fedorahosted.org/sssd/ticket/1826
---
 src/responder/nss/nsssrv_mmap_cache.c | 76 +++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
index f72923e..e0c31cc 100644
--- a/src/responder/nss/nsssrv_mmap_cache.c
+++ b/src/responder/nss/nsssrv_mmap_cache.c
@@ -801,7 +801,8 @@ static errno_t sss_mc_set_recycled(int fd)
 /*
  * When we (re)create a new file we must mark the current file as recycled
  * so active clients will abandon its use asap.
- * We unlink the current file and make a new one
+ * We unlink the current file and make a new one.
+ * We do this when the call to open() on the current file was succesful.
  */
 static errno_t sss_mc_create_file(struct sss_mc_ctx *mc_ctx)
 {
@@ -826,14 +827,15 @@ static errno_t sss_mc_create_file(struct sss_mc_ctx *mc_ctx)
         }
 
         close(ofd);
-    }
 
-    errno = 0;
-    ret = unlink(mc_ctx->file);
-    if (ret == -1) {
-        ret = errno;
-        DEBUG(SSSDBG_TRACE_FUNC, ("Failed to rm mmap file %s: %d(%s)\n",
-                                  mc_ctx->file, ret, strerror(ret)));
+        errno = 0;
+        ret = unlink(mc_ctx->file);
+        if (ret == -1) {
+            ret = errno;
+            DEBUG(SSSDBG_TRACE_FUNC, ("Failed to rm mmap file %s: %d(%s)\n",
+                                      mc_ctx->file, ret, strerror(ret)));
+        }
+
     }
 
     /* temporarily relax umask as we need the file to be readable
@@ -855,6 +857,7 @@ static errno_t sss_mc_create_file(struct sss_mc_ctx *mc_ctx)
         DEBUG(SSSDBG_FATAL_FAILURE,
               ("Failed to lock file %s.\n", mc_ctx->file));
         close(mc_ctx->fd);
+        mc_ctx->fd = -1;
 
         /* Report on unlink failures but don't overwrite the errno
          * from sss_br_lock_file
@@ -897,6 +900,36 @@ static void sss_mc_header_update(struct sss_mc_ctx *mc_ctx, int status)
     MC_LOWER_BARRIER(h);
 }
 
+static int mc_ctx_destructor(struct sss_mc_ctx *mc_ctx)
+{
+    int ret;
+
+    /* Print debug message to logs if munmap() or close()
+     * fail but always return 0 */
+
+    if (mc_ctx->mmap_base != NULL) {
+        ret = munmap(mc_ctx->mmap_base, mc_ctx->mmap_size);
+        if (ret == -1) {
+            ret = errno;
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  ("Failed to unmap old memory cache file."
+                   "[%d]: %s\n", ret, strerror(ret)));
+        }
+    }
+
+    if (mc_ctx->fd != -1) {
+        ret = close(mc_ctx->fd);
+        if (ret == -1) {
+            ret = errno;
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  ("Failed to close old memory cache file."
+                   "[%d]: %s\n", ret, strerror(ret)));
+        }
+    }
+
+    return 0;
+}
+
 errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name,
                             enum sss_mc_type type, size_t n_elem,
                             time_t timeout, struct sss_mc_ctx **mcc)
@@ -904,7 +937,7 @@ errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name,
     struct sss_mc_ctx *mc_ctx = NULL;
     unsigned int rseed;
     int payload;
-    int ret;
+    int ret, dret;
 
     switch (type) {
     case SSS_MC_PASSWD:
@@ -922,6 +955,7 @@ errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name,
         return ENOMEM;
     }
     mc_ctx->fd = -1;
+    talloc_set_destructor(mc_ctx, mc_ctx_destructor);
 
     mc_ctx->name = talloc_strdup(mc_ctx, name);
     if (!mc_ctx->name) {
@@ -1003,18 +1037,16 @@ errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name,
 
 done:
     if (ret) {
-        if (mc_ctx && mc_ctx->mmap_base) {
-            munmap(mc_ctx->mmap_base, mc_ctx->mmap_size);
-        }
+        /* Closing the file descriptor and ummaping the file
+         * from memory is done in the mc_ctx_destructor. */
         if (mc_ctx && mc_ctx->fd != -1) {
-            close(mc_ctx->fd);
-            ret = unlink(mc_ctx->file);
-            if (ret == -1) {
-                DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to rm mmap file %s: %d(%s)\n",
-                                            mc_ctx->file, ret, strerror(ret)));
+            dret = unlink(mc_ctx->file);
+            if (dret == -1) {
+                DEBUG(SSSDBG_CRIT_FAILURE,
+                      ("Failed to rm mmap file %s: %d(%s)\n", mc_ctx->file,
+                       dret, strerror(dret)));
             }
         }
-
         talloc_free(mc_ctx);
     } else {
         *mcc = mc_ctx;
@@ -1059,13 +1091,7 @@ errno_t sss_mmap_cache_reinit(TALLOC_CTX *mem_ctx, size_t n_elem,
         timeout = (*mc_ctx)->valid_time_slot;
     }
 
-    ret = talloc_free(*mc_ctx);
-    if (ret != 0) {
-        /* This can happen only if destructor is associated with this
-         * context */
-        DEBUG(SSSDBG_MINOR_FAILURE, ("Destructor asociated with memory"
-                                    " context failed.\n"));
-    }
+    talloc_free(*mc_ctx);
 
     /* make sure we do not leave a potentially freed pointer around */
     *mc_ctx = NULL;
-- 
1.7.11.2

