ITS#9983 Rework op->o_refcnt decref sequencing

epoch_append should be called at the point the object is not reachable
anymore, otherwise a thread from a "future" might still access it post
reclamation.

diff --git a/servers/lloadd/bind.c b/servers/lloadd/bind.c
index 038a3607396b018d293fb0db278ffaa309667726..eaecb24f1b9a4a25640587062a9a4fa6bcc0e756 100644 (file)
--- a/servers/lloadd/bind.c
+++ b/servers/lloadd/bind.c
@@ -410,7 +410,7 @@ request_bind( LloadConnection *client, LloadOperation *op )
         Debug( LDAP_DEBUG_ANY, "request_bind: "
                 "ber_alloc failed\n" );
 
-        operation_unlink( op );
+        OPERATION_UNLINK(op);
 
         CONNECTION_LOCK(client);
         goto fail;
@@ -989,7 +989,7 @@ handle_vc_bind_response(
     }
 
 done:
-    operation_unlink( op );
+    OPERATION_UNLINK(op);
     ber_free( ber, 1 );
     return rc;
 }

diff --git a/servers/lloadd/client.c b/servers/lloadd/client.c
index 8faf4aafad019c4b98f9b02468d809d5ff7960a0..1241286f29c73929a23034163ecac7fa636b216f 100644 (file)
--- a/servers/lloadd/client.c
+++ b/servers/lloadd/client.c
@@ -45,7 +45,7 @@ request_abandon( LloadConnection *c, LloadOperation *op )
                 "connid=%lu msgid=%d invalid integer sent in abandon request\n",
                 c->c_connid, op->o_client_msgid );
 
-        operation_unlink( op );
+        OPERATION_UNLINK(op);
         CONNECTION_LOCK_DESTROY(c);
         return -1;
     }
@@ -81,7 +81,7 @@ request_abandon( LloadConnection *c, LloadOperation *op )
     operation_abandon( request );
 
 done:
-    operation_unlink( op );
+    OPERATION_UNLINK(op);
     return rc;
 }
 
@@ -332,7 +332,7 @@ fail:
         operation_send_reject( op, LDAP_OTHER, "internal error", 0 );
     }
 
-    operation_unlink( op );
+    OPERATION_UNLINK(op);
     if ( rc ) {
         CONNECTION_LOCK_DESTROY(client);
     }
@@ -381,7 +381,7 @@ handle_one_request( LloadConnection *c )
         case LDAP_REQ_UNBIND:
             /* There is never a response for this operation */
             op->o_res = LLOAD_OP_COMPLETED;
-            operation_unlink( op );
+            OPERATION_UNLINK(op);
 
             Debug( LDAP_DEBUG_STATS, "handle_one_request: "
                     "received unbind, closing client connid=%lu\n",

diff --git a/servers/lloadd/connection.c b/servers/lloadd/connection.c
index 14f663e66ffd582316d4ef7452708a3e3ac0a471..a3f5323f0ba3ef5616b6f8fd4e1abe4aaa583d90 100644 (file)
--- a/servers/lloadd/connection.c
+++ b/servers/lloadd/connection.c
@@ -569,7 +569,7 @@ lload_connection_close( LloadConnection *c, void *arg )
         }
 
         CONNECTION_UNLOCK(c);
-        operation_unlink( op );
+        OPERATION_UNLINK(op);
         CONNECTION_LOCK(c);
     } while ( c->c_ops );
 

diff --git a/servers/lloadd/epoch.c b/servers/lloadd/epoch.c
index 3daf1c3eda16dda19a3ed16f5a9f80023cc724a8..72b6d5d6d6c4596092c233eca2b65528e109afa7 100644 (file)
--- a/servers/lloadd/epoch.c
+++ b/servers/lloadd/epoch.c
@@ -289,7 +289,11 @@ acquire_ref( uintptr_t *refp )
 }
 
 int
