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

[Petr Spacek]

On 21.2.2014 19:35, Lukas Slebodnik wrote:
> 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

ACK. Thank you for discovering this!

Pushed to master: e346fbce099eacb1cd860e0624dcaaea36161169

-- 
Petr^2 Spacek