[PATCH] A few Coverity fixes
Jeremy Allison
jra at samba.org
Fri Oct 14 18:33:09 UTC 2016
On Fri, Oct 14, 2016 at 01:07:46PM +0200, Volker Lendecke wrote:
> Hi!
>
> Review appreciated!
>
> Thanks, Volker
Thanks for patch #1. I remember when first looking at this code
thinking we had a deadlock here too until I spotted the
pthread_mutex_unlock(&pool->mutex); in pthreadpool_free(),
so I know how the analyzer feels :-) :-).
Currently reviewing these...
Cheers,
Jeremy.
> From 6de5a078c761d817ec1bb88d0ebd3b635026328c Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 12 Oct 2016 12:12:28 +0200
> Subject: [PATCH 1/6] pthreadpool: Rearrange locks a bit
>
> Coverity ID 1373624 says we have a deadlock between pthreadpool_prepare and
> pthreadpool_destroy. Coverity somehow misses that pthreadpool_free unlocks
> pool->mutex, so I think this is a false positive. Nevertheless this re-arranges
> the code a bit for more clarity, hoping that Coverity now can better track the
> locks and unlocks. Also, the human reader might have to jump between routines a
> bit less.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/pthreadpool/pthreadpool.c | 30 ++++++++++++++++--------------
> 1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/source3/lib/pthreadpool/pthreadpool.c b/source3/lib/pthreadpool/pthreadpool.c
> index a1eb924..f34e219 100644
> --- a/source3/lib/pthreadpool/pthreadpool.c
> +++ b/source3/lib/pthreadpool/pthreadpool.c
> @@ -234,9 +234,6 @@ static int pthreadpool_free(struct pthreadpool *pool)
> {
> int ret, ret1;
>
> - ret = pthread_mutex_unlock(&pool->mutex);
> - assert(ret == 0);
> -
> ret = pthread_mutex_destroy(&pool->mutex);
> ret1 = pthread_cond_destroy(&pool->condvar);
>
> @@ -275,23 +272,26 @@ int pthreadpool_destroy(struct pthreadpool *pool)
> return ret;
> }
>
> - if (pool->num_threads == 0) {
> - ret = pthreadpool_free(pool);
> - return ret;
> - }
> -
> if (pool->shutdown) {
> ret = pthread_mutex_unlock(&pool->mutex);
> assert(ret == 0);
> return EBUSY;
> }
>
> + pool->shutdown = true;
> +
> + if (pool->num_threads == 0) {
> + ret = pthread_mutex_unlock(&pool->mutex);
> + assert(ret == 0);
> +
> + ret = pthreadpool_free(pool);
> + return ret;
> + }
> +
> /*
> * We have active threads, tell them to finish.
> */
>
> - pool->shutdown = true;
> -
> ret = pthread_cond_broadcast(&pool->condvar);
>
> ret1 = pthread_mutex_unlock(&pool->mutex);
> @@ -308,16 +308,18 @@ int pthreadpool_destroy(struct pthreadpool *pool)
> static void pthreadpool_server_exit(struct pthreadpool *pool)
> {
> int ret;
> + bool free_it;
>
> pool->num_threads -= 1;
>
> - if (pool->shutdown && (pool->num_threads == 0)) {
> - pthreadpool_free(pool);
> - return;
> - }
> + free_it = (pool->shutdown && (pool->num_threads == 0));
>
> ret = pthread_mutex_unlock(&pool->mutex);
> assert(ret == 0);
> +
> + if (free_it) {
> + pthreadpool_free(pool);
> + }
> }
>
> static bool pthreadpool_get_job(struct pthreadpool *p,
> --
> 2.1.4
>
>
> From bfdef700493a3ccfc3f598744973b1a672801ee1 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 12 Oct 2016 12:18:11 +0200
> Subject: [PATCH 2/6] talloc: Fix CID 1373621 Unchecked return value
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> lib/talloc/testsuite.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/lib/talloc/testsuite.c b/lib/talloc/testsuite.c
> index 8fe5d90..cff443c 100644
> --- a/lib/talloc/testsuite.c
> +++ b/lib/talloc/testsuite.c
> @@ -34,6 +34,12 @@
> #include <unistd.h>
> #include <sys/wait.h>
>
> +#ifdef NDEBUG
> +#undef NDEBUG
> +#endif
> +
> +#include <assert.h>
> +
> #include "talloc_testsuite.h"
>
> static struct timeval private_timeval_current(void)
> @@ -1750,7 +1756,8 @@ static void *thread_fn(void *arg)
> ret = pthread_cond_wait(&condvar, &mtx);
> if (ret != 0) {
> talloc_free(top_ctx);
> - pthread_mutex_unlock(&mtx);
> + ret = pthread_mutex_unlock(&mtx);
> + assert(ret == 0);
> return NULL;
> }
> }
> @@ -1760,7 +1767,8 @@ static void *thread_fn(void *arg)
>
> /* Tell the main thread it's ready for pickup. */
> pthread_cond_broadcast(&condvar);
> - pthread_mutex_unlock(&mtx);
> + ret = pthread_mutex_unlock(&mtx);
> + assert(ret == 0);
>
> talloc_free(top_ctx);
> return NULL;
> --
> 2.1.4
>
>
> From eac8fc6802d624f9a07949896e662ad5eabe1d63 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 12 Oct 2016 12:20:00 +0200
> Subject: [PATCH 3/6] pthreadpool: Fix CID 1373620 Unchecked return value from
> library
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/pthreadpool/tests.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/source3/lib/pthreadpool/tests.c b/source3/lib/pthreadpool/tests.c
> index 4d211f2..c4d2e6a 100644
> --- a/source3/lib/pthreadpool/tests.c
> +++ b/source3/lib/pthreadpool/tests.c
> @@ -132,7 +132,9 @@ static int test_busydestroy(void)
> pfd.fd = pthreadpool_pipe_signal_fd(p);
> pfd.events = POLLIN|POLLERR;
>
> - poll(&pfd, 1, -1);
> + do {
> + ret = poll(&pfd, 1, -1);
> + } while ((ret == -1) && (errno == EINTR));
>
> ret = pthreadpool_pipe_finished_jobs(p, &jobid, 1);
> if (ret < 0) {
> --
> 2.1.4
>
>
> From 47c5e37d50c3b3b41a41cf79827a790de1b19b0d Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 12 Oct 2016 12:21:36 +0200
> Subject: [PATCH 4/6] messaging: Fix CID 1373625 Unused value
>
> Hmm. I wonder how that cut&paste happened...
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source4/lib/messaging/tests/messaging.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/source4/lib/messaging/tests/messaging.c b/source4/lib/messaging/tests/messaging.c
> index 783850a..ba58978 100644
> --- a/source4/lib/messaging/tests/messaging.c
> +++ b/source4/lib/messaging/tests/messaging.c
> @@ -171,6 +171,7 @@ static bool test_messaging_overflow(struct torture_context *tctx)
> } while ((nwritten == -1) && (errno == EINTR));
>
> ret = close(down_pipe[1]);
> + torture_assert(tctx, ret == 0, "close failed");
>
> do {
> nread = read(down_pipe[0], &c, 1);
> --
> 2.1.4
>
>
> From 827ef505d3713f3c7a673afed2577791692ae9d2 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 12 Oct 2016 12:25:39 +0200
> Subject: [PATCH 5/6] messaging: Fix CID 1373622 Extra high-order bits
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/messages_dgm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/lib/messages_dgm.c b/source3/lib/messages_dgm.c
> index 6bdd589..49b3903 100644
> --- a/source3/lib/messages_dgm.c
> +++ b/source3/lib/messages_dgm.c
> @@ -1133,7 +1133,7 @@ static void messaging_dgm_read_handler(struct tevent_context *ev,
> msghdr_prep_recv_fds(&msg, msgbuf, msgbufsize, INT8_MAX);
>
> #ifdef MSG_CMSG_CLOEXEC
> - flags |= MSG_CMSG_CLOEXEC;
> + msg.msg_flags |= MSG_CMSG_CLOEXEC;
> #endif
>
> received = recvmsg(ctx->sock, &msg, 0);
> --
> 2.1.4
>
>
> From 9116c9de3b08529950de65ca22518afa35ca728f Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 12 Oct 2016 12:27:56 +0200
> Subject: [PATCH 6/6] talloc: Fix CID 1373619 Unchecked return value
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> lib/talloc/testsuite.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/lib/talloc/testsuite.c b/lib/talloc/testsuite.c
> index cff443c..7f98f4b 100644
> --- a/lib/talloc/testsuite.c
> +++ b/lib/talloc/testsuite.c
> @@ -1839,8 +1839,8 @@ static bool test_pthread_talloc_passing(void)
> printf("pthread_cond_wait %d failed (%d)\n", i,
> ret);
> talloc_free(mem_ctx);
> - pthread_mutex_unlock(&mtx);
> - return false;
> + ret = pthread_mutex_unlock(&mtx);
> + assert(ret == 0);
> }
> }
>
> @@ -1849,7 +1849,8 @@ static bool test_pthread_talloc_passing(void)
>
> /* Tell the sub-threads we're ready for another. */
> pthread_cond_broadcast(&condvar);
> - pthread_mutex_unlock(&mtx);
> + ret = pthread_mutex_unlock(&mtx);
> + assert(ret == 0);
> }
>
> CHECK_SIZE("pthread_talloc_passing", mem_ctx, NUM_THREADS * 100);
> --
> 2.1.4
>
More information about the samba-technical
mailing list