opendev/gerrit
Code Issues Proposed changes

Fix IncludingGroupMembership.containsAnyOf infinite loop

Browse Source 
This class is using a map to store the memberOf and can be accessed
by multiple threads concurrently. The map was not synchronized which was
causing an infinite loop under very specific sequence of events. Here is
a snippet of the thread dump when that problem happened:

"SSH gerrit review ..." prio=1 RUNNABLE
java.util.HashMap.getEntry(HashMap.java:446)
java.util.HashMap.get(HashMap.java:405)
com.google.gerrit.server.account.IncludingGroupMembership.containsAnyOf(IncludingGroupMembership.java:77)
com.google.gerrit.server.account.UniversalGroupBackend$UniversalGroupMembership.containsAnyOf(UniversalGroupBackend.java:152)
com.google.gerrit.server.account.IncludingGroupMembership.search(IncludingGroupMembership.java:115)
com.google.gerrit.server.account.IncludingGroupMembership.containsAnyOf(IncludingGroupMembership.java:93)
com.google.gerrit.server.account.IncludingGroupMembership.contains(IncludingGroupMembership.java:69)
com.google.gerrit.server.account.UniversalGroupBackend$UniversalGroupMembership.contains(UniversalGroupBackend.java:129)
com.google.gerrit.server.project.ProjectControl.match(ProjectControl.java:493)

Change the HashMap to a ConcurrentHashMap to handle concurrent access.

Change-Id: I36f654281b7a1d7126936d86d64e6827d8987577
This commit is contained in:
Hugo Arès 2014-09-25 10:16:59 -04:00 committed by David Pursehouse
parent 5766bc02d8
commit 16e38d5685
1 changed files with 2 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
View File

@ -16,7 +16,6 @@ package com.google.gerrit.server.account;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists; import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets; import com.google.common.collect.Sets;
import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser;
@ -26,6 +25,7 @@ import com.google.inject.assistedinject.Assisted;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Set; import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
/** /**
* Group membership checker for the internal group system. * Group membership checker for the internal group system.
@ -53,7 +53,7 @@ public class IncludingGroupMembership implements GroupMembership {
this.user = user; this.user = user;
Set<AccountGroup.UUID> groups = user.state().getInternalGroups(); Set<AccountGroup.UUID> groups = user.state().getInternalGroups();
memberOf = Maps.newHashMapWithExpectedSize(groups.size()); memberOf = new ConcurrentHashMap<>(groups.size());
for (AccountGroup.UUID g : groups) { for (AccountGroup.UUID g : groups) {
memberOf.put(g, true); memberOf.put(g, true);
} }