-try_release_ref( uintptr_t *refp, void *object, dispose_cb *cb )
+try_release_ref(
+        uintptr_t *refp,
+        void *object,
+        dispose_cb *unlink_cb,
+        dispose_cb *destroy_cb )
 {
     uintptr_t refcnt, new_refcnt;
 
@@ -307,7 +311,10 @@ try_release_ref( uintptr_t *refp, void *object, dispose_cb *cb )
     assert( new_refcnt == refcnt - 1 );
 
     if ( !new_refcnt ) {
-        epoch_append( object, cb );
+        if ( unlink_cb ) {
+            unlink_cb( object );
+        }
+        epoch_append( object, destroy_cb );
     }
 
     return refcnt;

diff --git a/servers/lloadd/epoch.h b/servers/lloadd/epoch.h
index c552ef008999327c4cafb08b3aac6f38a7ef10af..06b70be73637059420883e7d74c1331af04af853 100644 (file)
--- a/servers/lloadd/epoch.h
+++ b/servers/lloadd/epoch.h
@@ -108,7 +108,11 @@ int acquire_ref( uintptr_t *refp );
  * @return 0 if reference was already zero, non-zero if reference
  * count was non-zero at the time of call
  */
-int try_release_ref( uintptr_t *refp, void *object, dispose_cb *cb );
+int try_release_ref(
+        uintptr_t *refp,
+        void *object,
+        dispose_cb *unlink_cb,
+        dispose_cb *destroy_cb );
 
 /** @brief Read reference count
  *

diff --git a/servers/lloadd/extended.c b/servers/lloadd/extended.c
index c49623e85a4f1918d0eb5aa154a05fa6dbbbaf44..54d97005f5bf14995e48a22f5d662fcf85ca3ab7 100644 (file)
--- a/servers/lloadd/extended.c
+++ b/servers/lloadd/extended.c
@@ -90,7 +90,7 @@ handle_starttls( LloadConnection *c, LloadOperation *op )
     output = c->c_pendingber;
     if ( output == NULL && (output = ber_alloc()) == NULL ) {
         checked_unlock( &c->c_io_mutex );
-        operation_unlink( op );
+        OPERATION_UNLINK(op);
         CONNECTION_LOCK_DESTROY(c);
         return -1;
     }
@@ -115,7 +115,7 @@ handle_starttls( LloadConnection *c, LloadOperation *op )
     op->o_res = LLOAD_OP_COMPLETED;
     CONNECTION_UNLOCK(c);
 
-    operation_unlink( op );
+    OPERATION_UNLINK(op);
 
     return -1;
 #endif /* HAVE_TLS */

diff --git a/servers/lloadd/lload.h b/servers/lloadd/lload.h
index 85febe4fd18dc6e655968593c0daf34272ae8c7d..f7d3bba488095ff72fa0d40b734e440a3f3c9a2a 100644 (file)
--- a/servers/lloadd/lload.h
+++ b/servers/lloadd/lload.h
@@ -526,11 +526,15 @@ enum op_result {
 /*
  * Operation reference tracking:
  * - o_refcnt is set to 1, never incremented
- * - operation_unlink sets it to 0 and on transition from 1 clears both
+ * - OPERATION_UNLINK sets it to 0 and on transition from 1 clears both
  *   connection links (o_client, o_upstream)
  */
 struct LloadOperation {
     uintptr_t o_refcnt;
+#define OPERATION_UNLINK(op) \
+    try_release_ref( &(op)->o_refcnt, (op), \
+            (dispose_cb *)operation_unlink, \
+            (dispose_cb *)operation_destroy )
 
     LloadConnection *o_client;
     unsigned long o_client_connid;

diff --git a/servers/lloadd/operation.c b/servers/lloadd/operation.c
index 0f875bb8ebfb63cf937e75ab934f03524d7c50b4..73f91a1eb3709413c4b6904d43ad38273dc38028 100644 (file)
--- a/servers/lloadd/operation.c
+++ b/servers/lloadd/operation.c
@@ -234,12 +234,7 @@ operation_unlink( LloadOperation *op )
     uintptr_t prev_refcnt;
     int result = 0;
 
-    if ( !( prev_refcnt = try_release_ref(
-                    &op->o_refcnt, op, (dispose_cb *)operation_destroy ) ) ) {
-        return result;
-    }
-
-    assert( prev_refcnt == 1 );
+    assert( op->o_refcnt == 0 );
 
     Debug( LDAP_DEBUG_TRACE, "operation_unlink: "
             "unlinking operation between client connid=%lu and upstream "
@@ -458,7 +453,7 @@ operation_abandon( LloadOperation *op )
     }
 
 done:
-    operation_unlink( op );
+    OPERATION_UNLINK(op);
 }
 
 void
@@ -525,7 +520,7 @@ operation_send_reject(
     connection_write_cb( -1, 0, c );
 
 done:
-    operation_unlink( op );
+    OPERATION_UNLINK(op);
 }
 
 /*
@@ -619,7 +614,7 @@ connection_timeout( LloadConnection *upstream, void *arg )
         if ( upstream->c_type != LLOAD_C_BIND && rc == LDAP_SUCCESS ) {
             rc = operation_send_abandon( op, upstream );
         }
-        operation_unlink( op );
+        OPERATION_UNLINK(op);
     }
 
     if ( rc == LDAP_SUCCESS ) {

diff --git a/servers/lloadd/upstream.c b/servers/lloadd/upstream.c
index 4d3419db8da18d590cbac91112081484b9b5a5da..771858dff093287e2151df3a67eb74fedde6a4aa 100644 (file)
--- a/servers/lloadd/upstream.c
+++ b/servers/lloadd/upstream.c
@@ -129,7 +129,7 @@ forward_final_response(
 
     op->o_res = LLOAD_OP_COMPLETED;
     if ( !op->o_pin_id ) {
-        operation_unlink( op );
+        OPERATION_UNLINK(op);
     }
 
     return rc;