YARN-1865. ShellScriptBuilder does not check for some error conditions. Contributed by Remus Rusanu.

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

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java

@@ -53,6 +53,34 @@ abstract public class Shell {
     return IS_JAVA7_OR_ABOVE;
   }
 
+  /**
+   * Maximum command line length in Windows
+   * KB830473 documents this as 8191
+   */
+  public static final int WINDOWS_MAX_SHELL_LENGHT = 8191;
+
+  /**
+   * Checks if a given command (String[]) fits in the Windows maximum command line length
+   * Note that the input is expected to already include space delimiters, no extra count
+   * will be added for delimiters.
+   *
+   * @param commands command parts, including any space delimiters
+   */
+  public static void checkWindowsCommandLineLength(String...commands)
+      throws IOException {
+    int len = 0;
+    for (String s: commands) {
+      len += s.length();
+    }
+    if (len > WINDOWS_MAX_SHELL_LENGHT) {
+      throw new IOException(String.format(
+          "The command line has a length of %d exceeds maximum allowed length of %d. " +
+          "Command starts with: %s",
+          len, WINDOWS_MAX_SHELL_LENGHT,
+          StringUtils.join("", commands).substring(0, 100)));
+    }
+  }
+
   /** a Unix command to get the current user's name */
   public final static String USER_NAME_COMMAND = "whoami";
 

hadoop-yarn-project/CHANGES.txt

@@ -71,6 +71,9 @@ Release 2.5.0 - UNRELEASED
     YARN-1940. deleteAsUser() terminates early without deleting more files on
     error (Rushabh S Shah via jlowe)
 
+    YARN-1865. ShellScriptBuilder does not check for some error conditions.
+    (Remus Rusanu via ivanmi)
+
 Release 2.4.1 - UNRELEASED
 
   INCOMPATIBLE CHANGES

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java

@@ -479,15 +479,20 @@ public class ContainerLaunch implements Callable<Integer> {
         + appIdStr;
   }
 
