[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