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 2335ccefd1acc76277e03b449894d3a3dc36ed42..f03e43b08ad1cdd627b6f7d7f0939d5a2577f8f0 100644 (file)
--- a/servers/lloadd/bind.c
+++ b/servers/lloadd/bind.c
@@ -406,7 +406,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;
@@ -985,7 +985,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 a71626096999b79fc046ff8fa99cd33650196cd6..32ef79d72b793d2f0e05270308f9a9e3360adcca 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;
 }
 
@@ -201,7 +201,7 @@ fail:
         operation_send_reject( op, LDAP_OTHER, "internal error", 0 );
     }
 
-    operation_unlink( op );
+    OPERATION_UNLINK(op);
     if ( rc ) {
         CONNECTION_LOCK_DESTROY(client);
     }
@@ -250,7 +250,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 967c6c5b3dde4c7251bef32123378f730b2202d0..4f49d533b6f9514fc450a59405c8316d04273e8b 100644 (file)
--- a/servers/lloadd/connection.c
+++ b/servers/lloadd/connection.c
@@ -545,7 +545,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 43ea5896b7d4642cfaa887b342ab5401eec99091..80afb851576817d385f0aa1616d008db71680da7 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 a9b98e03df377ec8fbf5ddade8acc87745416cc9..40ddadf37f6191f1c5938c8d533aa634dc84a3cc 100644 (file)
--- a/servers/lloadd/lload.h
+++ b/servers/lloadd/lload.h
@@ -431,11 +431,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 907440439643160881c69c9af54256d87b76df4c..0a42a71466e42aa5dd578167af3a8086039314db 100644 (file)
--- a/servers/lloadd/operation.c
+++ b/servers/lloadd/operation.c
@@ -223,12 +223,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 "
@@ -432,7 +427,7 @@ operation_abandon( LloadOperation *op )
     }
 
 done:
-    operation_unlink( op );
+    OPERATION_UNLINK(op);
 }
 
 void
@@ -499,7 +494,7 @@ operation_send_reject(
     connection_write_cb( -1, 0, c );
 
 done:
-    operation_unlink( op );
+    OPERATION_UNLINK(op);
 }
 
 /*
@@ -592,7 +587,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 e85d6da248beded7d4b426a4592340a8a3415901..6824fa8a8ad46393581b6c6ab8ad31ba3802275e 100644 (file)
--- a/servers/lloadd/upstream.c
+++ b/servers/lloadd/upstream.c
@@ -108,7 +108,7 @@ forward_final_response(
 
     op->o_res = LLOAD_OP_COMPLETED;
     if ( !op->o_pin_id ) {
-        operation_unlink( op );
+        OPERATION_UNLINK(op);
     }
 
     return rc;