From be19e9b1eb448fd3b6d2e7abbc357bcab791d3b4 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Fri, 14 Sep 2018 08:41:02 +0000 Subject: [PATCH] Review thread-safety Document locking strategy Fix a few issues: - Iterators could throw ConcurrentModificationException - Syncs on open/save didn't lock roles Map Update with a view to supporting runtime reloading (BZ 58590) git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1840901 13f79535-47bb-0310-9956-ffa450edef68 --- .../catalina/users/MemoryUserDatabase.java | 218 ++++++++++++------ 1 file changed, 145 insertions(+), 73 deletions(-) diff --git a/java/org/apache/catalina/users/MemoryUserDatabase.java b/java/org/apache/catalina/users/MemoryUserDatabase.java index ba7bc1cda85f..01c98943973c 100644 --- a/java/org/apache/catalina/users/MemoryUserDatabase.java +++ b/java/org/apache/catalina/users/MemoryUserDatabase.java @@ -22,8 +22,12 @@ import java.io.InputStream; import java.io.OutputStreamWriter; import java.io.PrintWriter; -import java.util.HashMap; +import java.util.ArrayList; import java.util.Iterator; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import org.apache.catalina.Globals; import org.apache.catalina.Group; @@ -42,10 +46,34 @@ * Concrete implementation of {@link UserDatabase} that loads all defined users, * groups, and roles into an in-memory data structure, and uses a specified XML * file for its persistent storage. + *

+ * This class is thread-safe. + *

