HADOOP-13434. Add bash quoting to Shell class. (Owen O'Malley)

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

@@ -42,7 +42,7 @@
  * A base class for running a Shell command.
  *
  * <code>Shell</code> can be used to run shell commands like <code>du</code> or
- * <code>df</code>. It also offers facilities to gate commands by 
+ * <code>df</code>. It also offers facilities to gate commands by
  * time-intervals.
  */
 @InterfaceAudience.Public
@@ -118,6 +118,21 @@ public static void checkWindowsCommandLineLength(String...commands)
     }
   }
 
+  /**
+   * Quote the given arg so that bash will interpret it as a single value.
+   * Note that this quotes it for one level of bash, if you are passing it
+   * into a badly written shell script, you need to fix your shell script.
+   * @param arg the argument to quote
+   * @return the quoted string
+   */
+  static String bashQuote(String arg) {
+    StringBuilder buffer = new StringBuilder(arg.length() + 2);
+    buffer.append('\'');
+    buffer.append(arg.replace("'", "'\\''"));
+    buffer.append('\'');
+    return buffer.toString();
+  }
+
   /** a Unix command to get the current user's name: {@value}. */
   public static final String USER_NAME_COMMAND = "whoami";
 
@@ -173,7 +188,7 @@ private static OSType getOSType() {
   /** a Unix command to get the current user's groups list. */
   public static String[] getGroupsCommand() {
     return (WINDOWS)? new String[]{"cmd", "/c", "groups"}
-                    : new String[]{"bash", "-c", "groups"};
+                    : new String[]{"groups"};
   }
 
   /**
@@ -184,10 +199,14 @@ public static String[] getGroupsCommand() {
    */
   public static String[] getGroupsForUserCommand(final String user) {
     //'groups username' command return is inconsistent across different unixes
-    return WINDOWS ?
+    if (WINDOWS) {
-      new String[]
+      return new String[]
-          {getWinUtilsPath(), "groups", "-F", "\"" + user + "\""}
+          {getWinUtilsPath(), "groups", "-F", "\"" + user + "\""};
-      : new String[] {"bash", "-c", "id -gn " + user + "; id -Gn " + user};
+    } else {
+      String quotedUser = bashQuote(user);
+      return new String[] {"bash", "-c", "id -gn " + quotedUser +
+                            "; id -Gn " + quotedUser};
+    }
   }
 
   /**
@@ -199,17 +218,20 @@ public static String[] getGroupsForUserCommand(final String user) {
    */
   public static String[] getGroupsIDForUserCommand(final String user) {
     //'groups username' command return is inconsistent across different unixes
-    return WINDOWS ?
+    if (WINDOWS) {
-        new String[]
+      return new String[]{getWinUtilsPath(), "groups", "-F", "\"" + user +
-            {getWinUtilsPath(), "groups", "-F", "\"" + user + "\""}
+                           "\""};
-        : new String[] {"bash", "-c", "id -g " + user + "; id -G " + user};
+    } else {
+      String quotedUser = bashQuote(user);
+      return new String[] {"bash", "-c", "id -g " + quotedUser + "; id -G " +
+                            quotedUser};
+    }
   }
 
   /** A command to get a given netgroup's user list. */
   public static String[] getUsersForNetgroupCommand(final String netgroup) {
     //'groups username' command return is non-consistent across different unixes
-    return WINDOWS ? new String [] {"cmd", "/c", "getent netgroup " + netgroup}
+    return new String[] {"getent", "netgroup", netgroup};
-                    : new String [] {"bash", "-c", "getent netgroup " + netgroup};
   }
 
   /** Return a command to get permission information. */
@@ -233,14 +255,15 @@ public static String[] getSetPermissionCommand(String perm, boolean recursive) {
 
   /**
    * Return a command to set permission for specific file.
-   * 
+   *
    * @param perm String permission to set
    * @param recursive boolean true to apply to all sub-directories recursively
    * @param file String file to set
    * @return String[] containing command and arguments
    */
   public static String[] getSetPermissionCommand(String perm,
-      boolean recursive, String file) {
+                                                 boolean recursive,
+                                                 String file) {
     String[] baseCmd = getSetPermissionCommand(perm, recursive);
     String[] cmdWithFile = Arrays.copyOf(baseCmd, baseCmd.length + 1);
     cmdWithFile[cmdWithFile.length - 1] = file;
@@ -290,9 +313,9 @@ public static String[] getSignalKillCommand(int code, String pid) {
 
     if (isSetsidAvailable) {
       // Use the shell-builtin as it support "--" in all Hadoop supported OSes
-      return new String[] { "bash", "-c", "kill -" + code + " -- -" + pid };
+      return new String[] {"kill", "-" + code, "--", "-" + pid};
     } else {
-      return new String[] { "bash", "-c", "kill -" + code + " " + pid };
+      return new String[] {"kill", "-" + code, pid };
     }
   }
 
@@ -310,7 +333,7 @@ public static String getEnvironmentVariableRegex() {
    * Returns a File referencing a script with the given basename, inside the
    * given parent directory.  The file extension is inferred by platform:
    * <code>".cmd"</code> on Windows, or <code>".sh"</code> otherwise.
-   * 
+   *
    * @param parent File parent directory
    * @param basename String script file basename
    * @return File referencing the script in the directory
@@ -342,8 +365,8 @@ public static String appendScriptExtension(String basename) {
   public static String[] getRunScriptCommand(File script) {
     String absolutePath = script.getAbsolutePath();
     return WINDOWS ?
-      new String[] { "cmd", "/c", absolutePath }
+      new String[] {"cmd", "/c", absolutePath }
-      : new String[] { "/bin/bash", absolutePath };
+      : new String[] {"/bin/bash", bashQuote(absolutePath) };
   }
 
   /** a Unix command to set permission: {@value}. */
@@ -527,11 +550,11 @@ private static File getHadoopHomeDir() throws FileNotFoundException {
 
   /**
    *  Fully qualify the path to a binary that should be in a known hadoop
-   *  bin location. This is primarily useful for disambiguating call-outs 
+   *  bin location. This is primarily useful for disambiguating call-outs
-   *  to executable sub-components of Hadoop to avoid clashes with other 
+   *  to executable sub-components of Hadoop to avoid clashes with other
-   *  executables that may be in the path.  Caveat:  this call doesn't 
+   *  executables that may be in the path.  Caveat:  this call doesn't
-   *  just format the path to the bin directory.  It also checks for file 
+   *  just format the path to the bin directory.  It also checks for file
-   *  existence of the composed path. The output of this call should be 
+   *  existence of the composed path. The output of this call should be
    *  cached by callers.
    *
    * @param executable executable
@@ -840,7 +863,7 @@ protected void run() throws IOException {
   }
 
   /** Run the command. */
-  private void runCommand() throws IOException { 
+  private void runCommand() throws IOException {
     ProcessBuilder builder = new ProcessBuilder(getExecString());
     Timer timeOutTimer = null;
     ShellTimeoutTimerTask timeoutTimerTask = null;
@@ -869,7 +892,7 @@ private void runCommand() throws IOException {
     }
 
     builder.redirectErrorStream(redirectErrorStream);
-    
+
     if (Shell.WINDOWS) {
       synchronized (WindowsProcessLaunchLock) {
         // To workaround the race condition issue with child processes
@@ -890,7 +913,7 @@ private void runCommand() throws IOException {
       //One time scheduling.
       timeOutTimer.schedule(timeoutTimerTask, timeOutInterval);
     }
-    final BufferedReader errReader = 
+    final BufferedReader errReader =
             new BufferedReader(new InputStreamReader(
                 process.getErrorStream(), Charset.defaultCharset()));
     BufferedReader inReader =
@@ -928,7 +951,7 @@ public void run() {
       parseExecResult(inReader); // parse the output
       // clear the input stream buffer
       String line = inReader.readLine();
-      while(line != null) { 
+      while(line != null) {
         line = inReader.readLine();
       }
       // wait for the process to finish and check the exit code
@@ -1065,13 +1088,13 @@ public interface CommandExecutor {
 
   /**
    * A simple shell command executor.
-   * 
+   *
    * <code>ShellCommandExecutor</code>should be used in cases where the output
    * of the command needs no explicit parsing and where the command, working
    * directory and the environment remains unchanged. The output of the command
    * is stored as-is and is expected to be small.
    */
-  public static class ShellCommandExecutor extends Shell 
+  public static class ShellCommandExecutor extends Shell
       implements CommandExecutor {
 
     private String[] command;
@@ -1098,7 +1121,7 @@ public ShellCommandExecutor(String[] execString, File dir,
 
     /**
      * Create a new instance of the ShellCommandExecutor to execute a command.
-     * 
+     *
      * @param execString The command to execute with arguments
      * @param dir If not-null, specifies the directory which should be set
      *            as the current working directory for the command.
@@ -1112,7 +1135,7 @@ public ShellCommandExecutor(String[] execString, File dir,
      * @param inheritParentEnv Indicates if the process should inherit the env
      *                         vars from the parent process or not.
      */
-    public ShellCommandExecutor(String[] execString, File dir, 
+    public ShellCommandExecutor(String[] execString, File dir,
         Map<String, String> env, long timeout, boolean inheritParentEnv) {
       command = execString.clone();
       if (dir != null) {
@@ -1137,7 +1160,7 @@ public void execute() throws IOException {
               + StringUtils.join(" ", command));
         }
       }
-      this.run();    
+      this.run();
     }
 
     @Override
@@ -1190,7 +1213,7 @@ public void close() {
   /**
    * To check if the passed script to shell command executor timed out or
    * not.
-   * 
+   *
    * @return if the script timed out.
    */
   public boolean isTimedOut() {
@@ -1199,15 +1222,15 @@ public boolean isTimedOut() {
 
   /**
    * Declare that the command has timed out.
-   * 
+   *
    */
   private void setTimedOut() {
     this.timedOut.set(true);
   }
 
-  /** 
+  /**
-   * Static method to execute a shell command. 
+   * Static method to execute a shell command.
-   * Covers most of the simple cases without requiring the user to implement  
+   * Covers most of the simple cases without requiring the user to implement
    * the <code>Shell</code> interface.
    * @param cmd shell command to execute.
    * @return the output of the executed command.
@@ -1229,7 +1252,7 @@ public static String execCommand(String ... cmd) throws IOException {
 
   public static String execCommand(Map<String, String> env, String[] cmd,
       long timeout) throws IOException {
-    ShellCommandExecutor exec = new ShellCommandExecutor(cmd, null, env, 
+    ShellCommandExecutor exec = new ShellCommandExecutor(cmd, null, env,
                                                           timeout);
     exec.execute();
     return exec.getOutput();
@@ -1267,7 +1290,7 @@ public void run() {
         p.exitValue();
       } catch (Exception e) {
         //Process has not terminated.
-        //So check if it has completed 
+        //So check if it has completed
         //if not just destroy it.
         if (p != null && !shell.completed.get()) {
           shell.setTimedOut();

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java

@@ -238,9 +238,9 @@ public void testGetCheckProcessIsAliveCommand() throws Exception {
       expectedCommand =
           new String[]{getWinUtilsPath(), "task", "isAlive", anyPid };
     } else if (Shell.isSetsidAvailable) {
-      expectedCommand = new String[] { "bash", "-c", "kill -0 -- -" + anyPid };
+      expectedCommand = new String[] {"kill", "-0", "--", "-" + anyPid };
     } else {
-      expectedCommand = new String[]{ "bash", "-c", "kill -0 " + anyPid };
+      expectedCommand = new String[] {"kill", "-0", anyPid };
     }
     Assert.assertArrayEquals(expectedCommand, checkProcessAliveCommand);
   }
@@ -258,9 +258,9 @@ public void testGetSignalKillCommand() throws Exception {
       expectedCommand =
           new String[]{getWinUtilsPath(), "task", "kill", anyPid };
     } else if (Shell.isSetsidAvailable) {
-      expectedCommand = new String[] { "bash", "-c", "kill -9 -- -" + anyPid };
+      expectedCommand = new String[] {"kill", "-9", "--", "-" + anyPid };
     } else {
-      expectedCommand = new String[]{ "bash", "-c", "kill -9 " + anyPid };
+      expectedCommand = new String[] {"kill", "-9", anyPid };
     }
     Assert.assertArrayEquals(expectedCommand, checkProcessAliveCommand);
   }
@@ -464,4 +464,10 @@ private void assertExContains(Exception ex, String expectedText)
     }
   }
 
+  @Test
+  public void testBashQuote() {
+    assertEquals("'foobar'", Shell.bashQuote("foobar"));
+    assertEquals("'foo'\\''bar'", Shell.bashQuote("foo'bar"));
+    assertEquals("''\\''foo'\\''bar'\\'''", Shell.bashQuote("'foo'bar'"));
+  }
 }