|
 |
b663b9 |
From 1f3e1ad55f72a885e27db41be28ce1037ff0ce93 Mon Sep 17 00:00:00 2001
|
|
|
b663b9 |
From: Thierry Bordaz <tbordaz@redhat.com>
|
|
|
b663b9 |
Date: Fri, 1 Jun 2018 16:12:40 +0200
|
|
|
b663b9 |
Subject: [PATCH] Ticket 49736 - Hardening of active connection list
|
|
|
b663b9 |
|
|
|
b663b9 |
Bug Description:
|
|
|
b663b9 |
In case of a bug in the management of the connection refcnt
|
|
|
b663b9 |
it can happen that there are several attempts to move a connection
|
|
|
b663b9 |
out of the active list.
|
|
|
b663b9 |
|
|
|
b663b9 |
It triggers a crash because when derefencing c->c_prev.
|
|
|
b663b9 |
c_prev is never NULL on the active list
|
|
|
b663b9 |
|
|
|
b663b9 |
Fix Description:
|
|
|
b663b9 |
The fix tests if the connection is already out of the active list.
|
|
|
b663b9 |
If such case, it just returns.
|
|
|
b663b9 |
|
|
|
b663b9 |
A potential issue that is not addressed by this fix is:
|
|
|
b663b9 |
Thread A and Thread B are using 'c' but c->refcnt=1 (it should be 2)
|
|
|
b663b9 |
Thread A "closes" 'c', 'c' is move out of active list (free) because of refcnt=0
|
|
|
b663b9 |
A new connection happens selecting the free connection 'c', moving it to the active list.
|
|
|
b663b9 |
Thread C is using 'c' from the new connection c->refcnt=1
|
|
|
b663b9 |
Thread B "closes" 'c', 'c' is moved out of the active list.
|
|
|
b663b9 |
-> new operation coming on 'c' will not be detected
|
|
|
b663b9 |
-> Thread C will likely crash when sending result
|
|
|
b663b9 |
|
|
|
b663b9 |
https://pagure.io/389-ds-base/issue/49736
|
|
|
b663b9 |
|
|
|
b663b9 |
Reviewed by: Mark Reynolds (thanks!)
|
|
|
b663b9 |
|
|
|
b663b9 |
Platforms tested: F26
|
|
|
b663b9 |
|
|
|
b663b9 |
Flag Day: no
|
|
|
b663b9 |
|
|
|
b663b9 |
Doc impact: no
|
|
|
b663b9 |
|
|
|
b663b9 |
(cherry picked from commit b0e05806232b781eed3ff102485045a358d7659b)
|
|
|
b663b9 |
---
|
|
|
b663b9 |
ldap/servers/slapd/conntable.c | 21 +++++++++++++++++++++
|
|
|
b663b9 |
1 file changed, 21 insertions(+)
|
|
|
b663b9 |
|
|
|
b663b9 |
diff --git a/ldap/servers/slapd/conntable.c b/ldap/servers/slapd/conntable.c
|
|
|
b663b9 |
index 114871d17..cb68a1119 100644
|
|
|
b663b9 |
--- a/ldap/servers/slapd/conntable.c
|
|
|
b663b9 |
+++ b/ldap/servers/slapd/conntable.c
|
|
|
b663b9 |
@@ -243,6 +243,27 @@ connection_table_move_connection_out_of_active_list(Connection_Table *ct, Connec
|
|
|
b663b9 |
int c_sd; /* for logging */
|
|
|
b663b9 |
/* we always have previous element because list contains a dummy header */;
|
|
|
b663b9 |
PR_ASSERT(c->c_prev);
|
|
|
b663b9 |
+ if (c->c_prev == NULL) {
|
|
|
b663b9 |
+ /* c->c_prev is set when the connection is moved ON the active list
|
|
|
b663b9 |
+ * So this connection is already OUT of the active list
|
|
|
b663b9 |
+ *
|
|
|
b663b9 |
+ * Not sure how to recover from here.
|
|
|
b663b9 |
+ * Considering c->c_prev is NULL we can assume refcnt is now 0
|
|
|
b663b9 |
+ * and connection_cleanup was already called.
|
|
|
b663b9 |
+ * If it is not the case, then consequences are:
|
|
|
b663b9 |
+ * - Leak some memory (connext, unsent page result entries, various buffers)
|
|
|
b663b9 |
+ * - hanging connection (fd not closed)
|
|
|
b663b9 |
+ * A option would be to call connection_cleanup here.
|
|
|
b663b9 |
+ *
|
|
|
b663b9 |
+ * The logged message helps to know how frequently the problem exists
|
|
|
b663b9 |
+ */
|
|
|
b663b9 |
+ slapi_log_err(SLAPI_LOG_CRIT,
|
|
|
b663b9 |
+ "connection_table_move_connection_out_of_active_list",
|
|
|
b663b9 |
+ "conn %d is already OUT of the active list (refcnt is %d)\n",
|
|
|
b663b9 |
+ c->c_sd, c->c_refcnt);
|
|
|
b663b9 |
+
|
|
|
b663b9 |
+ return 0;
|
|
|
b663b9 |
+ }
|
|
|
b663b9 |
|
|
|
b663b9 |
#ifdef FOR_DEBUGGING
|
|
|
b663b9 |
slapi_log_err(SLAPI_LOG_DEBUG, "connection_table_move_connection_out_of_active_list", "Moving connection out of active list\n");
|
|
|
b663b9 |
--
|
|
|
b663b9 |
2.17.0
|
|
|
b663b9 |
|