From bd64a2a9cd82398bfded7e84f795be1473a298bb Mon Sep 17 00:00:00 2001 From: Aaron Myers Date: Mon, 19 May 2014 19:56:29 +0000 Subject: [PATCH] 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/CHANGES.txt | 3 + .../hadoop/security/UserGroupInformation.java | 42 +++++++----- .../security/TestUserGroupInformation.java | 66 +++++++++++++++++++ 3 files changed, 93 insertions(+), 18 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index fe678a788f..30149ad5ba 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/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 diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java index b1d164d58f..af552a798b 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java +++ b/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 token) { + public boolean addToken(Token token) { return (token != null) ? addToken(token.getService(), token) : false; } @@ -1403,10 +1403,11 @@ public class UserGroupInformation { * @param token Token to be added * @return true on successful add of new token */ - public synchronized boolean addToken(Text alias, - Token token) { - getCredentialsInternal().addToken(alias, token); - return true; + public boolean addToken(Text alias, Token token) { + synchronized (subject) { + getCredentialsInternal().addToken(alias, token); + return true; + } } /** @@ -1414,10 +1415,11 @@ public class UserGroupInformation { * * @return an unmodifiable collection of tokens associated with user */ - public synchronized - Collection> getTokens() { - return Collections.unmodifiableCollection( - new ArrayList>(getCredentialsInternal().getAllTokens())); + public Collection> getTokens() { + synchronized (subject) { + return Collections.unmodifiableCollection( + new ArrayList>(getCredentialsInternal().getAllTokens())); + } } /** @@ -1425,23 +1427,27 @@ public class UserGroupInformation { * * @return Credentials of tokens associated with this user */ - public synchronized Credentials getCredentials() { - Credentials creds = new Credentials(getCredentialsInternal()); - Iterator> iter = creds.getAllTokens().iterator(); - while (iter.hasNext()) { - if (iter.next() instanceof Token.PrivateToken) { - iter.remove(); + public Credentials getCredentials() { + synchronized (subject) { + Credentials creds = new Credentials(getCredentialsInternal()); + Iterator> iter = creds.getAllTokens().iterator(); + while (iter.hasNext()) { + if (iter.next() instanceof Token.PrivateToken) { + iter.remove(); + } } + return creds; } - return creds; } /** * Add the given Credentials to this user. * @param credentials of tokens and secrets */ - public synchronized void addCredentials(Credentials credentials) { - getCredentialsInternal().addAll(credentials); + public void addCredentials(Credentials credentials) { + synchronized (subject) { + getCredentialsInternal().addAll(credentials); + } } private synchronized Credentials getCredentialsInternal() { diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java index 614054f7c2..ed27762738 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java +++ b/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> 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(){ + @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 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(); + } + } + } + } }