From bcf2890502fbd11dd394048fe30d67c92aeec4fa Mon Sep 17 00:00:00 2001 From: "Robert (Bobby) Evans" Date: Fri, 8 May 2015 11:09:29 -0500 Subject: [PATCH] YARN-644: Basic null check is not performed on passed in arguments before using them in ContainerManagerImpl.startContainer --- hadoop-yarn-project/CHANGES.txt | 3 + .../ContainerManagerImpl.java | 27 +++++- .../BaseContainerManagerTest.java | 6 ++ .../TestContainerManager.java | 87 +++++++++++++++++++ 4 files changed, 122 insertions(+), 1 deletion(-) diff --git a/hadoop-yarn-project/CHANGES.txt b/hadoop-yarn-project/CHANGES.txt index 26febe9ede..7f6a09f249 100644 --- a/hadoop-yarn-project/CHANGES.txt +++ b/hadoop-yarn-project/CHANGES.txt @@ -113,6 +113,9 @@ Release 2.8.0 - UNRELEASED IMPROVEMENTS + YARN-644. Basic null check is not performed on passed in arguments before + using them in ContainerManagerImpl.startContainer (Varun Saxena via bobby) + YARN-1880. Cleanup TestApplicationClientProtocolOnHA (ozawa via harsh) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/ContainerManagerImpl.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/ContainerManagerImpl.java index acac600ac0..c48df64bd9 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/ContainerManagerImpl.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/ContainerManagerImpl.java @@ -151,6 +151,10 @@ public class ContainerManagerImpl extends CompositeService implements private static final Log LOG = LogFactory.getLog(ContainerManagerImpl.class); + static final String INVALID_NMTOKEN_MSG = "Invalid NMToken"; + static final String INVALID_CONTAINERTOKEN_MSG = + "Invalid ContainerToken"; + final Context context; private final ContainersMonitor containersMonitor; private Server server; @@ -641,6 +645,9 @@ protected NMTokenIdentifier selectNMTokenIdentifier( protected void authorizeUser(UserGroupInformation remoteUgi, NMTokenIdentifier nmTokenIdentifier) throws YarnException { + if (nmTokenIdentifier == null) { + throw RPCUtil.getRemoteException(INVALID_NMTOKEN_MSG); + } if (!remoteUgi.getUserName().equals( nmTokenIdentifier.getApplicationAttemptId().toString())) { throw RPCUtil.getRemoteException("Expected applicationAttemptId: " @@ -658,7 +665,12 @@ protected void authorizeUser(UserGroupInformation remoteUgi, @VisibleForTesting protected void authorizeStartRequest(NMTokenIdentifier nmTokenIdentifier, ContainerTokenIdentifier containerTokenIdentifier) throws YarnException { - + if (nmTokenIdentifier == null) { + throw RPCUtil.getRemoteException(INVALID_NMTOKEN_MSG); + } + if (containerTokenIdentifier == null) { + throw RPCUtil.getRemoteException(INVALID_CONTAINERTOKEN_MSG); + } ContainerId containerId = containerTokenIdentifier.getContainerID(); String containerIDStr = containerId.toString(); boolean unauthorized = false; @@ -717,6 +729,10 @@ protected void authorizeStartRequest(NMTokenIdentifier nmTokenIdentifier, for (StartContainerRequest request : requests.getStartContainerRequests()) { ContainerId containerId = null; try { + if (request.getContainerToken() == null || + request.getContainerToken().getIdentifier() == null) { + throw new IOException(INVALID_CONTAINERTOKEN_MSG); + } ContainerTokenIdentifier containerTokenIdentifier = BuilderUtils.newContainerTokenIdentifier(request.getContainerToken()); verifyAndGetContainerTokenIdentifier(request.getContainerToken(), @@ -946,6 +962,9 @@ public StopContainersResponse stopContainers(StopContainersRequest requests) new HashMap(); UserGroupInformation remoteUgi = getRemoteUgi(); NMTokenIdentifier identifier = selectNMTokenIdentifier(remoteUgi); + if (identifier == null) { + throw RPCUtil.getRemoteException(INVALID_NMTOKEN_MSG); + } for (ContainerId id : requests.getContainerIds()) { try { stopContainerInternal(identifier, id); @@ -1001,6 +1020,9 @@ public GetContainerStatusesResponse getContainerStatuses( new HashMap(); UserGroupInformation remoteUgi = getRemoteUgi(); NMTokenIdentifier identifier = selectNMTokenIdentifier(remoteUgi); + if (identifier == null) { + throw RPCUtil.getRemoteException(INVALID_NMTOKEN_MSG); + } for (ContainerId id : request.getContainerIds()) { try { ContainerStatus status = getContainerStatusInternal(id, identifier); @@ -1041,6 +1063,9 @@ private ContainerStatus getContainerStatusInternal(ContainerId containerID, protected void authorizeGetAndStopContainerRequest(ContainerId containerId, Container container, boolean stopRequest, NMTokenIdentifier identifier) throws YarnException { + if (identifier == null) { + throw RPCUtil.getRemoteException(INVALID_NMTOKEN_MSG); + } /* * For get/stop container status; we need to verify that 1) User (NMToken) * application attempt only has started container. 2) Requested containerId diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/BaseContainerManagerTest.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/BaseContainerManagerTest.java index 8c0ceeb4d1..968c01048c 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/BaseContainerManagerTest.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/BaseContainerManagerTest.java @@ -230,6 +230,12 @@ public Map getAuxServiceMetaData() { ByteBuffer.wrap("AuxServiceMetaData2".getBytes())); return serviceData; } + + @Override + protected NMTokenIdentifier selectNMTokenIdentifier( + UserGroupInformation remoteUgi) { + return new NMTokenIdentifier(); + } }; } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestContainerManager.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestContainerManager.java index 34495a2427..7bdfdfb0a2 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestContainerManager.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestContainerManager.java @@ -45,6 +45,9 @@ import org.apache.hadoop.yarn.api.protocolrecords.StartContainersResponse; import org.apache.hadoop.yarn.api.protocolrecords.StopContainersRequest; import org.apache.hadoop.yarn.api.protocolrecords.StopContainersResponse; +import org.apache.hadoop.yarn.api.protocolrecords.impl.pb.GetContainerStatusesRequestPBImpl; +import org.apache.hadoop.yarn.api.protocolrecords.impl.pb.StartContainersRequestPBImpl; +import org.apache.hadoop.yarn.api.protocolrecords.impl.pb.StopContainersRequestPBImpl; import org.apache.hadoop.yarn.api.records.ApplicationAttemptId; import org.apache.hadoop.yarn.api.records.ApplicationId; import org.apache.hadoop.yarn.api.records.ContainerExitStatus; @@ -83,6 +86,7 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.mockito.Mockito; public class TestContainerManager extends BaseContainerManagerTest { @@ -792,6 +796,89 @@ public void testStartContainerFailureWithUnknownAuxService() throws Exception { .contains("The auxService:" + serviceName + " does not exist")); } + /* Test added to verify fix in YARN-644 */ + @Test + public void testNullTokens() throws Exception { + ContainerManagerImpl cMgrImpl = + new ContainerManagerImpl(context, exec, delSrvc, nodeStatusUpdater, + metrics, new ApplicationACLsManager(conf), dirsHandler); + String strExceptionMsg = ""; + try { + cMgrImpl.authorizeStartRequest(null, new ContainerTokenIdentifier()); + } catch(YarnException ye) { + strExceptionMsg = ye.getMessage(); + } + Assert.assertEquals(strExceptionMsg, + ContainerManagerImpl.INVALID_NMTOKEN_MSG); + + strExceptionMsg = ""; + try { + cMgrImpl.authorizeStartRequest(new NMTokenIdentifier(), null); + } catch(YarnException ye) { + strExceptionMsg = ye.getMessage(); + } + Assert.assertEquals(strExceptionMsg, + ContainerManagerImpl.INVALID_CONTAINERTOKEN_MSG); + + strExceptionMsg = ""; + try { + cMgrImpl.authorizeGetAndStopContainerRequest(null, null, true, null); + } catch(YarnException ye) { + strExceptionMsg = ye.getMessage(); + } + Assert.assertEquals(strExceptionMsg, + ContainerManagerImpl.INVALID_NMTOKEN_MSG); + + strExceptionMsg = ""; + try { + cMgrImpl.authorizeUser(null, null); + } catch(YarnException ye) { + strExceptionMsg = ye.getMessage(); + } + Assert.assertEquals(strExceptionMsg, + ContainerManagerImpl.INVALID_NMTOKEN_MSG); + + ContainerManagerImpl spyContainerMgr = Mockito.spy(cMgrImpl); + UserGroupInformation ugInfo = UserGroupInformation.createRemoteUser("a"); + Mockito.when(spyContainerMgr.getRemoteUgi()).thenReturn(ugInfo); + Mockito.when(spyContainerMgr. + selectNMTokenIdentifier(ugInfo)).thenReturn(null); + + strExceptionMsg = ""; + try { + spyContainerMgr.stopContainers(new StopContainersRequestPBImpl()); + } catch(YarnException ye) { + strExceptionMsg = ye.getMessage(); + } + Assert.assertEquals(strExceptionMsg, + ContainerManagerImpl.INVALID_NMTOKEN_MSG); + + strExceptionMsg = ""; + try { + spyContainerMgr.getContainerStatuses( + new GetContainerStatusesRequestPBImpl()); + } catch(YarnException ye) { + strExceptionMsg = ye.getMessage(); + } + Assert.assertEquals(strExceptionMsg, + ContainerManagerImpl.INVALID_NMTOKEN_MSG); + + Mockito.doNothing().when(spyContainerMgr).authorizeUser(ugInfo, null); + List reqList + = new ArrayList(); + reqList.add(StartContainerRequest.newInstance(null, null)); + StartContainersRequest reqs = new StartContainersRequestPBImpl(); + reqs.setStartContainerRequests(reqList); + strExceptionMsg = ""; + try { + spyContainerMgr.startContainers(reqs); + } catch(YarnException ye) { + strExceptionMsg = ye.getCause().getMessage(); + } + Assert.assertEquals(strExceptionMsg, + ContainerManagerImpl.INVALID_CONTAINERTOKEN_MSG); + } + public static Token createContainerToken(ContainerId cId, long rmIdentifier, NodeId nodeId, String user, NMContainerTokenSecretManager containerTokenSecretManager)