From 05d4affb7296e157581880544fd0fb49b9d98325 Mon Sep 17 00:00:00 2001
From: Matthew Swift <matthew.swift@forgerock.com>
Date: Thu, 22 Sep 2011 15:51:27 +0000
Subject: [PATCH] Fix OPENDJ-293: InternalClientConnection memory leak when performing password modify/state extended operations or SASL binds
---
opends/src/server/org/opends/server/api/ClientConnection.java | 55 +++----------
opends/tests/unit-tests-testng/src/server/org/opends/server/types/PrivilegeTestCase.java | 107 ++++++++++++++++----------
opends/tests/unit-tests-testng/src/server/org/opends/server/core/GroupManagerTestCase.java | 3
opends/src/server/org/opends/server/protocols/internal/InternalClientConnection.java | 37 ++++-----
opends/src/server/org/opends/server/core/AuthenticatedUsers.java | 2
5 files changed, 97 insertions(+), 107 deletions(-)
diff --git a/opends/src/server/org/opends/server/api/ClientConnection.java b/opends/src/server/org/opends/server/api/ClientConnection.java
index 70dab74..3fe256f 100644
--- a/opends/src/server/org/opends/server/api/ClientConnection.java
+++ b/opends/src/server/org/opends/server/api/ClientConnection.java
@@ -23,6 +23,7 @@
*
*
* Copyright 2006-2009 Sun Microsystems, Inc.
+ * Portions copyright 2011 ForgeRock AS
*/
package org.opends.server.api;
@@ -89,8 +90,10 @@
*/
private static final DebugTracer TRACER = getTracer();
- // The set of authentication information for this client connection.
- private AuthenticationInfo authenticationInfo;
+ /**
+ * The set of authentication information for this client connection.
+ */
+ protected AuthenticationInfo authenticationInfo;
/**
* Indicates whether a multistage SASL bind is currently in progress
@@ -185,8 +188,7 @@
/**
* Performs any internal cleanup that may be necessary when this
- * client connection is disconnected, or if not on disconnec, then
- * ultimately whenever it is reaped by the garbage collector. In
+ * client connection is disconnected. In
* this case, it will be used to ensure that the connection is
* deregistered with the {@code AuthenticatedUsers} manager, and
* will then invoke the {@code finalizeClientConnection} method.
@@ -234,39 +236,6 @@
}
networkGroup.removeConnection(this);
-
- try
- {
- finalizeClientConnection();
- }
- catch (Exception e)
- {
- if (debugEnabled())
- {
- TRACER.debugCaught(DebugLogLevel.ERROR, e);
- }
- }
- }
-
-
-
- /**
- * Performs any cleanup work that may be necessary when this client
- * connection is terminated. By default, no action is taken.
- * <BR><BR>
- * If possible, this method will be invoked when the client
- * connection is disconnected. If it isn't invoked at that time,
- * then it will be called when the client connection object is
- * finalized by the garbage collector.
- */
- @org.opends.server.types.PublicAPI(
- stability=org.opends.server.types.StabilityLevel.VOLATILE,
- mayInstantiate=false,
- mayExtend=true,
- mayInvoke=false)
- protected void finalizeClientConnection()
- {
- // No implementation is required by default.
}
@@ -1316,7 +1285,7 @@
* user and should automatically inherit the root
* privilege set.
*/
- private void updatePrivileges(Entry entry, boolean isRoot)
+ protected void updatePrivileges(Entry entry, boolean isRoot)
{
privileges = getPrivileges(entry, isRoot);
}
@@ -1592,7 +1561,7 @@
* @throws DirectoryException If a problem occurs while attempting
* to make the determination.
*/
- public Set<Group> getGroups(Operation operation)
+ public Set<Group<?>> getGroups(Operation operation)
throws DirectoryException
{
// FIXME -- This probably isn't the most efficient implementation.
@@ -1616,17 +1585,17 @@
if ((authzDN == null) || authzDN.isNullDN())
{
- return Collections.<Group>emptySet();
+ return Collections.<Group<?>>emptySet();
}
Entry userEntry = DirectoryServer.getEntry(authzDN);
if (userEntry == null)
{
- return Collections.<Group>emptySet();
+ return Collections.<Group<?>>emptySet();
}
- HashSet<Group> groupSet = new HashSet<Group>();
- for (Group g :
+ HashSet<Group<?>> groupSet = new HashSet<Group<?>>();
+ for (Group<?> g :
DirectoryServer.getGroupManager().getGroupInstances())
{
if (g.isMember(userEntry))
diff --git a/opends/src/server/org/opends/server/core/AuthenticatedUsers.java b/opends/src/server/org/opends/server/core/AuthenticatedUsers.java
index 41f484b..eda392e 100644
--- a/opends/src/server/org/opends/server/core/AuthenticatedUsers.java
+++ b/opends/src/server/org/opends/server/core/AuthenticatedUsers.java
@@ -167,7 +167,7 @@
* @return The set of client connections authenticated as the specified user,
* or {@code null} if there are none.
*/
- synchronized CopyOnWriteArraySet<ClientConnection> get(DN userDN)
+ public CopyOnWriteArraySet<ClientConnection> get(DN userDN)
{
lock.readLock().lock();
try
diff --git a/opends/src/server/org/opends/server/protocols/internal/InternalClientConnection.java b/opends/src/server/org/opends/server/protocols/internal/InternalClientConnection.java
index 35d6ff4..65c9057 100644
--- a/opends/src/server/org/opends/server/protocols/internal/InternalClientConnection.java
+++ b/opends/src/server/org/opends/server/protocols/internal/InternalClientConnection.java
@@ -23,6 +23,7 @@
*
*
* Copyright 2006-2009 Sun Microsystems, Inc.
+ * Portions copyright 2011 ForgeRock AS
*/
package org.opends.server.protocols.internal;
@@ -99,9 +100,6 @@
// The static connection for root-based connections.
private static InternalClientConnection rootConnection;
- // The authentication info for this connection.
- private AuthenticationInfo authenticationInfo;
-
// The empty operation list for this connection.
private LinkedList<Operation> operationList;
@@ -236,11 +234,23 @@
{
super();
-
this.setNetworkGroup(NetworkGroup.getInternalNetworkGroup());
- this.authenticationInfo = authInfo;
- super.setAuthenticationInfo(authInfo);
+ // Don't call super.setAuthenticationInfo() since this will register this
+ // connection in the authenticated users table, which is unnecessary and
+ // will also cause the connection to be leaked since internal connections
+ // are never closed/disconnected.
+ if (authInfo == null)
+ {
+ this.authenticationInfo = new AuthenticationInfo();
+ updatePrivileges(null, false);
+ }
+ else
+ {
+ this.authenticationInfo = authInfo;
+ updatePrivileges(authInfo.getAuthorizationEntry(), authInfo.isRoot());
+ }
+
super.setSizeLimit(0);
super.setTimeLimit(0);
super.setIdleTimeLimit(0);
@@ -655,21 +665,6 @@
/**
- * Retrieves information about the authentication that has been
- * performed for this connection.
- *
- * @return Information about the user that is currently
- * authenticated on this connection.
- */
- @Override()
- public AuthenticationInfo getAuthenticationInfo()
- {
- return authenticationInfo;
- }
-
-
-
- /**
* This method has no effect, as the authentication info for
* internal client connections is set when the connection is created
* and cannot be changed after the fact.
diff --git a/opends/tests/unit-tests-testng/src/server/org/opends/server/core/GroupManagerTestCase.java b/opends/tests/unit-tests-testng/src/server/org/opends/server/core/GroupManagerTestCase.java
index b8856bd..44c03f0 100644
--- a/opends/tests/unit-tests-testng/src/server/org/opends/server/core/GroupManagerTestCase.java
+++ b/opends/tests/unit-tests-testng/src/server/org/opends/server/core/GroupManagerTestCase.java
@@ -23,6 +23,7 @@
*
*
* Copyright 2008-2010 Sun Microsystems, Inc.
+ * Portions copyright 2011 ForgeRock AS
*/
package org.opends.server.core;
@@ -1430,7 +1431,7 @@
assertFalse(conn0.isMemberOf(group2, searchOperation));
assertFalse(conn0.isMemberOf(group3, searchOperation));
- Set<Group> groupSet = conn0.getGroups(null);
+ Set<Group<?>> groupSet = conn0.getGroups(null);
assertTrue(groupSet.isEmpty());
groupSet = conn0.getGroups(searchOperation);
diff --git a/opends/tests/unit-tests-testng/src/server/org/opends/server/types/PrivilegeTestCase.java b/opends/tests/unit-tests-testng/src/server/org/opends/server/types/PrivilegeTestCase.java
index 88e6186..bf4315b 100644
--- a/opends/tests/unit-tests-testng/src/server/org/opends/server/types/PrivilegeTestCase.java
+++ b/opends/tests/unit-tests-testng/src/server/org/opends/server/types/PrivilegeTestCase.java
@@ -37,13 +37,16 @@
import java.io.BufferedWriter;
import java.io.File;
import java.io.FileWriter;
+import java.net.Socket;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.UUID;
+import java.util.concurrent.CopyOnWriteArraySet;
import org.opends.server.TestCaseUtils;
import org.opends.server.admin.std.meta.GlobalCfgDefn;
import org.opends.server.admin.std.meta.RootDNCfgDefn;
+import org.opends.server.api.ClientConnection;
import org.opends.server.backends.task.Task;
import org.opends.server.backends.task.TaskBackend;
import org.opends.server.backends.task.TaskState;
@@ -63,9 +66,10 @@
import org.opends.server.core.SchemaConfigManager;
import org.opends.server.protocols.internal.InternalClientConnection;
import org.opends.server.protocols.internal.InternalSearchOperation;
-import org.opends.server.tools.LDAPModify;
-import org.opends.server.tools.LDAPPasswordModify;
-import org.opends.server.tools.LDAPSearch;
+import org.opends.server.protocols.ldap.BindRequestProtocolOp;
+import org.opends.server.protocols.ldap.BindResponseProtocolOp;
+import org.opends.server.protocols.ldap.LDAPMessage;
+import org.opends.server.tools.*;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.DataProvider;
@@ -115,7 +119,7 @@
{
TestCaseUtils.startServer();
TestCaseUtils.enableBackend("unindexedRoot");
-
+
TestCaseUtils.dsconfig(
"set-sasl-mechanism-handler-prop",
"--handler-name", "DIGEST-MD5",
@@ -2563,45 +2567,66 @@
"sn: User",
"userPassword: password");
- Entry testEntry =
- DirectoryServer.getEntry(DN.decode("cn=Test User,o=test"));
- AuthenticationInfo authInfo = new AuthenticationInfo(testEntry, false);
- InternalClientConnection testConnection =
- new InternalClientConnection(authInfo);
-
-
- // Make sure the user starts out without any privileges.
- for (Privilege p : Privilege.values())
+ // We won't use an internal connection here because these are not notified
+ // of dynamic changes to authentication info.
+ Socket s = new Socket("127.0.0.1", TestCaseUtils.getServerLdapPort());
+ try
{
- assertFalse(testConnection.hasPrivilege(p, null));
+ TestCaseUtils.configureSocket(s);
+ LDAPReader r = new LDAPReader(s);
+ LDAPWriter w = new LDAPWriter(s);
+
+ BindRequestProtocolOp bindRequest = new BindRequestProtocolOp(
+ ByteString.valueOf("cn=Test User,o=test"), 3,
+ ByteString.valueOf("password"));
+ LDAPMessage message = new LDAPMessage(1, bindRequest);
+ w.writeMessage(message);
+
+ message = r.readMessage();
+ BindResponseProtocolOp bindResponse = message.getBindResponseProtocolOp();
+ assertEquals(bindResponse.getResultCode(), 0);
+
+ CopyOnWriteArraySet<ClientConnection> connections = DirectoryServer
+ .getAuthenticatedUsers().get(DN.decode("cn=Test User,o=test"));
+
+ assertNotNull(connections);
+ assertEquals(connections.size(), 1);
+ ClientConnection testConnection = connections.iterator().next();
+
+ // Make sure the user starts out without any privileges.
+ for (Privilege p : Privilege.values())
+ {
+ assertFalse(testConnection.hasPrivilege(p, null));
+ }
+
+ // Modify the user entry to add the CONFIG_READ privilege and verify that
+ // the client connection reflects that.
+ ArrayList<Modification> mods = new ArrayList<Modification>();
+ mods.add(new Modification(ModificationType.ADD, Attributes.create(
+ "ds-privilege-name", "config-read")));
+ ModifyOperation modifyOperation = rootConnection.processModify(
+ DN.decode("cn=Test User,o=test"), mods);
+ assertEquals(modifyOperation.getResultCode(), ResultCode.SUCCESS);
+ assertTrue(testConnection.hasPrivilege(Privilege.CONFIG_READ, null));
+
+ // Take the privilege away from the user and verify that it is recognized
+ // immediately.
+ mods.clear();
+ mods.add(new Modification(ModificationType.DELETE, Attributes.create(
+ "ds-privilege-name", "config-read")));
+ modifyOperation = rootConnection.processModify(
+ DN.decode("cn=Test User,o=test"), mods);
+ assertEquals(modifyOperation.getResultCode(), ResultCode.SUCCESS);
+ assertFalse(testConnection.hasPrivilege(Privilege.CONFIG_READ, null));
+
+ DeleteOperation deleteOperation = rootConnection.processDelete(DN
+ .decode("cn=Test User,o=test"));
+ assertEquals(deleteOperation.getResultCode(), ResultCode.SUCCESS);
}
-
-
- // Modify the user entry to add the CONFIG_READ privilege and verify that
- // the client connection reflects that.
- ArrayList<Modification> mods = new ArrayList<Modification>();
- mods.add(new Modification(ModificationType.ADD,
- Attributes.create("ds-privilege-name", "config-read")));
- ModifyOperation modifyOperation =
- rootConnection.processModify(DN.decode("cn=Test User,o=test"), mods);
- assertEquals(modifyOperation.getResultCode(), ResultCode.SUCCESS);
- assertTrue(testConnection.hasPrivilege(Privilege.CONFIG_READ, null));
-
-
- // Take the privilege away from the user and verify that it is recognized
- // immediately.
- mods.clear();
- mods.add(new Modification(ModificationType.DELETE,
- Attributes.create("ds-privilege-name", "config-read")));
- modifyOperation =
- rootConnection.processModify(DN.decode("cn=Test User,o=test"), mods);
- assertEquals(modifyOperation.getResultCode(), ResultCode.SUCCESS);
- assertFalse(testConnection.hasPrivilege(Privilege.CONFIG_READ, null));
-
-
- DeleteOperation deleteOperation =
- rootConnection.processDelete(DN.decode("cn=Test User,o=test"));
- assertEquals(deleteOperation.getResultCode(), ResultCode.SUCCESS);
+ finally
+ {
+ s.close();
+ }
}
--
Gitblit v1.10.0