Hello,
these three bugs were found accidentally while analyzing requirements for
https://fedorahosted.org/bind-dyndb-ldap/ticket/125
All three should go to master before the release because they were source of
subtle bugs.
--
Petr^2 Spacek
From 578e72ffa221f320acfa1a4f7eadb8d97996476f Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Tue, 14 Jun 2016 15:16:41 +0200
Subject: [PATCH] Rework hack for synchronous event processing to make it
reliable.
Previously a pointer to event was used as unique ID which was a terrible
idea. The address was sometimes reused by allocator. Address re-use
lead to false positives in event tracing logic and thus some events
were processed asynchronously even if synchronous processing was requested.
https://fedorahosted.org/bind-dyndb-ldap/ticket/125
---
src/ldap_helper.c | 32 +++++++++-----------------------
src/syncrepl.c | 29 ++++++++++++++++++++---------
src/syncrepl.h | 5 +++--
src/types.h | 14 ++++++++++++++
4 files changed, 46 insertions(+), 34 deletions(-)
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 5264e2d75c27864055ab1c3836b4ffaf98270af7..4610a79cb89f270af1bac799c3e17662267eecc0 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -198,18 +198,6 @@ const ldap_auth_pair_t supported_ldap_auth[] = {
{ AUTH_INVALID, NULL },
};
-#define LDAPDB_EVENT_SYNCREPL_UPDATE (LDAPDB_EVENTCLASS + 1)
-
-typedef struct ldap_syncreplevent ldap_syncreplevent_t;
-struct ldap_syncreplevent {
- ISC_EVENT_COMMON(ldap_syncreplevent_t);
- isc_mem_t *mctx;
- char *dbname;
- char *prevdn;
- int chgtype;
- ldap_entry_t *entry;
-};
-
extern const settings_set_t settings_default_set;
/** Local configuration file */
@@ -3696,7 +3684,7 @@ update_zone(isc_task_t *task, isc_event_t *event)
cleanup:
if (inst != NULL) {
sync_concurr_limit_signal(inst->sctx);
- sync_event_signal(inst->sctx, event);
+ sync_event_signal(inst->sctx, pevent);
if (dns_name_dynamic(&prevname))
dns_name_free(&prevname, inst->mctx);
}
@@ -3732,7 +3720,7 @@ update_config(isc_task_t * task, isc_event_t *event)
cleanup:
if (inst != NULL) {
sync_concurr_limit_signal(inst->sctx);
- sync_event_signal(inst->sctx, event);
+ sync_event_signal(inst->sctx, pevent);
}
if (result != ISC_R_SUCCESS)
log_error_r("update_config (syncrepl) failed for %s. "
@@ -3764,7 +3752,7 @@ update_serverconfig(isc_task_t * task, isc_event_t *event)
cleanup:
if (inst != NULL) {
sync_concurr_limit_signal(inst->sctx);
- sync_event_signal(inst->sctx, event);
+ sync_event_signal(inst->sctx, pevent);
}
if (result != ISC_R_SUCCESS)
log_error_r("update_serverconfig (syncrepl) failed for %s. "
@@ -4069,15 +4057,15 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t **entryp, int chgtype)
isc_result_t result = ISC_R_SUCCESS;
ldap_syncreplevent_t *pevent = NULL;
ldap_entry_t *entry = NULL;
- isc_event_t *wait_event = NULL;
dns_name_t *zone_name = NULL;
dns_zone_t *zone_ptr = NULL;
char *dn = NULL;
char *dbname = NULL;
isc_mem_t *mctx = NULL;
isc_taskaction_t action = NULL;
isc_task_t *task = NULL;
sync_state_t sync_state;
+ isc_boolean_t synchronous;
REQUIRE(entryp != NULL);
entry = *entryp;
@@ -4113,10 +4101,12 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t **entryp, int chgtype)
isc_task_attach(inst->task, &task);
result = ISC_R_SUCCESS;
}
+ synchronous = ISC_FALSE;
} else {
/* For configuration object and zone object use single task
* to make sure that the exclusive mode actually works. */
isc_task_attach(inst->task, &task);
+ synchronous = ISC_TRUE;
}
REQUIRE(task != NULL);
@@ -4168,23 +4158,19 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t **entryp, int chgtype)
pevent->prevdn = NULL;
pevent->chgtype = chgtype;
pevent->entry = entry;
- wait_event = (isc_event_t *)pevent;
- isc_task_send(task, (isc_event_t **)&pevent);
- *entryp = NULL; /* event handler will deallocate the LDAP entry */
/* Lock syncrepl queue to prevent zone, config and resource records
* from racing with each other. */
- if (action == update_zone || action == update_config
- || action == update_serverconfig)
- CHECK(sync_event_wait(inst->sctx, wait_event));
+ CHECK(sync_event_send(inst->sctx, task, &pevent, synchronous));
+ *entryp = NULL; /* event handler will deallocate the LDAP entry */
cleanup:
if (zone_ptr != NULL)
dns_zone_detach(&zone_ptr);
if (result != ISC_R_SUCCESS)
log_error_r("syncrepl_update failed for %s",
ldap_entry_logname(entry));
- if (wait_event == NULL) {
+ if (pevent != NULL) {
/* Event was not sent */
sync_concurr_limit_signal(inst->sctx);
diff --git a/src/syncrepl.c b/src/syncrepl.c
index 1117358e9a1ac3de89f8c1504dae87ca1eaaa80a..00796448b4e5121a8cc5cb2e623b959553bb6bcb 100644
--- a/src/syncrepl.c
+++ b/src/syncrepl.c
@@ -91,10 +91,11 @@ struct sync_ctx {
isc_condition_t cond; /**< for signal when task_cnt == 0 */
sync_state_t state;
ldap_instance_t *inst;
- isc_event_t *last_ev; /**< Last processed event */
ISC_LIST(task_element_t) tasks; /**< list of tasks processing
events from initial
synchronization phase */
+ isc_uint32_t next_id; /**< next sequential id */
+ isc_uint32_t last_id; /**< last processed event */
};
/**
@@ -587,20 +588,29 @@ sync_concurr_limit_signal(sync_ctx_t *sctx) {
}
/**
- * Wait until given event ev is processed.
+ * Send ISC event to specified task and optionally wait until given event
+ * is processed.
*
- * End of event processing has to be signalled by
- * sync_event_signal() call.
+ * End of event processing has to be signaled by
+ * @see sync_event_signal() call.
*/
isc_result_t
-sync_event_wait(sync_ctx_t *sctx, isc_event_t *ev) {
+sync_event_send(sync_ctx_t *sctx, isc_task_t *task, ldap_syncreplevent_t **ev,
+ isc_boolean_t synchronous) {
isc_result_t result;
isc_time_t abs_timeout;
+ isc_uint32_t seqid;
+ isc_boolean_t locked = ISC_FALSE;
REQUIRE(sctx != NULL);
LOCK(&sctx->mutex);
- while (sctx->last_ev != ev) {
+ locked = ISC_TRUE;
+ /* overflow is not a problem as long as the modulo is smaller than
+ * constant used by sync_concurr_limit_wait() */
+ (*ev)->seqid = seqid = ++sctx->next_id % 0xffffffff;
+ isc_task_send(task, (isc_event_t **)ev);
+ while (synchronous == ISC_TRUE && sctx->last_id != seqid) {
if (ldap_instance_isexiting(sctx->inst) == ISC_TRUE)
CLEANUP_WITH(ISC_R_SHUTTINGDOWN);
@@ -613,20 +623,21 @@ sync_event_wait(sync_ctx_t *sctx, isc_event_t *ev) {
result = ISC_R_SUCCESS;
cleanup:
- UNLOCK(&sctx->mutex);
+ if (locked == ISC_TRUE)
+ UNLOCK(&sctx->mutex);
return result;
}
/**
* Signal that given syncrepl event was processed.
*/
void
-sync_event_signal(sync_ctx_t *sctx, isc_event_t *ev) {
+sync_event_signal(sync_ctx_t *sctx, ldap_syncreplevent_t *ev) {
REQUIRE(sctx != NULL);
REQUIRE(ev != NULL);
LOCK(&sctx->mutex);
- sctx->last_ev = ev;
+ sctx->last_id = ev->seqid;
BROADCAST(&sctx->cond);
UNLOCK(&sctx->mutex);
}
diff --git a/src/syncrepl.h b/src/syncrepl.h
index eb6b8d0c9a6a0ab8f7595f4735e333fce9808f26..ba3070a2cd70adcb7543148e458ed89a8b7d2bec 100644
--- a/src/syncrepl.h
+++ b/src/syncrepl.h
@@ -58,9 +58,10 @@ void
sync_concurr_limit_signal(sync_ctx_t *sctx) ATTR_NONNULLS;
isc_result_t
-sync_event_wait(sync_ctx_t *sctx, isc_event_t *ev) ATTR_NONNULLS ATTR_CHECKRESULT;
+sync_event_send(sync_ctx_t *sctx, isc_task_t *task, ldap_syncreplevent_t **ev,
+ isc_boolean_t synchronous) ATTR_NONNULLS ATTR_CHECKRESULT;
void
-sync_event_signal(sync_ctx_t *sctx, isc_event_t *ev) ATTR_NONNULLS;
+sync_event_signal(sync_ctx_t *sctx, ldap_syncreplevent_t *ev) ATTR_NONNULLS;
#endif /* SYNCREPL_H_ */
diff --git a/src/types.h b/src/types.h
index 92c9a7c109d2cfba635fd1373cdff76f826ab5f1..57d55796d0678925c7b2c58d730b36857d8599ee 100644
--- a/src/types.h
+++ b/src/types.h
@@ -5,6 +5,7 @@
#ifndef _LD_TYPES_H_
#define _LD_TYPES_H_
+#include <isc/event.h>
#include <isc/refcount.h>
#include <dns/name.h>
@@ -36,4 +37,17 @@ typedef struct mldapdb mldapdb_t;
typedef struct ldap_entry ldap_entry_t;
typedef struct settings_set settings_set_t;
+
+#define LDAPDB_EVENT_SYNCREPL_UPDATE (LDAPDB_EVENTCLASS + 1)
+typedef struct ldap_syncreplevent ldap_syncreplevent_t;
+struct ldap_syncreplevent {
+ ISC_EVENT_COMMON(ldap_syncreplevent_t);
+ isc_mem_t *mctx;
+ char *dbname;
+ char *prevdn;
+ int chgtype;
+ ldap_entry_t *entry;
+ isc_uint32_t seqid;
+};
+
#endif /* !_LD_TYPES_H_ */
--
2.5.5
From b45ff02fd42a2e2694b0f5959b583605da5e7d93 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Tue, 14 Jun 2016 15:19:07 +0200
Subject: [PATCH] Avoid redundant sync_task_add() calls.
inst->task is always added to inst->sctx task list during sctx
initialization so there is no point in adding it again and again.
---
src/ldap_helper.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 4610a79cb89f270af1bac799c3e17662267eecc0..50c0277f4e578db74ba6b44d49082631c3b15692 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -4064,7 +4064,6 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t **entryp, int chgtype)
isc_mem_t *mctx = NULL;
isc_taskaction_t action = NULL;
isc_task_t *task = NULL;
- sync_state_t sync_state;
isc_boolean_t synchronous;
REQUIRE(entryp != NULL);
@@ -4133,16 +4132,6 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t **entryp, int chgtype)
goto cleanup;
}
- /* All events for single zone are handled by one task, so we don't
- * need to spend time with normal records. */
- if (action == update_zone || action == update_config
- || action == update_serverconfig) {
- INSIST(task == inst->task); /* For task-exclusive mode */
- sync_state_get(inst->sctx, &sync_state);
- if (sync_state == sync_configinit || sync_state == sync_datainit)
- CHECK(sync_task_add(inst->sctx, task));
- }
-
pevent = (ldap_syncreplevent_t *)isc_event_allocate(inst->mctx,
inst, LDAPDB_EVENT_SYNCREPL_UPDATE,
action, NULL,
--
2.5.5
From de083b965e3f73e398b1dc9ca4d47e2263c89850 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Tue, 14 Jun 2016 16:37:44 +0200
Subject: [PATCH] Make task selection in syncrepl_update() deterministic.
The fallback task selection logic in syncrepl_update() was
hidding problems in other parts of code and made event processing
non-deterministic, which lead to subtle bugs.
Now the fallback is removed so records destined at non-existing zones
will fail immediatelly instead of failing in later stages or causing
mayhem.
https://fedorahosted.org/bind-dyndb-ldap/ticket/125
---
src/ldap_helper.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 50c0277f4e578db74ba6b44d49082631c3b15692..5d19e1f6d626ec5cad97c8c78ec665d1b91633c8 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -4088,18 +4088,9 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t **entryp, int chgtype)
* See discussion about run_exclusive_begin() function in lock.c. */
if ((entry->class & LDAP_ENTRYCLASS_RR) != 0 &&
(entry->class & LDAP_ENTRYCLASS_MASTER) == 0) {
- result = zr_get_zone_ptr(inst->zone_register, zone_name,
- &zone_ptr, NULL);
- if (result == ISC_R_SUCCESS && dns_zone_getmgr(zone_ptr) != NULL)
- dns_zone_gettask(zone_ptr, &task);
- else {
- /* TODO: Fix race condition:
- * zone is not (yet) present in zone register */
- log_debug(1, "TODO: %s: task fallback",
- ldap_entry_logname(entry));
- isc_task_attach(inst->task, &task);
- result = ISC_R_SUCCESS;
- }
+ CHECK(zr_get_zone_ptr(inst->zone_register, zone_name,
+ &zone_ptr, NULL));
+ dns_zone_gettask(zone_ptr, &task);
synchronous = ISC_FALSE;
} else {
/* For configuration object and zone object use single task
--
2.5.5
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
