From b22ca95d87dd05991db9bb8622a82afcbfdc4be9 Mon Sep 17 00:00:00 2001 From: Christine Caulfield Date: Fri, 5 Jun 2020 07:49:25 +0100 Subject: [PATCH 1/2] array: More locking fixes helgrind threw out a couple more locking errors in the logging/array code. --- lib/array.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/array.c b/lib/array.c index 67ee1c7cd..5b4b65802 100644 --- a/lib/array.c +++ b/lib/array.c @@ -135,11 +135,10 @@ qb_array_index(struct qb_array * a, int32_t idx, void **element_out) b = BIN_NUM_GET((uint32_t) idx); assert(b < MAX_BINS); + (void)qb_thread_lock(a->grow_lock); if (b >= a->num_bins || a->bin[b] == NULL) { int32_t bin_alloced = QB_FALSE; - (void)qb_thread_lock(a->grow_lock); - if (b >= a->num_bins) { rc = _grow_bin_array(a, b + 1); if (rc < 0) { @@ -154,11 +153,13 @@ qb_array_index(struct qb_array * a, int32_t idx, void **element_out) } bin_alloced = QB_TRUE; } - (void)qb_thread_unlock(a->grow_lock); if (bin_alloced && a->new_bin_cb) { a->new_bin_cb(a, b); } + } else { + // new_bin_cb() needs to be called unlocked so can't extend the lock after the if block */ + (void)qb_thread_unlock(a->grow_lock); } elem = ELEM_NUM_GET(idx); @@ -188,10 +189,15 @@ qb_array_new_bin_cb_set(struct qb_array * a, qb_array_new_bin_cb_fn fn) size_t qb_array_num_bins_get(struct qb_array * a) { + size_t bins; + if (a == NULL) { return -EINVAL; } - return a->num_bins; + (void)qb_thread_lock(a->grow_lock); + bins = a->num_bins; + (void)qb_thread_unlock(a->grow_lock); + return bins; } size_t @@ -217,13 +223,13 @@ qb_array_grow(struct qb_array * a, size_t max_elements) } a->max_elements = max_elements; b = QB_MIN((max_elements / MAX_ELEMENTS_PER_BIN) + 1, MAX_BINS); + (void)qb_thread_lock(a->grow_lock); if (b > a->num_bins) { - (void)qb_thread_lock(a->grow_lock); if (b >= a->num_bins) { rc = _grow_bin_array(a, b + 1); } - (void)qb_thread_unlock(a->grow_lock); } + (void)qb_thread_unlock(a->grow_lock); return rc; } @@ -231,6 +237,12 @@ void qb_array_free(struct qb_array *a) { size_t i; + + /* In theory we should lock before accessing a->num_bins + * but as we're freeing the whole array here it seems + * a bit pointless as any other threads would segv when we + * unlock anyway + */ for (i = 0; i < a->num_bins; i++) { free(a->bin[i]); } From 6ae509b62998036c4d524c011f8c704a83295c0f Mon Sep 17 00:00:00 2001 From: Christine Caulfield Date: Mon, 8 Jun 2020 12:48:10 +0100 Subject: [PATCH 2/2] We also need to protect a->max_elements --- lib/array.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/array.c b/lib/array.c index 5b4b65802..8b467ceda 100644 --- a/lib/array.c +++ b/lib/array.c @@ -122,20 +122,24 @@ qb_array_index(struct qb_array * a, int32_t idx, void **element_out) if (idx < 0) { return -ERANGE; } + (void)qb_thread_lock(a->grow_lock); if ((uint32_t) idx >= a->max_elements) { if (a->autogrow_elements == 0) { + (void)qb_thread_unlock(a->grow_lock); return -ERANGE; } else { + /* qb_array_grow gets the lock */ + (void)qb_thread_unlock(a->grow_lock); rc = qb_array_grow(a, idx + 1); if (rc != 0) { return rc; } + (void)qb_thread_lock(a->grow_lock); } } b = BIN_NUM_GET((uint32_t) idx); assert(b < MAX_BINS); - (void)qb_thread_lock(a->grow_lock); if (b >= a->num_bins || a->bin[b] == NULL) { int32_t bin_alloced = QB_FALSE; @@ -153,12 +157,12 @@ qb_array_index(struct qb_array * a, int32_t idx, void **element_out) } bin_alloced = QB_TRUE; } + /* new_bin_cb() needs to be called unlocked so can't extend the lock after the if block */ (void)qb_thread_unlock(a->grow_lock); if (bin_alloced && a->new_bin_cb) { a->new_bin_cb(a, b); } } else { - // new_bin_cb() needs to be called unlocked so can't extend the lock after the if block */ (void)qb_thread_unlock(a->grow_lock); } @@ -218,12 +222,14 @@ qb_array_grow(struct qb_array * a, size_t max_elements) if (a == NULL || max_elements > QB_ARRAY_MAX_ELEMENTS) { return -EINVAL; } + + (void)qb_thread_lock(a->grow_lock); if (max_elements <= a->max_elements) { + (void)qb_thread_unlock(a->grow_lock); return 0; } a->max_elements = max_elements; b = QB_MIN((max_elements / MAX_ELEMENTS_PER_BIN) + 1, MAX_BINS); - (void)qb_thread_lock(a->grow_lock); if (b > a->num_bins) { if (b >= a->num_bins) { rc = _grow_bin_array(a, b + 1);