From c8df3668ecc37c2d58cad35520a762eaec3c8539 Mon Sep 17 00:00:00 2001 From: Jason Lowe Date: Thu, 20 Jul 2017 11:03:04 -0500 Subject: [PATCH] YARN-6837. Null LocalResource visibility or resource type can crash the nodemanager. Contributed by Jinjiang Ling --- .../impl/pb/ContainerLaunchContextPBImpl.java | 8 ++ .../TestApplicationClientProtocolRecords.java | 52 +++++++++++ .../ContainerManagerImpl.java | 6 ++ .../TestContainerManager.java | 90 +++++++++++++++++++ 4 files changed, 156 insertions(+) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/api/records/impl/pb/ContainerLaunchContextPBImpl.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/api/records/impl/pb/ContainerLaunchContextPBImpl.java index f07a9d6f055..d722cc58dbb 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/api/records/impl/pb/ContainerLaunchContextPBImpl.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/api/records/impl/pb/ContainerLaunchContextPBImpl.java @@ -222,6 +222,14 @@ extends ContainerLaunchContext { throw new NullPointerException( "Null resource URL for local resource " + rsrcEntry.getKey() + " : " + rsrcEntry.getValue()); + } else if (rsrcEntry.getValue().getType() == null) { + throw new NullPointerException( + "Null resource type for local resource " + rsrcEntry.getKey() + " : " + + rsrcEntry.getValue()); + } else if (rsrcEntry.getValue().getVisibility() == null) { + throw new NullPointerException( + "Null resource visibility for local resource " + rsrcEntry.getKey() + " : " + + rsrcEntry.getValue()); } } } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/api/records/impl/pb/TestApplicationClientProtocolRecords.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/api/records/impl/pb/TestApplicationClientProtocolRecords.java index 8773d11e2c0..6c51516434e 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/api/records/impl/pb/TestApplicationClientProtocolRecords.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/api/records/impl/pb/TestApplicationClientProtocolRecords.java @@ -25,6 +25,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import org.apache.hadoop.fs.Path; import org.apache.hadoop.io.DataOutputBuffer; import org.apache.hadoop.security.Credentials; import org.apache.hadoop.yarn.api.records.ApplicationAccessType; @@ -32,6 +33,7 @@ import org.apache.hadoop.yarn.api.records.ContainerLaunchContext; import org.apache.hadoop.yarn.api.records.LocalResource; import org.apache.hadoop.yarn.api.records.LocalResourceType; import org.apache.hadoop.yarn.api.records.LocalResourceVisibility; +import org.apache.hadoop.yarn.api.records.URL; import org.apache.hadoop.yarn.factories.RecordFactory; import org.apache.hadoop.yarn.factory.providers.RecordFactoryProvider; import org.junit.Assert; @@ -95,4 +97,54 @@ public class TestApplicationClientProtocolRecords { Assert.assertTrue(e.getMessage().contains("Null resource URL for local resource")); } } + + /* + * This test validates the scenario in which the client sets a null value for + * local resource type. + */ + @Test + public void testCLCPBImplNullResourceType() throws IOException { + RecordFactory recordFactory = RecordFactoryProvider.getRecordFactory(null); + try { + LocalResource resource = recordFactory.newRecordInstance(LocalResource.class); + resource.setResource(URL.fromPath(new Path("."))); + resource.setSize(-1); + resource.setVisibility(LocalResourceVisibility.APPLICATION); + resource.setType(null); + resource.setTimestamp(System.currentTimeMillis()); + Map localResources = + new HashMap(); + localResources.put("null_type_resource", resource); + ContainerLaunchContext containerLaunchContext = recordFactory.newRecordInstance(ContainerLaunchContext.class); + containerLaunchContext.setLocalResources(localResources); + Assert.fail("Setting an invalid local resource should be an error!"); + } catch (NullPointerException e) { + Assert.assertTrue(e.getMessage().contains("Null resource type for local resource")); + } + } + + /* + * This test validates the scenario in which the client sets a null value for + * local resource type. + */ + @Test + public void testCLCPBImplNullResourceVisibility() throws IOException { + RecordFactory recordFactory = RecordFactoryProvider.getRecordFactory(null); + try { + LocalResource resource = recordFactory.newRecordInstance(LocalResource.class); + resource.setResource(URL.fromPath(new Path("."))); + resource.setSize(-1); + resource.setVisibility(null); + resource.setType(LocalResourceType.FILE); + resource.setTimestamp(System.currentTimeMillis()); + Map localResources = + new HashMap(); + localResources.put("null_visibility_resource", resource); + ContainerLaunchContext containerLaunchContext = recordFactory.newRecordInstance(ContainerLaunchContext.class); + containerLaunchContext.setLocalResources(localResources); + Assert.fail("Setting an invalid local resource should be an error!"); + } catch (NullPointerException e) { + Assert.assertTrue(e.getMessage().contains("Null resource visibility for local resource")); + } + } } 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 b1d634aaebf..167d15d6fd1 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 @@ -1018,6 +1018,12 @@ public class ContainerManagerImpl extends CompositeService implements if (rsrc.getValue() == null || rsrc.getValue().getResource() == null) { throw new YarnException( "Null resource URL for local resource " + rsrc.getKey() + " : " + rsrc.getValue()); + } else if (rsrc.getValue().getType() == null) { + throw new YarnException( + "Null resource type for local resource " + rsrc.getKey() + " : " + rsrc.getValue()); + } else if (rsrc.getValue().getVisibility() == null) { + throw new YarnException( + "Null resource visibility for local resource " + rsrc.getKey() + " : " + rsrc.getValue()); } } 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 60df7cb8063..ba0ecce52df 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 @@ -1899,4 +1899,94 @@ public class TestContainerManager extends BaseContainerManagerTest { Assert.assertTrue(response.getFailedRequests().get(cId).getMessage() .contains("Null resource URL for local resource")); } + + @Test + public void testStartContainerFailureWithNullTypeLocalResource() + throws Exception { + containerManager.start(); + LocalResource rsrc_alpha = + recordFactory.newRecordInstance(LocalResource.class); + rsrc_alpha.setResource(URL.fromPath(new Path("./"))); + rsrc_alpha.setSize(-1); + rsrc_alpha.setVisibility(LocalResourceVisibility.APPLICATION); + rsrc_alpha.setType(null); + rsrc_alpha.setTimestamp(System.currentTimeMillis()); + Map localResources = + new HashMap(); + localResources.put("null_type_resource", rsrc_alpha); + ContainerLaunchContext containerLaunchContext = + recordFactory.newRecordInstance(ContainerLaunchContext.class); + ContainerLaunchContext spyContainerLaunchContext = + Mockito.spy(containerLaunchContext); + Mockito.when(spyContainerLaunchContext.getLocalResources()) + .thenReturn(localResources); + + ContainerId cId = createContainerId(0); + String user = "start_container_fail"; + Token containerToken = + createContainerToken(cId, DUMMY_RM_IDENTIFIER, context.getNodeId(), + user, context.getContainerTokenSecretManager()); + StartContainerRequest request = StartContainerRequest + .newInstance(spyContainerLaunchContext, containerToken); + + // start containers + List startRequest = + new ArrayList(); + startRequest.add(request); + StartContainersRequest requestList = + StartContainersRequest.newInstance(startRequest); + + StartContainersResponse response = + containerManager.startContainers(requestList); + Assert.assertTrue(response.getFailedRequests().size() == 1); + Assert.assertTrue(response.getSuccessfullyStartedContainers().size() == 0); + Assert.assertTrue(response.getFailedRequests().containsKey(cId)); + Assert.assertTrue(response.getFailedRequests().get(cId).getMessage() + .contains("Null resource type for local resource")); + } + + @Test + public void testStartContainerFailureWithNullVisibilityLocalResource() + throws Exception { + containerManager.start(); + LocalResource rsrc_alpha = + recordFactory.newRecordInstance(LocalResource.class); + rsrc_alpha.setResource(URL.fromPath(new Path("./"))); + rsrc_alpha.setSize(-1); + rsrc_alpha.setVisibility(null); + rsrc_alpha.setType(LocalResourceType.FILE); + rsrc_alpha.setTimestamp(System.currentTimeMillis()); + Map localResources = + new HashMap(); + localResources.put("null_visibility_resource", rsrc_alpha); + ContainerLaunchContext containerLaunchContext = + recordFactory.newRecordInstance(ContainerLaunchContext.class); + ContainerLaunchContext spyContainerLaunchContext = + Mockito.spy(containerLaunchContext); + Mockito.when(spyContainerLaunchContext.getLocalResources()) + .thenReturn(localResources); + + ContainerId cId = createContainerId(0); + String user = "start_container_fail"; + Token containerToken = + createContainerToken(cId, DUMMY_RM_IDENTIFIER, context.getNodeId(), + user, context.getContainerTokenSecretManager()); + StartContainerRequest request = StartContainerRequest + .newInstance(spyContainerLaunchContext, containerToken); + + // start containers + List startRequest = + new ArrayList(); + startRequest.add(request); + StartContainersRequest requestList = + StartContainersRequest.newInstance(startRequest); + + StartContainersResponse response = + containerManager.startContainers(requestList); + Assert.assertTrue(response.getFailedRequests().size() == 1); + Assert.assertTrue(response.getSuccessfullyStartedContainers().size() == 0); + Assert.assertTrue(response.getFailedRequests().containsKey(cId)); + Assert.assertTrue(response.getFailedRequests().get(cId).getMessage() + .contains("Null resource visibility for local resource")); + } }