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.

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;
 }

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",

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 );
 

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;

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
  *

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 */

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;

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 ) {

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;