On Mon, Dec 10, 2012 at 12:19:06PM -0500, Simo Sorce wrote: > On Mon, 2012-12-10 at 18:10 +0100, Jakub Hrozek wrote: > > On Mon, Dec 10, 2012 at 09:06:16AM -0500, Simo Sorce wrote: > > > On Mon, 2012-12-10 at 08:39 +0100, Jakub Hrozek wrote: > > > > Coverity bug again. > > > > > > > > https://fedorahosted.org/sssd/ticket/1704 > > > > > > Any reason why you do not close the file anymore in a lock error ? > > > > > > I know the caller will also cleanup but I would expect that if the > > > create function fails no file is open and no file is on disk ... > > > > > > > Hmm, did I attach the wrong patch? In the version I have in tree, there > > is close(mc_ctx->fd); called in case sss_br_lock_file() fails. It's not > > very readable from the diff itself, but in the source, the close would be > > on line 721 in src/responder/nss/nsssrv_mmap_cache.c > > > > Anyway, I'm attaching the patch again. > > Ah sorry, it is there, but I was also wondering if you shouldn't call > unlink ? > After all if you fail here you just have created an empty file, and I am > not sure we want to leave it around. > I know the code did not unlink but given you are fixing this already ... > > Simo.
Yes, that makes sense. A new patch is attached.
>From 4cec1016943222bb18da741a430dc016ddc6b0b1 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <[email protected]> Date: Sun, 9 Dec 2012 15:16:44 +0100 Subject: [PATCH] NSS: Fix the error handler in sss_mc_create_file https://fedorahosted.org/sssd/ticket/1704 The function is short enough so that we can simply stick with return and release resources before returning as appropriate. --- src/responder/nss/nsssrv_mmap_cache.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index c6c5a5558400938f7afa701a642070821542b160..542c837c3ce7c876603af6d90f3b790ed01ad46f 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c @@ -671,7 +671,7 @@ static errno_t sss_mc_create_file(struct sss_mc_ctx *mc_ctx) { mode_t old_mask; int ofd; - int ret; + int ret, uret; useconds_t t = 50000; int retries = 3; @@ -704,28 +704,34 @@ static errno_t sss_mc_create_file(struct sss_mc_ctx *mc_ctx) * by everyone for now */ old_mask = umask(0022); - ret = 0; + errno = 0; mc_ctx->fd = open(mc_ctx->file, O_CREAT | O_EXCL | O_RDWR, 0644); + umask(old_mask); if (mc_ctx->fd == -1) { ret = errno; DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to open mmap file %s: %d(%s)\n", mc_ctx->file, ret, strerror(ret))); - goto done; + return ret; } ret = sss_br_lock_file(mc_ctx->fd, 0, 1, retries, t); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, ("Failed to lock file %s.\n", mc_ctx->file)); - goto done; - } - -done: - /* reset mask back */ - umask(old_mask); - - if (ret) { close(mc_ctx->fd); + + /* Report on unlink failures but don't overwrite the errno + * from sss_br_lock_file + */ + errno = 0; + uret = unlink(mc_ctx->file); + if (uret == -1) { + uret = errno; + DEBUG(SSSDBG_TRACE_FUNC, ("Failed to rm mmap file %s: %d(%s)\n", + mc_ctx->file, uret, strerror(uret))); + } + + return ret; } return ret; -- 1.8.0.1
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