+ * This class does not enforce what, in an RDBMS, would be called referential + * integrity. Concurrent modifications may result in inconsistent data such as + * a User retaining a reference to a Role that has been removed from the + * database. * * @author Craig R. McClanahan * @since 4.1 */ +/* + * Implementation notes: + * + * Any operation that acts on a single element of the database (e.g. operations + * that create, read, update or delete a user, role or group) must first obtain + * the read lock. Operations that return iterators for users, roles or groups + * also fall into this category. + * + * Iterators must always be created from copies of the data to prevent possible + * corruption of the iterator due to the remove of all elements from the + * underlying Map that would occur during a subsequent re-loading of the + * database. + * + * Any operation that acts on multiple elements and expects the database to + * remain consistent during the operation (e.g. saving or loading the database) + * must first obtain the write lock. + */ public class MemoryUserDatabase implements UserDatabase { private static final Log log = LogFactory.getLog(MemoryUserDatabase.class); @@ -76,7 +104,7 @@ public MemoryUserDatabase(String id) { /** * The set of {@link Group}s defined in this database, keyed by group name. */ - protected final HashMap groups = new HashMap<>(); + protected final Map groups = new ConcurrentHashMap<>(); /** * The unique global identifier of this user database. @@ -109,12 +137,16 @@ public MemoryUserDatabase(String id) { /** * The set of {@link Role}s defined in this database, keyed by role name. */ - protected final HashMap roles = new HashMap<>(); + protected final Map roles = new ConcurrentHashMap<>(); /** * The set of {@link User}s defined in this database, keyed by user name. */ - protected final HashMap users = new HashMap<>(); + protected final Map users = new ConcurrentHashMap<>(); + + private final ReentrantReadWriteLock dbLock = new ReentrantReadWriteLock(); + private final Lock readLock = dbLock.readLock(); + private final Lock writeLock = dbLock.writeLock(); // ------------------------------------------------------------- Properties @@ -124,8 +156,11 @@ public MemoryUserDatabase(String id) { */ @Override public Iterator getGroups() { - synchronized (groups) { - return groups.values().iterator(); + readLock.lock(); + try { + return new ArrayList<>(groups.values()).iterator(); + } finally { + readLock.unlock(); } } @@ -182,8 +217,11 @@ public void setReadonly(boolean readonly) { */ @Override public Iterator getRoles() { - synchronized (roles) { - return roles.values().iterator(); + readLock.lock(); + try { + return new ArrayList<>(roles.values()).iterator(); + } finally { + readLock.unlock(); } } @@ -193,8 +231,11 @@ public Iterator getRoles() { */ @Override public Iterator getUsers() { - synchronized (users) { - return users.values().iterator(); + readLock.lock(); + try { + return new ArrayList<>(users.values()).iterator(); + } finally { + readLock.unlock(); } } @@ -209,13 +250,14 @@ public Iterator getUsers() { @Override public void close() throws Exception { - save(); - - synchronized (groups) { - synchronized (users) { - users.clear(); - groups.clear(); - } + writeLock.lock(); + try { + save(); + users.clear(); + groups.clear(); + roles.clear(); + } finally { + writeLock.unlock(); } } @@ -235,8 +277,11 @@ public Group createGroup(String groupname, String description) { } MemoryGroup group = new MemoryGroup(this, groupname, description); - synchronized (groups) { + readLock.lock(); + try { groups.put(group.getGroupname(), group); + } finally { + readLock.unlock(); } return group; } @@ -257,8 +302,11 @@ public Role createRole(String rolename, String description) { } MemoryRole role = new MemoryRole(this, rolename, description); - synchronized (roles) { + readLock.lock(); + try { roles.put(role.getRolename(), role); + } finally { + readLock.unlock(); } return role; } @@ -281,8 +329,11 @@ public User createUser(String username, String password, String fullName) { } MemoryUser user = new MemoryUser(this, username, password, fullName); - synchronized (users) { + readLock.unlock(); + try { users.put(user.getUsername(), user); + } finally { + readLock.unlock(); } return user; } @@ -296,8 +347,11 @@ public User createUser(String username, String password, String fullName) { */ @Override public Group findGroup(String groupname) { - synchronized (groups) { + readLock.lock(); + try { return groups.get(groupname); + } finally { + readLock.unlock(); } } @@ -310,8 +364,11 @@ public Group findGroup(String groupname) { */ @Override public Role findRole(String rolename) { - synchronized (roles) { + readLock.lock(); + try { return roles.get(rolename); + } finally { + readLock.unlock(); } } @@ -324,8 +381,11 @@ public Role findRole(String rolename) { */ @Override public User findUser(String username) { - synchronized (users) { + readLock.lock(); + try { return users.get(username); + } finally { + readLock.unlock(); } } @@ -338,37 +398,37 @@ public User findUser(String username) { @Override public void open() throws Exception { - synchronized (groups) { - synchronized (users) { - - // Erase any previous groups and users - users.clear(); - groups.clear(); - roles.clear(); - - String pathName = getPathname(); - try (InputStream is = ConfigFileLoader.getInputStream(getPathname())) { - // Construct a digester to read the XML input file - Digester digester = new Digester(); - try { - digester.setFeature( - "http://apache.org/xml/features/allow-java-encodings", true); - } catch (Exception e) { - log.warn(sm.getString("memoryUserDatabase.xmlFeatureEncoding"), e); - } - digester.addFactoryCreate("tomcat-users/group", - new MemoryGroupCreationFactory(this), true); - digester.addFactoryCreate("tomcat-users/role", - new MemoryRoleCreationFactory(this), true); - digester.addFactoryCreate("tomcat-users/user", - new MemoryUserCreationFactory(this), true); - - // Parse the XML input to load this database - digester.parse(is); - } catch (IOException ioe) { - log.error(sm.getString("memoryUserDatabase.fileNotFound", pathName)); + writeLock.lock(); + try { + // Erase any previous groups and users + users.clear(); + groups.clear(); + roles.clear(); + + String pathName = getPathname(); + try (InputStream is = ConfigFileLoader.getInputStream(getPathname())) { + // Construct a digester to read the XML input file + Digester digester = new Digester(); + try { + digester.setFeature( + "http://apache.org/xml/features/allow-java-encodings", true); + } catch (Exception e) { + log.warn(sm.getString("memoryUserDatabase.xmlFeatureEncoding"), e); } + digester.addFactoryCreate("tomcat-users/group", + new MemoryGroupCreationFactory(this), true); + digester.addFactoryCreate("tomcat-users/role", + new MemoryRoleCreationFactory(this), true); + digester.addFactoryCreate("tomcat-users/user", + new MemoryUserCreationFactory(this), true); + + // Parse the XML input to load this database + digester.parse(is); + } catch (IOException ioe) { + log.error(sm.getString("memoryUserDatabase.fileNotFound", pathName)); } + } finally { + writeLock.unlock(); } } @@ -380,14 +440,16 @@ public void open() throws Exception { */ @Override public void removeGroup(Group group) { - - synchronized (groups) { + readLock.lock(); + try { Iterator users = getUsers(); while (users.hasNext()) { User user = users.next(); user.removeGroup(group); } groups.remove(group.getGroupname()); + } finally { + readLock.unlock(); } } @@ -399,8 +461,8 @@ public void removeGroup(Group group) { */ @Override public void removeRole(Role role) { - - synchronized (roles) { + readLock.lock(); + try { Iterator groups = getGroups(); while (groups.hasNext()) { Group group = groups.next(); @@ -412,6 +474,8 @@ public void removeRole(Role role) { user.removeRole(role); } roles.remove(role.getRolename()); + } finally { + readLock.unlock(); } } @@ -423,8 +487,11 @@ public void removeRole(Role role) { */ @Override public void removeUser(User user) { - synchronized (users) { + readLock.lock(); + try { users.remove(user.getUsername()); + } finally { + readLock.unlock(); } } @@ -484,22 +551,27 @@ public void save() throws Exception { writer.println("xsi:schemaLocation=\"http://tomcat.apache.org/xml tomcat-users.xsd\""); writer.println(" version=\"1.0\">"); - // Print entries for each defined role, group, and user - Iterator values = null; - values = getRoles(); - while (values.hasNext()) { - writer.print(" "); - writer.println(values.next()); - } - values = getGroups(); - while (values.hasNext()) { - writer.print(" "); - writer.println(values.next()); - } - values = getUsers(); - while (values.hasNext()) { - writer.print(" "); - writer.println(((MemoryUser) values.next()).toXml()); + writeLock.lock(); + try { + // Print entries for each defined role, group, and user + Iterator values = null; + values = getRoles(); + while (values.hasNext()) { + writer.print(" "); + writer.println(values.next()); + } + values = getGroups(); + while (values.hasNext()) { + writer.print(" "); + writer.println(values.next()); + } + values = getUsers(); + while (values.hasNext()) { + writer.print(" "); + writer.println(((MemoryUser) values.next()).toXml()); + } + } finally { + writeLock.unlock(); } // Print the file epilog