The branch, master has been updated via edf77a1 ctdb-common: Reimplement pidfile_context_create() using pidfile_path_create() via 09b0398 util: Reimplement pidfile_create() using pidfile_path_create() via 411b7c8 util: New functions pidfile_path_create(), pidfile_fd_close() via 59ebb29 ctdb-common: Rename pidfile_create() -> pidfile_context_create() via 4dbfc16 util: Add pidfile.* to samba-util-core via d665b74 util: Clean up includes via 8731f1c util: pidfile_pid() should not unlink PID file from a70ab5f winbindd: Simplify two debug msgs
https://git.samba.org/?p=samba.git;a=shortlog;h=master - Log ----------------------------------------------------------------- commit edf77a112f44a3eda43c3d6cdb2985faf1335d35 Author: Martin Schwenke <mar...@meltin.net> Date: Mon Jul 31 15:26:36 2017 +1000 ctdb-common: Reimplement pidfile_context_create() using pidfile_path_create() Signed-off-by: Martin Schwenke <mar...@meltin.net> Reviewed-by: Volker Lendecke <v...@samba.org> Autobuild-User(master): Martin Schwenke <mart...@samba.org> Autobuild-Date(master): Wed Aug 2 07:28:44 CEST 2017 on sn-devel-144 commit 09b03988650b68897c02a521d570737578c72d85 Author: Martin Schwenke <mar...@meltin.net> Date: Mon Jul 31 15:20:19 2017 +1000 util: Reimplement pidfile_create() using pidfile_path_create() Signed-off-by: Martin Schwenke <mar...@meltin.net> Reviewed-by: Volker Lendecke <v...@samba.org> commit 411b7c87536a8639954bf1eb6876209abfb4e848 Author: Martin Schwenke <mar...@meltin.net> Date: Mon Jul 31 15:11:33 2017 +1000 util: New functions pidfile_path_create(), pidfile_fd_close() Uses the core of CTDB's create_pidfile_context() for pidfile_path_create(). pidfile_fd_close() is a subset of CTDB's pidfile_context_destructor(). Signed-off-by: Martin Schwenke <mar...@meltin.net> Reviewed-by: Volker Lendecke <v...@samba.org> commit 59ebb29e6a597586338c610b22927e5bd8a7628c Author: Martin Schwenke <mar...@meltin.net> Date: Mon Jul 31 15:16:45 2017 +1000 ctdb-common: Rename pidfile_create() -> pidfile_context_create() Signed-off-by: Martin Schwenke <mar...@meltin.net> Reviewed-by: Volker Lendecke <v...@samba.org> commit 4dbfc16bdd6253f887fd34bef8db5464c363e0e8 Author: Martin Schwenke <mar...@meltin.net> Date: Mon Jul 31 14:48:47 2017 +1000 util: Add pidfile.* to samba-util-core Signed-off-by: Martin Schwenke <mar...@meltin.net> Reviewed-by: Volker Lendecke <v...@samba.org> commit d665b74dbb4986c58fd887d7425d054c0533c924 Author: Martin Schwenke <mar...@meltin.net> Date: Mon Jul 31 14:47:01 2017 +1000 util: Clean up includes Signed-off-by: Martin Schwenke <mar...@meltin.net> Reviewed-by: Volker Lendecke <v...@samba.org> commit 8731f1cfab5e8cd2550237b589b5767a93935381 Author: Martin Schwenke <mar...@meltin.net> Date: Mon Jul 31 11:37:21 2017 +1000 util: pidfile_pid() should not unlink PID file This causes a race. If 2 callers to pidfile_create() both a find a stale PID file using pidfile_pid(). The 1st may then return to pidfile_create() and create a new PID file, which can then be unlinked by the 2nd caller. Consequently, PID file creation can not depend on creating the file, so drop O_EXCL from the call to open(). Signed-off-by: Martin Schwenke <mar...@meltin.net> Reviewed-by: Volker Lendecke <v...@samba.org> ----------------------------------------------------------------------- Summary of changes: ctdb/common/pidfile.c | 80 +++--------------------- ctdb/common/pidfile.h | 4 +- ctdb/common/sock_daemon.c | 2 +- ctdb/server/ctdb_daemon.c | 4 +- ctdb/tests/src/pidfile_test.c | 14 ++--- lib/util/pidfile.c | 138 +++++++++++++++++++++++++++++++++--------- lib/util/pidfile.h | 3 + lib/util/wscript_build | 4 +- 8 files changed, 134 insertions(+), 115 deletions(-) Changeset truncated at 500 lines: diff --git a/ctdb/common/pidfile.c b/ctdb/common/pidfile.c index 51c0c25..e78542d 100644 --- a/ctdb/common/pidfile.c +++ b/ctdb/common/pidfile.c @@ -23,6 +23,7 @@ #include <talloc.h> #include "lib/util/blocking.h" +#include "lib/util/pidfile.h" #include "common/pidfile.h" @@ -34,15 +35,11 @@ struct pidfile_context { static int pidfile_context_destructor(struct pidfile_context *pid_ctx); -int pidfile_create(TALLOC_CTX *mem_ctx, const char *pidfile, - struct pidfile_context **result) +int pidfile_context_create(TALLOC_CTX *mem_ctx, const char *pidfile, + struct pidfile_context **result) { struct pidfile_context *pid_ctx; - struct flock lck; - char tmp[64]; int fd, ret = 0; - int len; - ssize_t nwritten; pid_ctx = talloc_zero(mem_ctx, struct pidfile_context); if (pid_ctx == NULL) { @@ -57,67 +54,18 @@ int pidfile_create(TALLOC_CTX *mem_ctx, const char *pidfile, pid_ctx->pid = getpid(); - fd = open(pidfile, O_CREAT|O_WRONLY|O_NONBLOCK, 0644); - if (fd == -1) { - ret = errno; - goto fail; - } - - if (! set_close_on_exec(fd)) { - close(fd); - ret = EIO; - goto fail; - } - - pid_ctx->fd = fd; - - lck = (struct flock) { - .l_type = F_WRLCK, - .l_whence = SEEK_SET, - }; - - do { - ret = fcntl(fd, F_SETLK, &lck); - } while ((ret == -1) && (errno == EINTR)); - + ret = pidfile_path_create(pid_ctx->pidfile, &fd); if (ret != 0) { - ret = errno; - goto fail; + return ret; } - do { - ret = ftruncate(fd, 0); - } while ((ret == -1) && (errno == EINTR)); - - if (ret == -1) { - ret = EIO; - goto fail_unlink; - } - - len = snprintf(tmp, sizeof(tmp), "%u\n", pid_ctx->pid); - if (len < 0) { - ret = EIO; - goto fail_unlink; - } - - do { - nwritten = write(fd, tmp, len); - } while ((nwritten == -1) && (errno == EINTR)); - - if ((nwritten == -1) || (nwritten != len)) { - ret = EIO; - goto fail_unlink; - } + pid_ctx->fd = fd; talloc_set_destructor(pid_ctx, pidfile_context_destructor); *result = pid_ctx; return 0; -fail_unlink: - unlink(pidfile); - close(fd); - fail: talloc_free(pid_ctx); return ret; @@ -125,27 +73,13 @@ fail: static int pidfile_context_destructor(struct pidfile_context *pid_ctx) { - struct flock lck; - int ret; - if (getpid() != pid_ctx->pid) { return 0; } - lck = (struct flock) { - .l_type = F_UNLCK, - .l_whence = SEEK_SET, - }; - (void) unlink(pid_ctx->pidfile); - do { - ret = fcntl(pid_ctx->fd, F_SETLK, &lck); - } while ((ret == -1) && (errno == EINTR)); - - do { - ret = close(pid_ctx->fd); - } while ((ret == -1) && (errno == EINTR)); + pidfile_fd_close(pid_ctx->fd); return 0; } diff --git a/ctdb/common/pidfile.h b/ctdb/common/pidfile.h index 1450134..bc4e3a7 100644 --- a/ctdb/common/pidfile.h +++ b/ctdb/common/pidfile.h @@ -45,7 +45,7 @@ struct pidfile_context; * * Freeing the pidfile_context, will delete the pidfile. */ -int pidfile_create(TALLOC_CTX *mem_ctx, const char *pidfile, - struct pidfile_context **result); +int pidfile_context_create(TALLOC_CTX *mem_ctx, const char *pidfile, + struct pidfile_context **result); #endif /* __CTDB_PIDFILE_H__ */ diff --git a/ctdb/common/sock_daemon.c b/ctdb/common/sock_daemon.c index b53b4d8..a6d4301 100644 --- a/ctdb/common/sock_daemon.c +++ b/ctdb/common/sock_daemon.c @@ -477,7 +477,7 @@ int sock_daemon_setup(TALLOC_CTX *mem_ctx, const char *daemon_name, } if (pidfile != NULL) { - ret = pidfile_create(sockd, pidfile, &sockd->pid_ctx); + ret = pidfile_context_create(sockd, pidfile, &sockd->pid_ctx); if (ret != 0) { talloc_free(sockd); return EEXIST; diff --git a/ctdb/server/ctdb_daemon.c b/ctdb/server/ctdb_daemon.c index b5cee61..1030406 100644 --- a/ctdb/server/ctdb_daemon.c +++ b/ctdb/server/ctdb_daemon.c @@ -1158,8 +1158,8 @@ static void ctdb_remove_pidfile(void) static void ctdb_create_pidfile(TALLOC_CTX *mem_ctx) { if (ctdbd_pidfile != NULL) { - int ret = pidfile_create(mem_ctx, ctdbd_pidfile, - &ctdbd_pidfile_ctx); + int ret = pidfile_context_create(mem_ctx, ctdbd_pidfile, + &ctdbd_pidfile_ctx); if (ret != 0) { DEBUG(DEBUG_ERR, ("Failed to create PID file %s\n", diff --git a/ctdb/tests/src/pidfile_test.c b/ctdb/tests/src/pidfile_test.c index 00b64ca..592fc2b 100644 --- a/ctdb/tests/src/pidfile_test.c +++ b/ctdb/tests/src/pidfile_test.c @@ -34,7 +34,7 @@ static void test1(const char *pidfile) FILE *fp; pid_t pid; - ret = pidfile_create(NULL, pidfile, &pid_ctx); + ret = pidfile_context_create(NULL, pidfile, &pid_ctx); assert(ret == 0); assert(pid_ctx != NULL); @@ -77,7 +77,7 @@ static void test2(const char *pidfile) close(fd[0]); - ret = pidfile_create(NULL, pidfile, &pid_ctx); + ret = pidfile_context_create(NULL, pidfile, &pid_ctx); assert(ret == 0); assert(pid_ctx != NULL); @@ -107,14 +107,14 @@ static void test2(const char *pidfile) assert(pid == pid2); fclose(fp); - ret = pidfile_create(NULL, pidfile, &pid_ctx); + ret = pidfile_context_create(NULL, pidfile, &pid_ctx); assert(ret != 0); nread = read(fd[0], &ret, sizeof(ret)); assert(nread == sizeof(ret)); assert(ret == 0); - ret = pidfile_create(NULL, pidfile, &pid_ctx); + ret = pidfile_context_create(NULL, pidfile, &pid_ctx); assert(ret == 0); assert(pid_ctx != NULL); @@ -134,7 +134,7 @@ static void test3(const char *pidfile) size_t nread; struct stat st; - ret = pidfile_create(NULL, pidfile, &pid_ctx); + ret = pidfile_context_create(NULL, pidfile, &pid_ctx); assert(ret == 0); assert(pid_ctx != NULL); @@ -192,7 +192,7 @@ static void test4(const char *pidfile) close(fd[0]); - ret = pidfile_create(NULL, pidfile, &pid_ctx); + ret = pidfile_context_create(NULL, pidfile, &pid_ctx); nwritten = write(fd[1], &ret, sizeof(ret)); assert(nwritten == sizeof(ret)); @@ -216,7 +216,7 @@ static void test4(const char *pidfile) pid2 = waitpid(pid, &ret, 0); assert(pid2 == pid); - ret = pidfile_create(NULL, pidfile, &pid_ctx); + ret = pidfile_context_create(NULL, pidfile, &pid_ctx); assert(ret == 0); assert(pid_ctx != NULL); diff --git a/lib/util/pidfile.c b/lib/util/pidfile.c index 5590780..fdf9afb 100644 --- a/lib/util/pidfile.c +++ b/lib/util/pidfile.c @@ -1,9 +1,8 @@ -/* this code is broken - there is a race condition with the unlink (tridge) */ - /* Unix SMB/CIFS implementation. pidfile handling Copyright (C) Andrew Tridgell 1998 + Copyright (C) Amitay Isaccs 2016 This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -19,8 +18,13 @@ along with this program. If not, see <http://www.gnu.org/licenses/>. */ -#include "includes.h" +#include "replace.h" #include "system/filesys.h" + +#include "lib/util/blocking.h" +#include "lib/util/debug.h" +#include "lib/util/samba_util.h" /* For process_exists_by_pid() */ + #include "lib/util/pidfile.h" /** @@ -28,6 +32,104 @@ * @brief Pid file handling */ +int pidfile_path_create(const char *path, int *outfd) +{ + struct flock lck; + char tmp[64] = { 0 }; + pid_t pid; + int fd, ret = 0; + int len; + ssize_t nwritten; + + pid = getpid(); + + fd = open(path, O_CREAT|O_WRONLY|O_NONBLOCK, 0644); + if (fd == -1) { + return errno; + } + + if (! set_close_on_exec(fd)) { + close(fd); + return EIO; + } + + lck = (struct flock) { + .l_type = F_WRLCK, + .l_whence = SEEK_SET, + }; + + do { + ret = fcntl(fd, F_SETLK, &lck); + } while ((ret == -1) && (errno == EINTR)); + + if (ret != 0) { + ret = errno; + close(fd); + return ret; + } + + /* + * PID file is locked by us so from here on we should unlink + * on failure + */ + + do { + ret = ftruncate(fd, 0); + } while ((ret == -1) && (errno == EINTR)); + + if (ret == -1) { + ret = EIO; + goto fail_unlink; + } + + len = snprintf(tmp, sizeof(tmp), "%u\n", pid); + if (len < 0) { + ret = errno; + goto fail_unlink; + } + if (len >= sizeof(tmp)) { + ret = ENOSPC; + goto fail_unlink; + } + + do { + nwritten = write(fd, tmp, len); + } while ((nwritten == -1) && (errno == EINTR)); + + if ((nwritten == -1) || (nwritten != len)) { + ret = EIO; + goto fail_unlink; + } + + if (outfd != NULL) { + *outfd = fd; + } + return 0; + +fail_unlink: + unlink(path); + close(fd); + return ret; +} + +void pidfile_fd_close(int fd) +{ + struct flock lck = { + .l_type = F_UNLCK, + .l_whence = SEEK_SET, + }; + int ret; + + do { + ret = fcntl(fd, F_SETLK, &lck); + } while ((ret == -1) && (errno == EINTR)); + + do { + ret = close(fd); + } while ((ret == -1) && (errno == EINTR)); +} + + /** * return the pid in a pidfile. return 0 if the process (or pidfile) * does not exist @@ -79,9 +181,6 @@ pid_t pidfile_pid(const char *piddir, const char *name) noproc: close(fd); - DEBUG(10, ("Deleting %s, since %d is not a Samba process.\n", pidFile, - (int)ret)); - unlink(pidFile); return 0; } @@ -92,9 +191,8 @@ void pidfile_create(const char *piddir, const char *name) { size_t len = strlen(piddir) + strlen(name) + 6; char pidFile[len]; - int fd; - char buf[20]; pid_t pid; + int ret; snprintf(pidFile, sizeof(pidFile), "%s/%s.pid", piddir, name); @@ -105,26 +203,10 @@ void pidfile_create(const char *piddir, const char *name) exit(1); } - fd = open(pidFile, O_NONBLOCK | O_CREAT | O_WRONLY | O_EXCL, 0644); - if (fd == -1) { - DEBUG(0,("ERROR: can't open %s: Error was %s\n", pidFile, - strerror(errno))); - exit(1); - } - - smb_set_close_on_exec(fd); - - if (fcntl_lock(fd,F_SETLK,0,1,F_WRLCK)==false) { - DEBUG(0,("ERROR: %s : fcntl lock of file %s failed. Error was %s\n", - name, pidFile, strerror(errno))); - exit(1); - } - - memset(buf, 0, sizeof(buf)); - slprintf(buf, sizeof(buf) - 1, "%u\n", (unsigned int) getpid()); - if (write(fd, buf, strlen(buf)) != (ssize_t)strlen(buf)) { - DEBUG(0,("ERROR: can't write to file %s: %s\n", - pidFile, strerror(errno))); + ret = pidfile_path_create(pidFile, NULL); + if (ret != 0) { + DBG_ERR("ERROR: Failed to create PID file %s (%s)\n", + pidFile, strerror(ret)); exit(1); } diff --git a/lib/util/pidfile.h b/lib/util/pidfile.h index d504f52..d51161a 100644 --- a/lib/util/pidfile.h +++ b/lib/util/pidfile.h @@ -20,6 +20,9 @@ #ifndef _SAMBA_PIDFILE_H_ #define _SAMBA_PIDFILE_H_ +int pidfile_path_create(const char *path, int *outfd); +void pidfile_fd_close(int fd); + pid_t pidfile_pid(const char *piddir, const char *name); void pidfile_create(const char *piddir, const char *program_name); void pidfile_unlink(const char *piddir, const char *program_name); diff --git a/lib/util/wscript_build b/lib/util/wscript_build index 1d2f2ba..989db36 100644 --- a/lib/util/wscript_build +++ b/lib/util/wscript_build @@ -71,7 +71,7 @@ bld.SAMBA_SUBSYSTEM('samba-util-core', source='''data_blob.c util_file.c time.c signal.c util.c idtree.c fault.c substitute.c util_process.c util_strlist.c - strv_util.c bitmap.c select.c''', + strv_util.c bitmap.c select.c pidfile.c''', deps='''time-basic samba-debug socket-blocking talloc tevent execinfo pthread strv''', local_include=False) @@ -120,7 +120,7 @@ else: util_strlist_v3.c util_paths.c idtree_random.c base64.c util_str.c util_str_common.c ms_fnmatch.c - server_id.c dprintf.c pidfile.c + server_id.c dprintf.c tevent_debug.c memcache.c unix_match.c tfork.c''', deps='samba-util-core DYNCONFIG close-low-fd tini tiniparser genrand', public_deps='talloc tevent execinfo pthread LIBCRYPTO charset util_setid systemd systemd-daemon', -- Samba Shared Repository