[SSSD] [PATCH] GPO: Use sss_unique_file and close fd on failure

Jakub Hrozek jhrozek at redhat.com
Tue Sep 1 09:19:55 UTC 2015


On Wed, Aug 26, 2015 at 02:21:23PM +0200, Pavel Březina wrote:
> On 08/18/2015 03:35 PM, Jakub Hrozek wrote:
> >Hi,
> >
> >one more usage of sss_unique_file I forgot to send earlier.
> 
> Nack.
> 
> >      ret = close(fd);
> >+    fd = -1;
> >      if (ret == -1) {
> >          ret = errno;
> >          DEBUG(SSSDBG_CRIT_FAILURE,
> >@@ -369,12 +366,16 @@ static errno_t gpo_cache_store_file(const char *smb_path,
> >          goto done;
> >      }
> 
> Please, move this close(fd) call to done part, so we close fd only there.
> 
> >
> >+    ret = EOK;
> >   done:
> >-
> >      if (ret != EOK) {
> >        DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
> >      }
> 
> And when you are here, can you fix indentation in the debug statement above?
> 
> >
> >+    if (fd != -1) {
> >+        close(fd);
> >+    }
> >+
> >      talloc_free(tmp_ctx);
> >      return ret;
> >  }
> >-- 2.4.3
> >
> >
> >
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel at lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >

Please see the attached patch, does it look better to you?
-------------- next part --------------
>From d5e877d3f30521627e9a25fe12d0bf100c2afcb6 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 12 Aug 2015 12:45:34 +0200
Subject: [PATCH] GPO: Use sss_unique_file and close fd on failure

The GPO child didn't remove temporary file on failure and didn't close
the fd on failure (the latter was not much of a problem for a
short-lived child process).
---
 src/providers/ad/ad_gpo_child.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c
index 6547f9c05ea53134548ebd7f998fd66b25832854..4a8869d3dfc3f7983543ac26d8e0fb973e1ceb27 100644
--- a/src/providers/ad/ad_gpo_child.c
+++ b/src/providers/ad/ad_gpo_child.c
@@ -273,10 +273,10 @@ static errno_t gpo_cache_store_file(const char *smb_path,
                                     int buflen)
 {
     int ret;
+    int fret;
     int fd = -1;
     char *tmp_name = NULL;
     ssize_t written;
-    mode_t old_umask;
     char *filename = NULL;
     char *smb_path_with_suffix = NULL;
     TALLOC_CTX *tmp_ctx = NULL;
@@ -318,13 +318,10 @@ static errno_t gpo_cache_store_file(const char *smb_path,
         goto done;
     }
 
-    old_umask = umask(077);
-    fd = mkstemp(tmp_name);
-    umask(old_umask);
+    fd = sss_unique_file(tmp_ctx, tmp_name, &ret);
     if (fd == -1) {
-        ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
-              "mkstemp failed [%d][%s].\n", ret, strerror(ret));
+              "sss_unique_file failed [%d][%s].\n", ret, strerror(ret));
         goto done;
     }
 
@@ -353,14 +350,6 @@ static errno_t gpo_cache_store_file(const char *smb_path,
         goto done;
     }
 
-    ret = close(fd);
-    if (ret == -1) {
-        ret = errno;
-        DEBUG(SSSDBG_CRIT_FAILURE,
-              "close failed [%d][%s].\n", ret, strerror(ret));
-        goto done;
-    }
-
     ret = rename(tmp_name, filename);
     if (ret == -1) {
         ret = errno;
@@ -369,10 +358,19 @@ static errno_t gpo_cache_store_file(const char *smb_path,
         goto done;
     }
 
+    ret = EOK;
  done:
-
     if (ret != EOK) {
-      DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
+        DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
+    }
+
+    if (fd != -1) {
+        fret = close(fd);
+        if (fret == -1) {
+            fret = errno;
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                "close failed [%d][%s].\n", fret, strerror(fret));
+        }
     }
 
     talloc_free(tmp_ctx);
-- 
2.4.3



More information about the sssd-devel mailing list