The branch, master has been updated via bbd0bda smbd: Fix an uninitialized memory read via 7e12bfc pthreadpool: Add test for fork crash via ccc187f pthreadpool: Fix pthreadpools with fork via 925625b dbwrap_ctdb: avoid smbd/ctdb deadlocks: check whether we can work locally in db_ctdb_parse_record() from 7e53506 torture: Fix a torture crash with -O3
http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master - Log ----------------------------------------------------------------- commit bbd0bda9c2213215efeb13f5fe527d2409dc79b2 Author: Volker Lendecke <v...@samba.org> Date: Mon Mar 3 13:49:46 2014 +0100 smbd: Fix an uninitialized memory read This might be the reason for a few flaky builds. Signed-off-by: Volker Lendecke <v...@samba.org> Reviewed-by: Michael Adam <ob...@samba.org> Autobuild-User(master): Michael Adam <ob...@samba.org> Autobuild-Date(master): Mon Mar 3 16:30:53 CET 2014 on sn-devel-104 commit 7e12bfc6a8fe97da907525ae90288e37c4fa7a7f Author: Volker Lendecke <v...@samba.org> Date: Mon Mar 3 11:57:18 2014 +0000 pthreadpool: Add test for fork crash Signed-off-by: Volker Lendecke <v...@samba.org> Reviewed-by: Kamen Mazdrashki <kame...@samba.org> Reviewed-by: Simo Sorce <s...@samba.org> Reviewed-by: Michael Adam <ob...@samba.org> commit ccc187ff5e6cbc411476c5f5e68cc6bb1fe35818 Author: Volker Lendecke <v...@samba.org> Date: Mon Mar 3 12:20:41 2014 +0100 pthreadpool: Fix pthreadpools with fork The current could would crash if a pthreadpool was created, deleted and the process then fork()s. "pthreadpools" is NULL in this case, but the pthread_atfork handlers are in place. This fixes walking the pthreadpools list in reverse. Signed-off-by: Volker Lendecke <v...@samba.org> Reviewed-by: Kamen Mazdrashki <kame...@samba.org> Reviewed-by: Simo Sorce <s...@samba.org> Reviewed-by: Michael Adam <ob...@samba.org> commit 925625b52886d40b50fc631bad8bdc81970f7598 Author: Michael Adam <ob...@samba.org> Date: Fri Feb 28 15:49:30 2014 +0100 dbwrap_ctdb: avoid smbd/ctdb deadlocks: check whether we can work locally in db_ctdb_parse_record() If the same process tries to re-lock the same record it has already locked, don't go to the ctdbd again. There are situations where we already have a lock on a record and then do a dbwrap_parse_record() on that record, for instance in locking code: do_lock() -> grabs lock on brl record with brl_get_locks() -> calls brl_lock() -> calls brl_lock_posix or _windows_default() -> calls contend_level2_oplocks_begin() -> calls brl_locks_get_read_only() -> calls dbwrap_parse_record on the same brl record as above In the local (tdb) case, this is not a problem, because identical fcntl locks in the same process don't contend each other, but calling out to ctdb for this lets smbd and ctdb deadlock. db_ctdb_fetch_lock() already correclty checks first whether we can simply try to lock locally. But db_ctdb_parse_record() failed to do so for empty records, i.e. records that only consist of the ctdb record header. (These can be deleted records but can also be freshly created and still empty records.) This patch lets db_ctdb_parse_record() not skip local access for empty records, hence fixing the deadlock. PLAN: In the long run, it would be better to solve this generically on the dbwrap_layer, i.e. root the notion of an already locked record there, and skip any call to the db (tdb or ctdb backend) if we have it. This would also solve the problem for all calls like fetch_locked, parse_record and possibly others. But this is the urgent fix for now. Pair-Programmed-With: Volker Lendecke <v...@samba.org> Signed-off-by: Michael Adam <ob...@samba.org> Signed-off-by: Volker Lendecke <v...@samba.org> Tested-by: Björn Baumbach <b...@sernet.de> ----------------------------------------------------------------------- Summary of changes: source3/lib/dbwrap/dbwrap_ctdb.c | 11 +------- source3/lib/pthreadpool/pthreadpool.c | 21 ++++---------- source3/lib/pthreadpool/tests.c | 48 +++++++++++++++++++++++++++++++++ source3/locking/brlock.c | 2 + 4 files changed, 57 insertions(+), 25 deletions(-) Changeset truncated at 500 lines: diff --git a/source3/lib/dbwrap/dbwrap_ctdb.c b/source3/lib/dbwrap/dbwrap_ctdb.c index 87e644d..09a51ce 100644 --- a/source3/lib/dbwrap/dbwrap_ctdb.c +++ b/source3/lib/dbwrap/dbwrap_ctdb.c @@ -109,16 +109,7 @@ static int db_ctdb_ltdb_parser(TDB_DATA key, TDB_DATA data, if (data.dsize < sizeof(struct ctdb_ltdb_header)) { return -1; } - if (data.dsize == sizeof(struct ctdb_ltdb_header)) { - /* - * Making this a separate case that needs fixing - * separately. This is an empty record. ctdbd does not - * distinguish between empty and deleted records. Samba right - * now can live without empty records, so lets treat zero-size - * (i.e. deleted) records as non-existing. - */ - return -1; - } + state->parser( key, (struct ctdb_ltdb_header *)data.dptr, make_tdb_data(data.dptr + sizeof(struct ctdb_ltdb_header), diff --git a/source3/lib/pthreadpool/pthreadpool.c b/source3/lib/pthreadpool/pthreadpool.c index bd58d62..654d420 100644 --- a/source3/lib/pthreadpool/pthreadpool.c +++ b/source3/lib/pthreadpool/pthreadpool.c @@ -188,16 +188,11 @@ static void pthreadpool_parent(void) int ret; struct pthreadpool *pool; - pool = DLIST_TAIL(pthreadpools); - - while (1) { + for (pool = DLIST_TAIL(pthreadpools); + pool != NULL; + pool = DLIST_PREV(pool)) { ret = pthread_mutex_unlock(&pool->mutex); assert(ret == 0); - - if (pool == pthreadpools) { - break; - } - pool = pool->prev; } ret = pthread_mutex_unlock(&pthreadpools_mutex); @@ -209,9 +204,10 @@ static void pthreadpool_child(void) int ret; struct pthreadpool *pool; - pool = DLIST_TAIL(pthreadpools); + for (pool = DLIST_TAIL(pthreadpools); + pool != NULL; + pool = DLIST_PREV(pool)) { - while (1) { close(pool->sig_pipe[0]); close(pool->sig_pipe[1]); @@ -236,11 +232,6 @@ static void pthreadpool_child(void) ret = pthread_mutex_unlock(&pool->mutex); assert(ret == 0); - - if (pool == pthreadpools) { - break; - } - pool = pool->prev; } ret = pthread_mutex_unlock(&pthreadpools_mutex); diff --git a/source3/lib/pthreadpool/tests.c b/source3/lib/pthreadpool/tests.c index 95d37b6..170cedf 100644 --- a/source3/lib/pthreadpool/tests.c +++ b/source3/lib/pthreadpool/tests.c @@ -5,6 +5,8 @@ #include <stdlib.h> #include <pthread.h> #include <unistd.h> +#include <sys/types.h> +#include <sys/wait.h> #include "pthreadpool.h" static int test_init(void) @@ -318,6 +320,46 @@ static int test_threaded_addjob(int num_pools, int num_threads, int poolsize, return 0; } +static int test_fork(void) +{ + struct pthreadpool *p; + pid_t child, waited; + int status, ret; + + ret = pthreadpool_init(1, &p); + if (ret != 0) { + fprintf(stderr, "pthreadpool_init failed: %s\n", + strerror(ret)); + return -1; + } + ret = pthreadpool_destroy(p); + if (ret != 0) { + fprintf(stderr, "pthreadpool_destroy failed: %s\n", + strerror(ret)); + return -1; + } + + child = fork(); + if (child < 0) { + perror("fork failed"); + return -1; + } + if (child == 0) { + exit(0); + } + waited = wait(&status); + if (waited == -1) { + perror("wait failed"); + return -1; + } + if (waited != child) { + fprintf(stderr, "expected child %d, got %d\n", + (int)child, (int)waited); + return -1; + } + return 0; +} + int main(void) { int ret; @@ -328,6 +370,12 @@ int main(void) return 1; } + ret = test_fork(); + if (ret != 0) { + fprintf(stderr, "test_fork failed\n"); + return 1; + } + ret = test_jobs(10, 10000); if (ret != 0) { fprintf(stderr, "test_jobs failed\n"); diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c index e6c8949..ac22ba4 100644 --- a/source3/locking/brlock.c +++ b/source3/locking/brlock.c @@ -2085,6 +2085,8 @@ static void brl_get_locks_readonly_parser(TDB_DATA key, TDB_DATA data, if ((data.dsize % sizeof(struct lock_struct)) == 1) { br_lock->have_read_oplocks = (data.dptr[data.dsize-1] == 1); + } else { + br_lock->have_read_oplocks = false; } DEBUG(10, ("Got %d bytes, have_read_oplocks: %s\n", (int)data.dsize, -- Samba Shared Repository