-  private static abstract class ShellScriptBuilder {
+  @VisibleForTesting
+  static abstract class ShellScriptBuilder {
+    public static ShellScriptBuilder create() {
+      return Shell.WINDOWS ? new WindowsShellScriptBuilder() :
+        new UnixShellScriptBuilder();
+    }
 
     private static final String LINE_SEPARATOR =
         System.getProperty("line.separator");
     private final StringBuilder sb = new StringBuilder();
 
-    public abstract void command(List<String> command);
+    public abstract void command(List<String> command) throws IOException;
 
-    public abstract void env(String key, String value);
+    public abstract void env(String key, String value) throws IOException;
 
     public final void symlink(Path src, Path dst) throws IOException {
       if (!src.isAbsolute()) {
@@ -520,11 +525,19 @@ public class ContainerLaunch implements Callable<Integer> {
 
     protected abstract void link(Path src, Path dst) throws IOException;
 
-    protected abstract void mkdir(Path path);
+    protected abstract void mkdir(Path path) throws IOException;
   }
 
   private static final class UnixShellScriptBuilder extends ShellScriptBuilder {
 
+    private void errorCheck() {
+      line("hadoop_shell_errorcode=$?");
+      line("if [ $hadoop_shell_errorcode -ne 0 ]");
+      line("then");
+      line("  exit $hadoop_shell_errorcode");
+      line("fi");
+    }
+
     public UnixShellScriptBuilder(){
       line("#!/bin/bash");
       line();
@@ -533,6 +546,7 @@ public class ContainerLaunch implements Callable<Integer> {
     @Override
     public void command(List<String> command) {
       line("exec /bin/bash -c \"", StringUtils.join(" ", command), "\"");
+      errorCheck();
     }
 
     @Override
@@ -543,31 +557,43 @@ public class ContainerLaunch implements Callable<Integer> {
     @Override
     protected void link(Path src, Path dst) throws IOException {
       line("ln -sf \"", src.toUri().getPath(), "\" \"", dst.toString(), "\"");
+      errorCheck();
     }
 
     @Override
     protected void mkdir(Path path) {
       line("mkdir -p ", path.toString());
+      errorCheck();
     }
   }
 
   private static final class WindowsShellScriptBuilder
       extends ShellScriptBuilder {
 
+    private void errorCheck() {
+      line("@if %errorlevel% neq 0 exit /b %errorlevel%");
+    }
+
+    private void lineWithLenCheck(String... commands) throws IOException {
+      Shell.checkWindowsCommandLineLength(commands);
+      line(commands);
+    }
+
     public WindowsShellScriptBuilder() {
       line("@setlocal");
       line();
     }
 
     @Override
-    public void command(List<String> command) {
+    public void command(List<String> command) throws IOException {
-      line("@call ", StringUtils.join(" ", command));
+      lineWithLenCheck("@call ", StringUtils.join(" ", command));
+      errorCheck();
     }
 
     @Override
-    public void env(String key, String value) {
+    public void env(String key, String value) throws IOException {
-      line("@set ", key, "=", value,
+      lineWithLenCheck("@set ", key, "=", value);
-          "\nif %errorlevel% neq 0 exit /b %errorlevel%");
+      errorCheck();
     }
 
     @Override
@@ -578,16 +604,20 @@ public class ContainerLaunch implements Callable<Integer> {
       // If not on Java7+ on Windows, then copy file instead of symlinking.
       // See also FileUtil#symLink for full explanation.
       if (!Shell.isJava7OrAbove() && srcFile.isFile()) {
-        line(String.format("@copy \"%s\" \"%s\"", srcFileStr, dstFileStr));
+        lineWithLenCheck(String.format("@copy \"%s\" \"%s\"", srcFileStr, dstFileStr));
+        errorCheck();
       } else {
-        line(String.format("@%s symlink \"%s\" \"%s\"", Shell.WINUTILS,
+        lineWithLenCheck(String.format("@%s symlink \"%s\" \"%s\"", Shell.WINUTILS,
           dstFileStr, srcFileStr));
+        errorCheck();
       }
     }
 
     @Override
-    protected void mkdir(Path path) {
+    protected void mkdir(Path path) throws IOException {
-      line("@if not exist ", path.toString(), " mkdir ", path.toString());
+      lineWithLenCheck(String.format("@if not exist \"%s\" mkdir \"%s\"",
+          path.toString(), path.toString()));
+      errorCheck();
     }
   }
 
@@ -730,8 +760,7 @@ public class ContainerLaunch implements Callable<Integer> {
       Map<String,String> environment, Map<Path,List<String>> resources,
       List<String> command)
       throws IOException {
-    ShellScriptBuilder sb = Shell.WINDOWS ? new WindowsShellScriptBuilder() :
+    ShellScriptBuilder sb = ShellScriptBuilder.create();
-      new UnixShellScriptBuilder();
     if (environment != null) {
       for (Map.Entry<String,String> env : environment.entrySet()) {
         sb.env(env.getKey().toString(), env.getValue().toString());

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/TestContainerLaunch.java

@@ -19,6 +19,9 @@
 package org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
+import static org.junit.matchers.JUnitMatchers.*;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -27,6 +30,7 @@ import java.io.File;
 import java.io.FileOutputStream;
 import java.io.FileReader;
 import java.io.IOException;
+import java.io.PrintStream;
 import java.io.PrintWriter;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
@@ -37,7 +41,6 @@ import java.util.List;
 import java.util.Map;
 
 import org.junit.Assert;
-
 import org.apache.commons.codec.binary.Base64;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileUtil;
@@ -76,6 +79,7 @@ import org.apache.hadoop.yarn.server.nodemanager.containermanager.BaseContainerM
 import org.apache.hadoop.yarn.server.nodemanager.containermanager.container.Container;
 import org.apache.hadoop.yarn.server.nodemanager.containermanager.container.ContainerEventType;
 import org.apache.hadoop.yarn.server.nodemanager.containermanager.container.ContainerExitEvent;
+import org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher.ContainerLaunch.ShellScriptBuilder;
 import org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.ContainerLocalizer;
 import org.apache.hadoop.yarn.server.utils.BuilderUtils;
 import org.apache.hadoop.yarn.util.Apps;
@@ -83,6 +87,7 @@ import org.apache.hadoop.yarn.util.AuxiliaryServiceHelper;
 import org.apache.hadoop.yarn.util.ConverterUtils;
 import org.apache.hadoop.yarn.util.LinuxResourceCalculatorPlugin;
 import org.apache.hadoop.yarn.util.ResourceCalculatorPlugin;
+import org.junit.Assume;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -743,18 +748,18 @@ public class TestContainerLaunch extends BaseContainerManagerTest {
     }
   }
 
-  @Test
+  @Test (timeout = 30000)
   public void testDelayedKill() throws Exception {
     internalKillTest(true);
   }
 
-  @Test
+  @Test (timeout = 30000)
   public void testImmediateKill() throws Exception {
     internalKillTest(false);
   }
 
   @SuppressWarnings("rawtypes")
-  @Test
+  @Test (timeout = 10000)
   public void testCallFailureWithNullLocalizedResources() {
     Container container = mock(Container.class);
     when(container.getContainerId()).thenReturn(ContainerId.newInstance(
@@ -792,4 +797,166 @@ public class TestContainerLaunch extends BaseContainerManagerTest {
     return containerToken;
   }
 
+  /**
+   * Test that script exists with non-zero exit code when command fails.
+   * @throws IOException
+   */
+  @Test (timeout = 10000)
+  public void testShellScriptBuilderNonZeroExitCode() throws IOException {
+    ShellScriptBuilder builder = ShellScriptBuilder.create();
+    builder.command(Arrays.asList(new String[] {"unknownCommand"}));
+    File shellFile = Shell.appendScriptExtension(tmpDir, "testShellScriptBuilderError");
+    PrintStream writer = new PrintStream(new FileOutputStream(shellFile));
+    builder.write(writer);
+    writer.close();
+    try {
+      FileUtil.setExecutable(shellFile, true);
+
+      Shell.ShellCommandExecutor shexc = new Shell.ShellCommandExecutor(
+          new String[]{shellFile.getAbsolutePath()}, tmpDir);
+      try {
+        shexc.execute();
+        fail("builder shell command was expected to throw");
+      }
+      catch(IOException e) {
+        // expected
+        System.out.println("Received an expected exception: " + e.getMessage());
+      }
+    }
+    finally {
+      FileUtil.fullyDelete(shellFile);
+    }
+  }
+
+  private static final String expectedMessage = "The command line has a length of";
+  
+  @Test (timeout = 10000)
+  public void testWindowsShellScriptBuilderCommand() throws IOException {
+    String callCmd = "@call ";
+    
+    // Test is only relevant on Windows
+    Assume.assumeTrue(Shell.WINDOWS);
+
+    // The tests are built on assuming 8191 max command line length
+    assertEquals(8191, Shell.WINDOWS_MAX_SHELL_LENGHT);
+
+    ShellScriptBuilder builder = ShellScriptBuilder.create();
+
+    // Basic tests: less length, exact length, max+1 length 
+    builder.command(Arrays.asList(
+        org.apache.commons.lang.StringUtils.repeat("A", 1024)));
+    builder.command(Arrays.asList(
+        org.apache.commons.lang.StringUtils.repeat(
+            "E", Shell.WINDOWS_MAX_SHELL_LENGHT - callCmd.length())));
+    try {
+      builder.command(Arrays.asList(
+          org.apache.commons.lang.StringUtils.repeat(
+              "X", Shell.WINDOWS_MAX_SHELL_LENGHT -callCmd.length() + 1)));
+      fail("longCommand was expected to throw");
+    } catch(IOException e) {
+      assertThat(e.getMessage(), containsString(expectedMessage));
+    }
+
+    // Composite tests, from parts: less, exact and +
+    builder.command(Arrays.asList(
+        org.apache.commons.lang.StringUtils.repeat("A", 1024),
+        org.apache.commons.lang.StringUtils.repeat("A", 1024),
+        org.apache.commons.lang.StringUtils.repeat("A", 1024)));
+
+    // buildr.command joins the command parts with an extra space
+    builder.command(Arrays.asList(
+        org.apache.commons.lang.StringUtils.repeat("E", 4095),
+        org.apache.commons.lang.StringUtils.repeat("E", 2047),
+        org.apache.commons.lang.StringUtils.repeat("E", 2047 - callCmd.length())));
+
+    try {
+      builder.command(Arrays.asList(
+          org.apache.commons.lang.StringUtils.repeat("X", 4095), 
+          org.apache.commons.lang.StringUtils.repeat("X", 2047),
+          org.apache.commons.lang.StringUtils.repeat("X", 2048 - callCmd.length())));
+      fail("long commands was expected to throw");
+    } catch(IOException e) {
+      assertThat(e.getMessage(), containsString(expectedMessage));
+    }
+  }
+  
+  @Test (timeout = 10000)
+  public void testWindowsShellScriptBuilderEnv() throws IOException {
+    // Test is only relevant on Windows
+    Assume.assumeTrue(Shell.WINDOWS);
+
+    // The tests are built on assuming 8191 max command line length
+    assertEquals(8191, Shell.WINDOWS_MAX_SHELL_LENGHT);
+
+    ShellScriptBuilder builder = ShellScriptBuilder.create();
+
+    // test env
+    builder.env("somekey", org.apache.commons.lang.StringUtils.repeat("A", 1024));
+    builder.env("somekey", org.apache.commons.lang.StringUtils.repeat(
+        "A", Shell.WINDOWS_MAX_SHELL_LENGHT - ("@set somekey=").length()));
+    try {
+      builder.env("somekey", org.apache.commons.lang.StringUtils.repeat(
+          "A", Shell.WINDOWS_MAX_SHELL_LENGHT - ("@set somekey=").length()) + 1);
+      fail("long env was expected to throw");
+    } catch(IOException e) {
+      assertThat(e.getMessage(), containsString(expectedMessage));
+    }
+  }
+    
+  @Test (timeout = 10000)
+  public void testWindowsShellScriptBuilderMkdir() throws IOException {
+    String mkDirCmd = "@if not exist \"\" mkdir \"\"";
+
+    // Test is only relevant on Windows
+    Assume.assumeTrue(Shell.WINDOWS);
+
+    // The tests are built on assuming 8191 max command line length
+    assertEquals(8191, Shell.WINDOWS_MAX_SHELL_LENGHT);
+
+    ShellScriptBuilder builder = ShellScriptBuilder.create();
+
+    // test mkdir
+    builder.mkdir(new Path(org.apache.commons.lang.StringUtils.repeat("A", 1024)));
+    builder.mkdir(new Path(org.apache.commons.lang.StringUtils.repeat(
+        "E", (Shell.WINDOWS_MAX_SHELL_LENGHT - mkDirCmd.length())/2)));
+    try {
+      builder.mkdir(new Path(org.apache.commons.lang.StringUtils.repeat(
+          "X", (Shell.WINDOWS_MAX_SHELL_LENGHT - mkDirCmd.length())/2 +1)));
+      fail("long mkdir was expected to throw");
+    } catch(IOException e) {
+      assertThat(e.getMessage(), containsString(expectedMessage));
+    }    
+  }
+
+  @Test (timeout = 10000)
+  public void testWindowsShellScriptBuilderLink() throws IOException {
+    // Test is only relevant on Windows
+    Assume.assumeTrue(Shell.WINDOWS);
+
+    String linkCmd = "@" +Shell.WINUTILS + " symlink \"\" \"\"";
+
+    // The tests are built on assuming 8191 max command line length
+    assertEquals(8191, Shell.WINDOWS_MAX_SHELL_LENGHT);
+
+    ShellScriptBuilder builder = ShellScriptBuilder.create();
+
+    // test link
+    builder.link(new Path(org.apache.commons.lang.StringUtils.repeat("A", 1024)),
+        new Path(org.apache.commons.lang.StringUtils.repeat("B", 1024)));
+    builder.link(
+        new Path(org.apache.commons.lang.StringUtils.repeat(
+            "E", (Shell.WINDOWS_MAX_SHELL_LENGHT - linkCmd.length())/2)),
+        new Path(org.apache.commons.lang.StringUtils.repeat(
+            "F", (Shell.WINDOWS_MAX_SHELL_LENGHT - linkCmd.length())/2)));
+    try {
+      builder.link(
+          new Path(org.apache.commons.lang.StringUtils.repeat(
+              "X", (Shell.WINDOWS_MAX_SHELL_LENGHT - linkCmd.length())/2 + 1)),
+          new Path(org.apache.commons.lang.StringUtils.repeat(
+              "Y", (Shell.WINDOWS_MAX_SHELL_LENGHT - linkCmd.length())/2) + 1));
+      fail("long link was expected to throw");
+    } catch(IOException e) {
+      assertThat(e.getMessage(), containsString(expectedMessage));
+    }
+  }
 }