From: Ondřej Kuzník <ondra@mistotebe.net>
Date: Wed, 18 Jan 2023 13:03:23 +0000 (+0000)
Subject: ITS#9983 Rework op->o_refcnt decref sequencing
X-Git-Tag: OPENLDAP_REL_ENG_2_5_14~16
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1dc911bd204f448d093a3e8793913dc26106bdf2;p=thirdparty%2Fopenldap.git

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 2335ccefd1..f03e43b08a 100644
--- 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 a716260969..32ef79d72b 100644
--- 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 967c6c5b3d..4f49d533b6 100644
--- 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 3daf1c3eda..72b6d5d6d6 100644
--- 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 c552ef0089..06b70be736 100644
--- 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 43ea5896b7..80afb85157 100644
--- 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 a9b98e03df..40ddadf37f 100644
--- 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 9074404396..0a42a71466 100644
--- 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 e85d6da248..6824fa8a8a 100644
--- 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;