[Freeipa-devel] [PATCH][bind-dyndb-ldap] Fix potential dereference of NULL pointer in sync_ctx_init

[Lukas Slebodnik]

On (13/12/13 17:44), Petr Spacek wrote:
>On 12.11.2013 16:13, Petr Spacek wrote:
>>On 5.11.2013 12:29, Tomas Hozza wrote:
>>>----- Original Message -----
>>>>Hello,
>>>>
>>>>Improve performance of initial LDAP synchronization.
>>>>
>>>>Changes are not journaled and SOA serial is not incremented during initial
>>>>LDAP synchronization.
>>>>
>>>>This eliminates unnecessary synchronous writes to journal and also
>>>>unnecessary SOA serial writes to LDAP.
>>>>
>>>>See commit messages and comments in syncrepl.c for all the gory details.
>>>
>>>
>>>ACK.
>>>
>>>Patches look good. AXFR and IXFR works as expected. Also BIND starts up much
>>>faster with these patches. Good job... :)
>>>
>>>Regards,
>>>
>>>Tomas
>>
>>Hmm, further testing revealed that patch 203 changed behavior little bit:
>>Zones were loaded from LDAP correctly, but the SOA serial wasn't changed at
>>all. As a result, zone transfers return inconsistent results if the data in
>>LDAP are changed when BIND was not running.
>>
>>Patch 203-v2 imitates the old behavior from bind-dyndb-ldap 3.x: Zone serial
>>is bumped *once* for each zone, so any changed in LDAP will be transferred
>>correctly (with new serial).
>
>Patch 202 v2 was rebased and fixes reconnection to LDAP and solves
>deadlock caused by too eager locking.
>
>Patch 203 v3 was rebased and fixes reconnection to LDAP.
>
>These patches should go to master branch.
>
>-- 
>Petr^2 Spacek
>

When I was testing upcoming bind-dyndb-ldap 4.0 release,
There was an interesting warning from clang static analyser.
I thought it was a false passitive, but it isn't.

Patch is attached


>From b73e345393d55fe411875d52e6fe4c98e1de8c04 Mon Sep 17 00:00:00 2001
>From: Petr Spacek <pspacek at redhat.com>
>Date: Mon, 9 Dec 2013 11:11:10 +0100
>Subject: [PATCH] Detect end of initial LDAP synchronization phase.
>
>LDAP intermediate message with refreshDone = TRUE is translated to
>sync_barrier_wait() call. This call sends 'barrier event' to all tasks
>involved in syncrepl event processing. The call will return when all tasks
>have processed all events enqueued before the call.
>
>Effectively, all events produced by initial LDAP synchronization
>are processed first. Current state of synchronization is available via
>sync_state_get() call.
>
>See comments in syncrepl.c for all the gory details.
>
>Signed-off-by: Petr Spacek <pspacek at redhat.com>
>---
> src/Makefile.am   |   2 +
> src/ldap_helper.c |  67 +++++++++--
> src/ldap_helper.h |   2 +
> src/syncrepl.c    | 351 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> src/syncrepl.h    |  63 ++++++++++
> 5 files changed, 473 insertions(+), 12 deletions(-)
> create mode 100644 src/syncrepl.c
> create mode 100644 src/syncrepl.h
>

<snip>

>+/**
>+ * Initialize synchronization context.
>+ *
>+ * @param[in]	task	Task used for first synchronization events.
>+ * 			Typically the ldap_inst->task.
>+ * @param[out]	sctxp	The new synchronization context.
>+ *
>+ * @post state == sync_init
>+ * @post task_cnt == 1
>+ * @post tasks list contains the task
>+ */
>+isc_result_t
>+sync_ctx_init(isc_mem_t *mctx, isc_task_t *task, sync_ctx_t **sctxp) {
>+	isc_result_t result;
>+	sync_ctx_t *sctx = NULL;
>+	isc_boolean_t lock_ready = ISC_FALSE;
>+	isc_boolean_t cond_ready = ISC_FALSE;
>+	isc_boolean_t refcount_ready = ISC_FALSE;
>+
>+	REQUIRE(sctxp != NULL && *sctxp == NULL);
                             ^^^^^^^^^^^^^^
                  *sctxp is NULL


>+	REQUIRE(ISCAPI_TASK_VALID(task));
>+
>+	CHECKED_MEM_GET_PTR(mctx, sctx);
>+	ZERO_PTR(sctx);
>+	isc_mem_attach(mctx, &sctx->mctx);
>+
>+	CHECK(isc_mutex_init(&sctx->mutex));
>+	lock_ready = ISC_TRUE;
>+	CHECK(isc_condition_init(&sctx->cond));
>+	cond_ready = ISC_TRUE;
>+
>+	/* refcount includes ldap_inst->task implicitly */
>+	CHECK(isc_refcount_init(&sctx->task_cnt, 0));
>+	refcount_ready = ISC_TRUE;
>+
>+	ISC_LIST_INIT(sctx->tasks);
>+
>+	sctx->state = sync_init;
>+	CHECK(sync_task_add(sctx, task));
>+
>+	*sctxp = sctx;
    ^^^^^^
value to *sctxp is asigned only on this line.

>+	return ISC_R_SUCCESS;
>+
>+cleanup:
*sctxp will be NULL in cleanup section

>+	if (lock_ready == ISC_TRUE)
>+		isc_mutex_destroy(&(*sctxp)->mutex);
                          &(NULL)->mutex
                     It does not look like a good idea :-)
>+	if (cond_ready == ISC_TRUE)
>+		isc_condition_init(&(*sctxp)->cond);
>+	if (refcount_ready == ISC_TRUE)
>+		isc_refcount_destroy(&(*sctxp)->task_cnt);
>+	MEM_PUT_AND_DETACH(*sctxp);
>+	return result;
>+}
>+

LS
-------------- next part --------------
>From dcf99122eb33b63799be91d3c13a9967420880c3 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Fri, 21 Feb 2014 19:18:28 +0100
Subject: [PATCH] Fix potential dereference of NULL pointer in sync_ctx_init

Value is asigned to the output argument sync_ctx_t **sctxp
before returning ISC_R_SUCCESS. Thus we should use local variable sctx
in cleanup section.
---
 src/syncrepl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/syncrepl.c b/src/syncrepl.c
index 2a6d159fe16fffbf928198f9240cd8b1fcd41005..c6b326bab9733f7cb5065381d26e999ddbb570db 100644
--- a/src/syncrepl.c
+++ b/src/syncrepl.c
@@ -215,12 +215,12 @@ sync_ctx_init(isc_mem_t *mctx, isc_task_t *task, sync_ctx_t **sctxp) {
 
 cleanup:
 	if (lock_ready == ISC_TRUE)
-		isc_mutex_destroy(&(*sctxp)->mutex);
+		isc_mutex_destroy(&sctx->mutex);
 	if (cond_ready == ISC_TRUE)
-		isc_condition_init(&(*sctxp)->cond);
+		isc_condition_init(&sctx->cond);
 	if (refcount_ready == ISC_TRUE)
-		isc_refcount_destroy(&(*sctxp)->task_cnt);
-	MEM_PUT_AND_DETACH(*sctxp);
+		isc_refcount_destroy(&sctx->task_cnt);
+	MEM_PUT_AND_DETACH(sctx);
 	return result;
 }
 
-- 
1.8.5.3