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@lists.fedorahosted.org > >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > >
Please see the attached patch, does it look better to you?
>From d5e877d3f30521627e9a25fe12d0bf100c2afcb6 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@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
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel