[389-commits] 2 commits - ldap/servers
Noriko Hosoi
nhosoi at fedoraproject.org
Fri Jun 5 06:10:28 UTC 2015
ldap/servers/slapd/back-ldbm/dblayer.c | 30 ++-------
ldap/servers/slapd/opshared.c | 41 +++++-------
ldap/servers/slapd/pagedresults.c | 110 +++++++++++++++++++++------------
3 files changed, 98 insertions(+), 83 deletions(-)
New commits:
commit f3c3125fc9fd4dc1cbd41027d6442cb525efd014
Author: Noriko Hosoi <nhosoi at redhat.com>
Date: Thu Jun 4 16:27:05 2015 -0700
Ticket #48192 - Individual abandoned simple paged results request has no chance to be cleaned up
Description: When allocating a slot in simple paged results array stashed
in a connection in pagedresults_parse_control_value, a new code is added
to scan the array if the existing slot is timed out or not. If it is, the
slot is released and a search results is released if it is attached.
Also, if a request is abandoned, instead of returning a valid cookie, it
changed to return an empty cookie to inform the client the request was not
successfully completed.
https://fedorahosted.org/389/ticket/48192
Reviewed by rmeggins at redhat.com (Thank you, Rich!!)
diff --git a/ldap/servers/slapd/opshared.c b/ldap/servers/slapd/opshared.c
index fa88053..1623fb0 100644
--- a/ldap/servers/slapd/opshared.c
+++ b/ldap/servers/slapd/opshared.c
@@ -517,8 +517,7 @@ op_shared_search (Slapi_PBlock *pb, int send_result)
rc = pagedresults_parse_control_value(pb, ctl_value, &pagesize, &pr_idx, be);
/* Let's set pr_idx even if it fails; in case, pr_idx == -1. */
slapi_pblock_set(pb, SLAPI_PAGED_RESULTS_INDEX, &pr_idx);
- if ((LDAP_SUCCESS == rc) ||
- (LDAP_CANCELLED == rc) || (0 == pagesize)) {
+ if ((LDAP_SUCCESS == rc) || (LDAP_CANCELLED == rc) || (0 == pagesize)) {
unsigned int opnote = SLAPI_OP_NOTE_SIMPLEPAGED;
op_set_pagedresults(operation);
pr_be = pagedresults_get_current_be(pb->pb_conn, pr_idx);
@@ -545,9 +544,8 @@ op_shared_search (Slapi_PBlock *pb, int send_result)
}
slapi_pblock_set( pb, SLAPI_OPERATION_NOTES, &opnote );
if ((LDAP_CANCELLED == rc) || (0 == pagesize)) {
- /* paged-results-request was abandoned */
- pagedresults_set_response_control(pb, 0, estimate,
- curr_search_count, pr_idx);
+ /* paged-results-request was abandoned; making an empty cookie. */
+ pagedresults_set_response_control(pb, 0, estimate, -1, pr_idx);
send_ldap_result(pb, 0, NULL,
"Simple Paged Results Search abandoned",
0, NULL);
@@ -574,10 +572,21 @@ op_shared_search (Slapi_PBlock *pb, int send_result)
/* adjust time and size limits */
compute_limits (pb);
-
- /* call the pre-search plugins. if they succeed, call the backend
- search function. then call the post-search plugins. */
+ /* set the timelimit to clean up the too-long-lived-paged results requests */
+ if (op_is_pagedresults(operation)) {
+ time_t optime, time_up;
+ int tlimit;
+ slapi_pblock_get( pb, SLAPI_SEARCH_TIMELIMIT, &tlimit );
+ slapi_pblock_get( pb, SLAPI_OPINITIATED_TIME, &optime );
+ time_up = (tlimit==-1 ? -1 : optime + tlimit); /* -1: no time limit */
+ pagedresults_set_timelimit(pb->pb_conn, pb->pb_op, time_up, pr_idx);
+ }
+
+ /*
+ * call the pre-search plugins. if they succeed, call the backend
+ * search function. then call the post-search plugins.
+ */
/* ONREPL - should regular plugin be called for internal searches ? */
if (plugin_call_plugins(pb, SLAPI_PLUGIN_PRE_SEARCH_FN) == 0)
{
@@ -642,16 +651,6 @@ op_shared_search (Slapi_PBlock *pb, int send_result)
}
}
- /* set the timelimit to clean up the too-long-lived-paged results requests */
- if (op_is_pagedresults(operation)) {
- time_t optime, time_up;
- int tlimit;
- slapi_pblock_get( pb, SLAPI_SEARCH_TIMELIMIT, &tlimit );
- slapi_pblock_get( pb, SLAPI_OPINITIATED_TIME, &optime );
- time_up = (tlimit==-1 ? -1 : optime + tlimit); /* -1: no time limit */
- pagedresults_set_timelimit(pb->pb_conn, pb->pb_op, time_up, pr_idx);
- }
-
/* PAR: now filters have been rewritten, we can assign plugins to work on them */
index_subsys_assign_filter_decoders(pb);
@@ -880,10 +879,7 @@ op_shared_search (Slapi_PBlock *pb, int send_result)
slapi_pblock_get(pb, SLAPI_SEARCH_RESULT_SET, &sr);
if (PAGEDRESULTS_SEARCH_END == pr_stat) {
if (sr) { /* in case a left over sr is found, clean it up */
- PR_Lock(pb->pb_conn->c_mutex);
- pagedresults_set_search_result(pb->pb_conn, operation, NULL, 1, pr_idx);
- be->be_search_results_release(&sr);
- PR_Unlock(pb->pb_conn->c_mutex);
+ pagedresults_free_one(pb->pb_conn, operation, pr_idx);
}
if (NULL == next_be) {
/* no more entries && no more backends */
@@ -1306,7 +1302,6 @@ iterate(Slapi_PBlock *pb, Slapi_Backend *be, int send_result,
operation_out_of_disk_space();
}
pr_stat = PAGEDRESULTS_SEARCH_END;
- pagedresults_set_timelimit(pb->pb_conn, pb->pb_op, 0, pr_idx);
rval = -1;
done = 1;
continue;
diff --git a/ldap/servers/slapd/pagedresults.c b/ldap/servers/slapd/pagedresults.c
index 327da54..402dd10 100644
--- a/ldap/servers/slapd/pagedresults.c
+++ b/ldap/servers/slapd/pagedresults.c
@@ -41,6 +41,27 @@
#include "slap.h"
+/* helper function to clean up one prp slot */
+static void
+_pr_cleanup_one_slot(PagedResults *prp)
+{
+ PRLock *prmutex = NULL;
+ if (!prp) {
+ return;
+ }
+ if (prp->pr_current_be && prp->pr_current_be->be_search_results_release) {
+ /* sr is left; release it. */
+ prp->pr_current_be->be_search_results_release(&(prp->pr_search_result_set));
+ }
+ /* clean up the slot */
+ if (prp->pr_mutex) {
+ /* pr_mutex is reused; back it up and reset it. */
+ prmutex = prp->pr_mutex;
+ }
+ memset(prp, '\0', sizeof(PagedResults));
+ prp->pr_mutex = prmutex;
+}
+
/*
* Parse the value from an LDAPv3 "Simple Paged Results" control. They look
* like this:
@@ -65,6 +86,7 @@ pagedresults_parse_control_value( Slapi_PBlock *pb,
Connection *conn = pb->pb_conn;
Operation *op = pb->pb_op;
BerElement *ber = NULL;
+ PagedResults *prp = NULL;
LDAPDebug0Args(LDAP_DEBUG_TRACE, "--> pagedresults_parse_control_value\n");
if ( NULL == conn || NULL == op || NULL == pagesize || NULL == index ) {
@@ -117,13 +139,31 @@ pagedresults_parse_control_value( Slapi_PBlock *pb,
}
*index = maxlen; /* the first position in the new area */
} else {
- for (i = 0; i < conn->c_pagedresults.prl_maxlen; i++) {
- if (!conn->c_pagedresults.prl_list[i].pr_current_be) {
- conn->c_pagedresults.prl_list[i].pr_current_be = be;
+ time_t ctime = current_time();
+ prp = conn->c_pagedresults.prl_list;
+ for (i = 0; i < conn->c_pagedresults.prl_maxlen; i++, prp++) {
+ if (!prp->pr_current_be) { /* unused slot; take it */
+ prp->pr_current_be = be;
+ *index = i;
+ break;
+ } else if (((prp->pr_timelimit > 0) && (ctime < prp->pr_timelimit)) || /* timelimit exceeded */
+ (prp->pr_flags & CONN_FLAG_PAGEDRESULTS_ABANDONED) /* abandoned */) {
+ _pr_cleanup_one_slot(prp);
+ conn->c_pagedresults.prl_count--;
+ prp->pr_current_be = be;
*index = i;
break;
}
}
+ /* cleaning up the rest of the timedout if any */
+ for (++i; i < conn->c_pagedresults.prl_maxlen; i++, prp++) {
+ if (prp->pr_current_be &&
+ (((prp->pr_timelimit > 0) && (ctime < prp->pr_timelimit)) || /* timelimit exceeded */
+ (prp->pr_flags & CONN_FLAG_PAGEDRESULTS_ABANDONED)) /* abandoned */) {
+ _pr_cleanup_one_slot(prp);
+ conn->c_pagedresults.prl_count--;
+ }
+ }
}
if ((*index > -1) && (*index < conn->c_pagedresults.prl_maxlen) &&
!conn->c_pagedresults.prl_list[*index].pr_mutex) {
@@ -131,7 +171,6 @@ pagedresults_parse_control_value( Slapi_PBlock *pb,
}
conn->c_pagedresults.prl_count++;
} else {
- PagedResults *prp = NULL;
/* Repeated paged results request.
* PagedResults is already allocated. */
char *ptr = slapi_ch_malloc(cookie.bv_len + 1);
@@ -156,8 +195,10 @@ pagedresults_parse_control_value( Slapi_PBlock *pb,
slapi_ch_free((void **)&cookie.bv_val);
if ((*index > -1) && (*index < conn->c_pagedresults.prl_maxlen)) {
- if (conn->c_pagedresults.prl_list[*index].pr_flags &
- CONN_FLAG_PAGEDRESULTS_ABANDONED) {
+ if (conn->c_pagedresults.prl_list[*index].pr_flags & CONN_FLAG_PAGEDRESULTS_ABANDONED) {
+ /* repeated case? */
+ prp = conn->c_pagedresults.prl_list + *index;
+ _pr_cleanup_one_slot(prp);
rc = LDAP_CANCELLED;
} else {
/* Need to keep the latest msgid to prepare for the abandon. */
@@ -273,20 +314,8 @@ pagedresults_free_one( Connection *conn, Operation *op, int index )
"conn=%d paged requests list count is %d\n",
conn->c_connid, conn->c_pagedresults.prl_count);
} else if (index < conn->c_pagedresults.prl_maxlen) {
- PRLock *prmutex = NULL;
PagedResults *prp = conn->c_pagedresults.prl_list + index;
- if (prp && prp->pr_current_be &&
- prp->pr_current_be->be_search_results_release &&
- prp->pr_search_result_set) {
- prp->pr_current_be->be_search_results_release(&(prp->pr_search_result_set));
- }
- prp->pr_current_be = NULL;
- if (prp->pr_mutex) {
- /* pr_mutex is reused; back it up and reset it. */
- prmutex = prp->pr_mutex;
- }
- memset(prp, '\0', sizeof(PagedResults));
- prp->pr_mutex = prmutex;
+ _pr_cleanup_one_slot(prp);
conn->c_pagedresults.prl_count--;
rc = 0;
}
@@ -309,7 +338,7 @@ pagedresults_free_one_msgid_nolock( Connection *conn, ber_int_t msgid )
; /* Not a paged result. */
} else {
LDAPDebug1Arg(LDAP_DEBUG_TRACE,
- "--> pagedresults_free_one: msgid=%d\n", msgid);
+ "--> pagedresults_free_one_msgid_nolock: msgid=%d\n", msgid);
for (i = 0; i < conn->c_pagedresults.prl_maxlen; i++) {
if (conn->c_pagedresults.prl_list[i].pr_msgid == msgid) {
PagedResults *prp = conn->c_pagedresults.prl_list + i;
@@ -318,16 +347,14 @@ pagedresults_free_one_msgid_nolock( Connection *conn, ber_int_t msgid )
prp->pr_search_result_set) {
prp->pr_current_be->be_search_results_release(&(prp->pr_search_result_set));
}
- prp->pr_current_be = NULL;
prp->pr_flags |= CONN_FLAG_PAGEDRESULTS_ABANDONED;
prp->pr_flags &= ~CONN_FLAG_PAGEDRESULTS_PROCESSING;
- conn->c_pagedresults.prl_count--;
rc = 0;
break;
}
}
LDAPDebug1Arg(LDAP_DEBUG_TRACE,
- "<-- pagedresults_free_one: %d\n", rc);
+ "<-- pagedresults_free_one_msgid_nolock: %d\n", rc);
}
}
@@ -845,37 +872,42 @@ pagedresults_reset_processing(Connection *conn, int index)
}
#endif
-/* Are all the paged results requests timed out? */
+/*
+ * This timedout is mainly for an end user leaves a commandline untouched
+ * for a long time. This should not affect a permanent connection which
+ * manages multiple simple paged results requests over the connection.
+ *
+ * [rule]
+ * If there is just one slot and it's timed out, we return it is timedout.
+ * If there are multiple slots, the connection may be a permanent one.
+ * Do not return timed out here. But let the next request take care the
+ * timedout slot(s).
+ */
int
pagedresults_is_timedout_nolock(Connection *conn)
{
- int i;
PagedResults *prp = NULL;
time_t ctime;
- int rc = 0;
LDAPDebug0Args(LDAP_DEBUG_TRACE, "--> pagedresults_is_timedout\n");
- if (NULL == conn || 0 == conn->c_pagedresults.prl_count) {
- LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- pagedresults_is_timedout: -\n");
- return rc;
+ if (!conn || (0 == conn->c_pagedresults.prl_maxlen)) {
+ LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- pagedresults_is_timedout: false\n");
+ return 0;
}
ctime = current_time();
- for (i = 0; i < conn->c_pagedresults.prl_maxlen; i++) {
- prp = conn->c_pagedresults.prl_list + i;
+ prp = conn->c_pagedresults.prl_list;
+ if (prp && (1 == conn->c_pagedresults.prl_maxlen)) {
if (prp->pr_current_be && (prp->pr_timelimit > 0)) {
- if (ctime < prp->pr_timelimit) {
- LDAPDebug0Args(LDAP_DEBUG_TRACE,
- "<-- pagedresults_is_timedout: 0\n");
- return 0; /* at least, one request is not timed out. */
- } else {
- rc = 1; /* possibly timed out */
+ if (ctime > prp->pr_timelimit) {
+ LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- pagedresults_is_timedout: true\n");
+ return 1;
}
}
}
- LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- pagedresults_is_timedout: 1\n");
- return rc; /* all requests are timed out. */
+ LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- pagedresults_is_timedout: false\n");
+ return 0;
}
/* reset all timeout */
commit 460b764e846df7f41c62eec7015331c0caf7481c
Author: Noriko Hosoi <nhosoi at redhat.com>
Date: Thu Jun 4 23:07:39 2015 -0700
Revert "Ticket 48149 - ns-slapd double free or corruption crash"
This reverts commit 8d64b8e348bbfca51411723d396beec7db5c34b9.
This workaround is not needed since the latest libdb has no problem.
diff --git a/ldap/servers/slapd/back-ldbm/dblayer.c b/ldap/servers/slapd/back-ldbm/dblayer.c
index 568b7c4..ac315bb 100644
--- a/ldap/servers/slapd/back-ldbm/dblayer.c
+++ b/ldap/servers/slapd/back-ldbm/dblayer.c
@@ -102,19 +102,15 @@
#endif
#if 1000*DB_VERSION_MAJOR + 100*DB_VERSION_MINOR >= 4100
-#define DB_OPEN(priv, oflags, db, txnid, file, database, type, flags, mode, rval) \
+#define DB_OPEN(oflags, db, txnid, file, database, type, flags, mode, rval) \
{ \
if (((oflags) & DB_INIT_TXN) && ((oflags) & DB_INIT_LOG)) \
{ \
- if ((priv)) slapi_rwlock_rdlock((priv)->dblayer_env_lock); \
(rval) = ((db)->open)((db), (txnid), (file), (database), (type), (flags)|DB_AUTO_COMMIT, (mode)); \
- if ((priv)) slapi_rwlock_unlock((priv)->dblayer_env_lock); \
} \
else \
{ \
- if ((priv)) slapi_rwlock_rdlock((priv)->dblayer_env_lock); \
(rval) = ((db)->open)((db), (txnid), (file), (database), (type), (flags), (mode)); \
- if ((priv)) slapi_rwlock_unlock((priv)->dblayer_env_lock); \
} \
}
/* 608145: db4.1 and newer does not require exclusive lock for checkpointing
@@ -122,7 +118,7 @@
#define DB_CHECKPOINT_LOCK(use_lock, lock) ;
#define DB_CHECKPOINT_UNLOCK(use_lock, lock) ;
#else /* older then db 41 */
-#define DB_OPEN(env, oflags, db, txnid, file, database, type, flags, mode, rval) \
+#define DB_OPEN(oflags, db, txnid, file, database, type, flags, mode, rval) \
(rval) = (db)->open((db), (file), (database), (type), (flags), (mode))
#define DB_CHECKPOINT_LOCK(use_lock, lock) if(use_lock) slapi_rwlock_wrlock(lock);
#define DB_CHECKPOINT_UNLOCK(use_lock, lock) if(use_lock) slapi_rwlock_unlock(lock);
@@ -2344,7 +2340,7 @@ int dblayer_instance_start(backend *be, int mode)
abs_id2entry_file = slapi_ch_smprintf( "%s%c%s", inst_dirp,
get_sep(inst_dirp), ID2ENTRY LDBM_FILENAME_SUFFIX);
- DB_OPEN(mypEnv, mypEnv->dblayer_openflags,
+ DB_OPEN(mypEnv->dblayer_openflags,
dbp, NULL/* txnid */, abs_id2entry_file, subname, DB_BTREE,
open_flags, priv->dblayer_file_mode, return_value);
dbp->close(dbp, 0);
@@ -2375,7 +2371,7 @@ int dblayer_instance_start(backend *be, int mode)
#endif
slapi_ch_free_string(&abs_id2entry_file);
}
- DB_OPEN(mypEnv, mypEnv->dblayer_openflags,
+ DB_OPEN(mypEnv->dblayer_openflags,
dbp, NULL/* txnid */, id2entry_file, subname, DB_BTREE,
open_flags, priv->dblayer_file_mode, return_value);
if (0 != return_value) {
@@ -2652,7 +2648,7 @@ dblayer_get_aux_id2entry_ext(backend *be, DB **ppDB, DB_ENV **ppEnv,
}
PR_ASSERT(dblayer_inst_exists(inst, NULL));
- DB_OPEN(mypEnv, envflags, dbp, NULL/* txnid */, id2entry_file, subname, DB_BTREE,
+ DB_OPEN(envflags, dbp, NULL/* txnid */, id2entry_file, subname, DB_BTREE,
dbflags, priv->dblayer_file_mode, rval);
if (rval) {
LDAPDebug(LDAP_DEBUG_ANY,
@@ -3224,7 +3220,7 @@ dblayer_open_file(backend *be, char* indexname, int open_flag,
}
abs_file_name = slapi_ch_smprintf("%s%c%s",
inst_dirp, get_sep(inst_dirp), file_name);
- DB_OPEN(pENV, pENV->dblayer_openflags,
+ DB_OPEN(pENV->dblayer_openflags,
dbp, NULL/* txnid */, abs_file_name, subname, DB_BTREE,
open_flags, priv->dblayer_file_mode, return_value);
dbp->close(dbp, 0);
@@ -3240,7 +3236,7 @@ dblayer_open_file(backend *be, char* indexname, int open_flag,
slapi_ch_free_string(&abs_file_name);
}
- DB_OPEN(pENV, pENV->dblayer_openflags,
+ DB_OPEN(pENV->dblayer_openflags,
dbp, NULL, /* txnid */ rel_path, subname, DB_BTREE,
open_flags, priv->dblayer_file_mode, return_value);
#if 1000*DB_VERSION_MAJOR + 100*DB_VERSION_MINOR == 4100
@@ -5170,7 +5166,6 @@ int dblayer_memp_stat(struct ldbminfo *li, DB_MPOOL_STAT **gsp,
{
dblayer_private *priv = NULL;
DB_ENV *env = NULL;
- int rc;
PR_ASSERT(NULL != li);
@@ -5180,10 +5175,7 @@ int dblayer_memp_stat(struct ldbminfo *li, DB_MPOOL_STAT **gsp,
env = priv->dblayer_env->dblayer_DB_ENV;
PR_ASSERT(NULL != env);
- slapi_rwlock_wrlock(priv->dblayer_env->dblayer_env_lock);
- rc = MEMP_STAT(env, gsp, fsp, 0, (void *)slapi_ch_malloc);
- slapi_rwlock_unlock(priv->dblayer_env->dblayer_env_lock);
- return rc;
+ return MEMP_STAT(env, gsp, fsp, 0, (void *)slapi_ch_malloc);
}
/* import wants this one */
@@ -5192,7 +5184,6 @@ int dblayer_memp_stat_instance(ldbm_instance *inst, DB_MPOOL_STAT **gsp,
{
DB_ENV *env = NULL;
dblayer_private *priv = NULL;
- int rc;
PR_ASSERT(NULL != inst);
@@ -5205,10 +5196,7 @@ int dblayer_memp_stat_instance(ldbm_instance *inst, DB_MPOOL_STAT **gsp,
}
PR_ASSERT(NULL != env);
- slapi_rwlock_wrlock(priv->dblayer_env->dblayer_env_lock);
- rc = MEMP_STAT(env, gsp, fsp, 0, (void *)slapi_ch_malloc);
- slapi_rwlock_unlock(priv->dblayer_env->dblayer_env_lock);
- return rc;
+ return MEMP_STAT(env, gsp, fsp, 0, (void *)slapi_ch_malloc);
}
/* Helper functions for recovery */
More information about the 389-commits
mailing list