HADOOP-10489. UserGroupInformation#getTokens and UserGroupInformation#addToken can lead to ConcurrentModificationException. Contributed by Robert Kanter.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1596020 13f79535-47bb-0310-9956-ffa450edef68

hadoop-common-project/hadoop-common/CHANGES.txt

@@ -486,6 +486,9 @@ Release 2.5.0 - UNRELEASED
     HADOOP-10401. ShellBasedUnixGroupsMapping#getGroups does not always return
     primary group first (Akira AJISAKA via Colin Patrick McCabe)
 
+    HADOOP-10489. UserGroupInformation#getTokens and UserGroupInformation
+    #addToken can lead to ConcurrentModificationException (Robert Kanter via atm)
+
 Release 2.4.1 - UNRELEASED
 
   INCOMPATIBLE CHANGES

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java

@@ -1392,7 +1392,7 @@ public class UserGroupInformation {
    * @param token Token to be added
    * @return true on successful add of new token
    */
-  public synchronized boolean addToken(Token<? extends TokenIdentifier> token) {
+  public boolean addToken(Token<? extends TokenIdentifier> token) {
     return (token != null) ? addToken(token.getService(), token) : false;
   }
 
@@ -1403,29 +1403,32 @@ public class UserGroupInformation {
    * @param token Token to be added
    * @return true on successful add of new token
    */
-  public synchronized boolean addToken(Text alias,
+  public boolean addToken(Text alias, Token<? extends TokenIdentifier> token) {
-                                       Token<? extends TokenIdentifier> token) {
+    synchronized (subject) {
       getCredentialsInternal().addToken(alias, token);
       return true;
     }
+  }
   
   /**
    * Obtain the collection of tokens associated with this user.
    * 
    * @return an unmodifiable collection of tokens associated with user
    */
-  public synchronized
+  public Collection<Token<? extends TokenIdentifier>> getTokens() {
-  Collection<Token<? extends TokenIdentifier>> getTokens() {
+    synchronized (subject) {
       return Collections.unmodifiableCollection(
           new ArrayList<Token<?>>(getCredentialsInternal().getAllTokens()));
     }
+  }
 
   /**
    * Obtain the tokens in credentials form associated with this user.
    * 
    * @return Credentials of tokens associated with this user
    */
-  public synchronized Credentials getCredentials() {
+  public Credentials getCredentials() {
+    synchronized (subject) {
       Credentials creds = new Credentials(getCredentialsInternal());
       Iterator<Token<?>> iter = creds.getAllTokens().iterator();
       while (iter.hasNext()) {
@@ -1435,14 +1438,17 @@ public class UserGroupInformation {
       }
       return creds;
     }
+  }
   
   /**
    * Add the given Credentials to this user.
    * @param credentials of tokens and secrets
    */
-  public synchronized void addCredentials(Credentials credentials) {
+  public void addCredentials(Credentials credentials) {
+    synchronized (subject) {
       getCredentialsInternal().addAll(credentials);
     }
+  }
 
   private synchronized Credentials getCredentialsInternal() {
     final Credentials credentials;

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java

@@ -37,6 +37,7 @@ import java.io.InputStreamReader;
 import java.lang.reflect.Method;
 import java.security.PrivilegedExceptionAction;
 import java.util.Collection;
+import java.util.ConcurrentModificationException;
 import java.util.LinkedHashSet;
 import java.util.Set;
 
@@ -845,4 +846,69 @@ public class TestUserGroupInformation {
     Collection<Token<? extends TokenIdentifier>> tokens = ugi.getCredentials().getAllTokens();
     assertEquals(1, tokens.size());
   }
+
+  /**
+   * This test checks a race condition between getting and adding tokens for
+   * the current user.  Calling UserGroupInformation.getCurrentUser() returns
+   * a new object each time, so simply making these methods synchronized was not
+   * enough to prevent race conditions and causing a
+   * ConcurrentModificationException.  These methods are synchronized on the
+   * Subject, which is the same object between UserGroupInformation instances.
+   * This test tries to cause a CME, by exposing the race condition.  Previously
+   * this test would fail every time; now it does not.
+   */
+  @Test
+  public void testTokenRaceCondition() throws Exception {
+    UserGroupInformation userGroupInfo =
+      UserGroupInformation.createUserForTesting(USER_NAME, GROUP_NAMES);
+    userGroupInfo.doAs(new PrivilegedExceptionAction<Void>(){
+      @Override
+      public Void run() throws Exception {
+        // make sure it is not the same as the login user because we use the
+        // same UGI object for every instantiation of the login user and you
+        // won't run into the race condition otherwise
+        assertNotEquals(UserGroupInformation.getLoginUser(),
+                        UserGroupInformation.getCurrentUser());
+
+        GetTokenThread thread = new GetTokenThread();
+        try {
+          thread.start();
+          for (int i = 0; i < 100; i++) {
+            @SuppressWarnings("unchecked")
+            Token<? extends TokenIdentifier> t = mock(Token.class);
+            when(t.getService()).thenReturn(new Text("t" + i));
+            UserGroupInformation.getCurrentUser().addToken(t);
+            assertNull("ConcurrentModificationException encountered",
+                thread.cme);
+          }
+        } catch (ConcurrentModificationException cme) {
+          cme.printStackTrace();
+          fail("ConcurrentModificationException encountered");
+        } finally {
+          thread.runThread = false;
+          thread.join(5 * 1000);
+        }
+        return null;
+      }});
+  }
+
+  static class GetTokenThread extends Thread {
+    boolean runThread = true;
+    volatile ConcurrentModificationException cme = null;
+
+    @Override
+    public void run() {
+      while(runThread) {
+        try {
+          UserGroupInformation.getCurrentUser().getCredentials();
+        } catch (ConcurrentModificationException cme) {
+          this.cme = cme;
+          cme.printStackTrace();
+          runThread = false;
+        } catch (IOException ex) {
+          ex.printStackTrace();
+        }
+      }
+    }
+  }
 }