Attachment 622544 Details for Bug 863576 – git patch file (389-ds-base-1.2.11 branch)

[patch] git patch file (389-ds-base-1.2.11 branch)

0001-Bug-863576-Dirsrv-deadlock-locking-up-IPA.patch (text/plain), 12.98 KB, created by Noriko Hosoi on 2012-10-06 01:10:57 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Noriko Hosoi

Created: 2012-10-06 01:10:57 UTC

Size: 12.98 KB

patch

obsolete

>From 14c47409ab0efe039ea2234738ea619fecff1aa9 Mon Sep 17 00:00:00 2001
>From: Noriko Hosoi <nhosoi@totoro.usersys.redhat.com>
>Date: Fri, 5 Oct 2012 17:56:22 -0700
>Subject: [PATCH] Bug 863576 - Dirsrv deadlock locking up IPA
>
>https://bugzilla.redhat.com/show_bug.cgi?id=863576
>
>Bug Description: Abandon of a Simple Paged Results request causes
>the self deadlock.  When abandoning a simple paged result request,
>the mutex for the connection object c_mutex is locked in do_abandon.
>But to free a pagedresult massage id, pagedresults_free_one_msgid
>called from do_abandon tries to acquire lock on c_mutex again.
>The NSPR lock function PR_Lock is not self re-entrant.  Thus the
>server hangs there due to the self-deadlock.
>
>Fix Description: This patch is removing to call PR_Lock(c_mutex)
>in pagedresults_free_one_msgid and renamed it to pagedresults_free_
>one_msgid_nolock.  To maintain the consistency, "_nolock" is added
>to other pagedresults apis which do not call PR_Lock in it.
>Also, stricter locking on c_mutex is being added to pagedresults_
>parse_control_value to protect the pagedresults related field in
>the connection object.
>---
> ldap/servers/slapd/abandon.c      |    2 +-
> ldap/servers/slapd/connection.c   |    4 +-
> ldap/servers/slapd/daemon.c       |    2 +-
> ldap/servers/slapd/pagedresults.c |  130 +++++++++++++++++++------------------
> ldap/servers/slapd/proto-slap.h   |    8 +-
> 5 files changed, 74 insertions(+), 72 deletions(-)
>
>diff --git a/ldap/servers/slapd/abandon.c b/ldap/servers/slapd/abandon.c
>index 4f00da9..094ae95 100644
>--- a/ldap/servers/slapd/abandon.c
>+++ b/ldap/servers/slapd/abandon.c
>@@ -153,7 +153,7 @@ do_abandon( Slapi_PBlock *pb )
> 	}
> 
> 	if ( op_is_pagedresults(o) ) {
>-		if ( 0 == pagedresults_free_one_msgid(pb->pb_conn, id) ) {
>+		if ( 0 == pagedresults_free_one_msgid_nolock(pb->pb_conn, id) ) {
> 			slapi_log_access( LDAP_DEBUG_STATS, "conn=%" NSPRIu64 
> 			                  " op=%d ABANDON targetop=Simple Paged Results\n",
> 			                  pb->pb_conn->c_connid, pb->pb_op->o_opid );
>diff --git a/ldap/servers/slapd/connection.c b/ldap/servers/slapd/connection.c
>index 9e43104..a3b1df5 100644
>--- a/ldap/servers/slapd/connection.c
>+++ b/ldap/servers/slapd/connection.c
>@@ -2094,7 +2094,7 @@ void connection_enter_leave_turbo(Connection *conn, int current_turbo_flag, int
> 	PR_Lock(conn->c_mutex);
> 	/* We can already be in turbo mode, or not */
> 	current_mode = current_turbo_flag;
>-	if (pagedresults_in_use(conn)) {
>+	if (pagedresults_in_use_nolock(conn)) {
> 		/* PAGED_RESULTS does not need turbo mode */
> 		new_mode = 0;
> 	} else if (conn->c_private->operation_rate == 0) {
>@@ -2780,7 +2780,7 @@ disconnect_server_nomutex( Connection *conn, PRUint64 opconnid, int opid, PRErro
> 	connection_abandon_operations( conn );
> 	/* needed here to ensure simple paged results timeout properly and 
> 	 * don't impact subsequent ops */
>-	pagedresults_reset_timedout(conn);
>+	pagedresults_reset_timedout_nolock(conn);
> 
> 	if (! config_check_referral_mode()) {
> 	    /*
>diff --git a/ldap/servers/slapd/daemon.c b/ldap/servers/slapd/daemon.c
>index 597e131..b611f5c 100644
>--- a/ldap/servers/slapd/daemon.c
>+++ b/ldap/servers/slapd/daemon.c
>@@ -1693,7 +1693,7 @@ setup_pr_read_pds(Connection_Table *ct, PRFileDesc **n_tcps, PRFileDesc **s_tcps
> 				{
> 					int add_fd = 1;
> 					/* check timeout for PAGED RESULTS */
>-                    if (pagedresults_is_timedout(c))
>+                    if (pagedresults_is_timedout_nolock(c))
> 					{
> 						/* Exceeded the timelimit; disconnect the client */
> 						disconnect_server_nomutex(c, c->c_connid, -1,
>diff --git a/ldap/servers/slapd/pagedresults.c b/ldap/servers/slapd/pagedresults.c
>index ea7de14..d445c06 100644
>--- a/ldap/servers/slapd/pagedresults.c
>+++ b/ldap/servers/slapd/pagedresults.c
>@@ -64,6 +64,7 @@ pagedresults_parse_control_value( Slapi_PBlock *pb,
>     struct berval cookie = {0};
>     Connection *conn = pb->pb_conn;
>     Operation *op = pb->pb_op;
>+    BerElement *ber = NULL;
> 
>     LDAPDebug0Args(LDAP_DEBUG_TRACE, "--> pagedresults_parse_control_value\n");
>     if ( NULL == conn || NULL == op || NULL == pagesize || NULL == index ) {
>@@ -76,70 +77,71 @@ pagedresults_parse_control_value( Slapi_PBlock *pb,
> 
>     if ( psbvp->bv_len == 0 || psbvp->bv_val == NULL )
>     {
>-        rc = LDAP_PROTOCOL_ERROR;
>+        LDAPDebug0Args(LDAP_DEBUG_ANY,
>+                    "<-- pagedresults_parse_control_value: no control value\n");
>+        return LDAP_PROTOCOL_ERROR;
>     }
>-    else
>+    ber = ber_init( psbvp );
>+    if ( ber == NULL )
>     {
>-        BerElement *ber = ber_init( psbvp );
>-        if ( ber == NULL )
>-        {
>-            rc = LDAP_OPERATIONS_ERROR;
>-        }
>-        else
>-        {
>-            if ( ber_scanf( ber, "{io}", pagesize, &cookie ) == LBER_ERROR )
>-            {
>-                rc = LDAP_PROTOCOL_ERROR;
>+        LDAPDebug0Args(LDAP_DEBUG_ANY,
>+                    "<-- pagedresults_parse_control_value: no control value\n");
>+        return LDAP_PROTOCOL_ERROR;
>+    }
>+    if ( ber_scanf( ber, "{io}", pagesize, &cookie ) == LBER_ERROR )
>+    {
>+        LDAPDebug0Args(LDAP_DEBUG_ANY,
>+             "<-- pagedresults_parse_control_value: corrupted control value\n");
>+        return LDAP_PROTOCOL_ERROR;
>+    }
>+
>+    PR_Lock(conn->c_mutex);
>+    /* the ber encoding is no longer needed */
>+    ber_free(ber, 1);
>+    if ( cookie.bv_len <= 0 ) {
>+        int i;
>+        int maxlen;
>+        /* first time? */
>+        maxlen = conn->c_pagedresults.prl_maxlen;
>+        if (conn->c_pagedresults.prl_count == maxlen) {
>+            if (0 == maxlen) { /* first time */
>+                conn->c_pagedresults.prl_maxlen = 1;
>+                conn->c_pagedresults.prl_list =
>+                    (PagedResults *)slapi_ch_calloc(1,
>+                                                sizeof(PagedResults));
>+            } else {
>+                /* new max length */
>+                conn->c_pagedresults.prl_maxlen *= 2;
>+                conn->c_pagedresults.prl_list =
>+                    (PagedResults *)slapi_ch_realloc(
>+                                (char *)conn->c_pagedresults.prl_list,
>+                                sizeof(PagedResults) *
>+                                conn->c_pagedresults.prl_maxlen);
>+                /* initialze newly allocated area */
>+                memset(conn->c_pagedresults.prl_list + maxlen, '\0',
>+                           sizeof(PagedResults) * maxlen);
>             }
>-            /* the ber encoding is no longer needed */
>-            ber_free(ber, 1);
>-            if ( cookie.bv_len <= 0 ) {
>-                int i;
>-                int maxlen;
>-                /* first time? */
>-                PR_Lock(conn->c_mutex);
>-                maxlen = conn->c_pagedresults.prl_maxlen;
>-                if (conn->c_pagedresults.prl_count == maxlen) {
>-                    if (0 == maxlen) { /* first time */
>-                        conn->c_pagedresults.prl_maxlen = 1;
>-                        conn->c_pagedresults.prl_list =
>-                            (PagedResults *)slapi_ch_calloc(1,
>-                                                        sizeof(PagedResults));
>-                    } else {
>-                        /* new max length */
>-                        conn->c_pagedresults.prl_maxlen *= 2;
>-                        conn->c_pagedresults.prl_list =
>-                            (PagedResults *)slapi_ch_realloc(
>-                                        (char *)conn->c_pagedresults.prl_list,
>-                                        sizeof(PagedResults) *
>-                                        conn->c_pagedresults.prl_maxlen);
>-                        /* initialze newly allocated area */
>-                        memset(conn->c_pagedresults.prl_list + maxlen, '\0',
>-                                   sizeof(PagedResults) * maxlen);
>-                    }
>-                    *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) {
>-                            *index = i;
>-                            break;
>-                        }
>-                    }
>+            *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) {
>+                    *index = i;
>+                    break;
>                 }
>-                conn->c_pagedresults.prl_count++;
>-                PR_Unlock(conn->c_mutex);
>-            } else {
>-                /* Repeated paged results request.
>-                 * PagedResults is already allocated. */
>-                char *ptr = slapi_ch_malloc(cookie.bv_len + 1);
>-                memcpy(ptr, cookie.bv_val, cookie.bv_len);
>-                *(ptr+cookie.bv_len) = '\0';
>-                *index = strtol(ptr, NULL, 10);
>-                slapi_ch_free_string(&ptr);
>             }
>-            slapi_ch_free((void **)&cookie.bv_val);
>         }
>+        conn->c_pagedresults.prl_count++;
>+    } else {
>+        /* Repeated paged results request.
>+         * PagedResults is already allocated. */
>+        char *ptr = slapi_ch_malloc(cookie.bv_len + 1);
>+        memcpy(ptr, cookie.bv_val, cookie.bv_len);
>+        *(ptr+cookie.bv_len) = '\0';
>+        *index = strtol(ptr, NULL, 10);
>+        slapi_ch_free_string(&ptr);
>     }
>+    slapi_ch_free((void **)&cookie.bv_val);
>+
>     if ((*index > -1) && (*index < conn->c_pagedresults.prl_maxlen)) {
>         /* Need to keep the latest msgid to prepare for the abandon. */
>         conn->c_pagedresults.prl_list[*index].pr_msgid = op->o_msgid;
>@@ -149,6 +151,7 @@ pagedresults_parse_control_value( Slapi_PBlock *pb,
>                       "pagedresults_parse_control_value: invalid cookie: %d\n",
>                       *index);
>     }
>+    PR_Unlock(conn->c_mutex);
> 
>     LDAPDebug1Arg(LDAP_DEBUG_TRACE,
>                   "<-- pagedresults_parse_control_value: idx %d\n", *index);
>@@ -261,7 +264,7 @@ pagedresults_free_one( Connection *conn, int index )
> }
> 
> int 
>-pagedresults_free_one_msgid( Connection *conn, ber_int_t msgid )
>+pagedresults_free_one_msgid_nolock( Connection *conn, ber_int_t msgid )
> {
>     int rc = -1;
>     int i;
>@@ -269,9 +272,9 @@ pagedresults_free_one_msgid( Connection *conn, ber_int_t msgid )
>     LDAPDebug1Arg(LDAP_DEBUG_TRACE,
>                   "--> pagedresults_free_one: msgid=%d\n", msgid);
>     if (conn && (msgid > -1)) {
>-        PR_Lock(conn->c_mutex);
>         if (conn->c_pagedresults.prl_count <= 0) {
>-            LDAPDebug2Args(LDAP_DEBUG_TRACE, "pagedresults_free_one_msgid: "
>+            LDAPDebug2Args(LDAP_DEBUG_TRACE,
>+                           "pagedresults_free_one_msgid_nolock: "
>                            "conn=%d paged requests list count is %d\n",
>                            conn->c_connid, conn->c_pagedresults.prl_count);
>         } else {
>@@ -285,7 +288,6 @@ pagedresults_free_one_msgid( Connection *conn, ber_int_t msgid )
>                 }
>             }
>         }
>-        PR_Unlock(conn->c_mutex);
>     }
> 
>     LDAPDebug1Arg(LDAP_DEBUG_TRACE, "<-- pagedresults_free_one: %d\n", rc);
>@@ -720,7 +722,7 @@ pagedresults_reset_processing(Connection *conn, int index)
> 
> /* Are all the paged results requests timed out? */
> int
>-pagedresults_is_timedout(Connection *conn)
>+pagedresults_is_timedout_nolock(Connection *conn)
> {
>     int i;
>     PagedResults *prp = NULL;
>@@ -753,7 +755,7 @@ pagedresults_is_timedout(Connection *conn)
> 
> /* reset all timeout */
> int
>-pagedresults_reset_timedout(Connection *conn)
>+pagedresults_reset_timedout_nolock(Connection *conn)
> {
>     int i;
>     PagedResults *prp = NULL;
>@@ -773,7 +775,7 @@ pagedresults_reset_timedout(Connection *conn)
> 
> /* paged results requests are in progress. */
> int
>-pagedresults_in_use(Connection *conn)
>+pagedresults_in_use_nolock(Connection *conn)
> {
>     LDAPDebug0Args(LDAP_DEBUG_TRACE, "--> pagedresults_in_use\n");
>     if (NULL == conn) {
>diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h
>index 1b62c13..7e438b7 100644
>--- a/ldap/servers/slapd/proto-slap.h
>+++ b/ldap/servers/slapd/proto-slap.h
>@@ -1421,11 +1421,11 @@ int pagedresults_set_timelimit(Connection *conn, time_t timelimit, int index);
> int pagedresults_cleanup(Connection *conn, int needlock);
> int pagedresults_check_or_set_processing(Connection *conn, int index);
> int pagedresults_reset_processing(Connection *conn, int index);
>-int pagedresults_is_timedout(Connection *conn);
>-int pagedresults_reset_timedout(Connection *conn);
>-int pagedresults_in_use(Connection *conn);
>+int pagedresults_is_timedout_nolock(Connection *conn);
>+int pagedresults_reset_timedout_nolock(Connection *conn);
>+int pagedresults_in_use_nolock(Connection *conn);
> int pagedresults_free_one(Connection *conn, int index);
>-int pagedresults_free_one_msgid( Connection *conn, ber_int_t msgid );
>+int pagedresults_free_one_msgid_nolock( Connection *conn, ber_int_t msgid );
> int op_is_pagedresults(Operation *op);
> int pagedresults_cleanup_all(Connection *conn, int needlock);
> void op_set_pagedresults(Operation *op);
>-- 
>1.7.7.6
>

Flags:

rmeggins: review+

Actions: View | Diff

Attachments on bug 863576: 622544