Merge "Fix escaping and grouping of arguments to the Target shells"
diff --git a/src/vogar/LocalTarget.java b/src/vogar/LocalTarget.java
index 5e194c4..d15bf7c 100644
--- a/src/vogar/LocalTarget.java
+++ b/src/vogar/LocalTarget.java
@@ -30,6 +30,8 @@
*/
public final class LocalTarget extends Target {
+ private static final ImmutableList<String> TARGET_PROCESS_PREFIX = ImmutableList.of("sh", "-c");
+
private final Log log;
private final Mkdir mkdir;
@@ -46,8 +48,8 @@
return new File("/tmp/vogar");
}
- @Override public List<String> targetProcessPrefix() {
- return ImmutableList.of("sh", "-c");
+ @Override protected ImmutableList<String> targetProcessPrefix() {
+ return TARGET_PROCESS_PREFIX;
}
@Override public String getDeviceUserName() {
diff --git a/src/vogar/Run.java b/src/vogar/Run.java
index 7cadb3a..b4ff076 100644
--- a/src/vogar/Run.java
+++ b/src/vogar/Run.java
@@ -314,13 +314,11 @@
}
/**
- * Returns an environment variable assignment to configure where the VM will
- * store its dexopt files. This must be set on production devices and is
- * optional for development devices.
+ * Returns the directory where the VM stores its dexopt files.
*/
- public String getAndroidData() {
+ public String getAndroidDataPath() {
// The VM wants the parent directory of a directory named "dalvik-cache"
- return "ANDROID_DATA=" + dalvikCache().getParentFile();
+ return dalvikCache().getParentFile().getPath();
}
/**
diff --git a/src/vogar/SshTarget.java b/src/vogar/SshTarget.java
index c70ccd0..94177db 100644
--- a/src/vogar/SshTarget.java
+++ b/src/vogar/SshTarget.java
@@ -56,7 +56,7 @@
return new File("/data/local/tmp/vogar");
}
- @Override public List<String> targetProcessPrefix() {
+ @Override protected ImmutableList<String> targetProcessPrefix() {
return sshCommandPrefixList;
}
diff --git a/src/vogar/Target.java b/src/vogar/Target.java
index da414ff..78fa297 100644
--- a/src/vogar/Target.java
+++ b/src/vogar/Target.java
@@ -16,16 +16,21 @@
package vogar;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
import java.io.File;
import java.io.FileNotFoundException;
+import java.util.ArrayList;
import java.util.List;
+import java.util.Map;
import vogar.tasks.Task;
/**
* A target runtime environment such as a remote device or the local host
*/
public abstract class Target {
- public abstract List<String> targetProcessPrefix();
+ protected abstract ImmutableList<String> targetProcessPrefix();
public abstract String getDeviceUserName();
public abstract List<File> ls(File directory) throws FileNotFoundException;
@@ -53,4 +58,127 @@
}
};
}
+
+ /**
+ * Create a {@link ScriptBuilder} appropriate for this target.
+ */
+ public ScriptBuilder newScriptBuilder() {
+ return new ScriptBuilder(targetProcessPrefix());
+ }
+
+ /**
+ * Responsible for constructing a one line script appropriate for a specific target.
+ */
+ public static class ScriptBuilder {
+
+ private static final Joiner SCRIPT_JOINER = Joiner.on(" ");
+
+ /**
+ * Escape any special shell characters so that the target shell treats them as literal
+ * characters.
+ *
+ * <p>e,g, an escaped space will not be treated as a token separator, an escaped dollar will
+ * not cause shell substitution.
+ */
+ @VisibleForTesting
+ static String escape(String token) {
+ int length = token.length();
+ StringBuilder builder = new StringBuilder(length);
+ for (int i = 0; i < length; ++i) {
+ char c = token.charAt(i);
+ if (Character.isWhitespace(c)
+ || c == '\'' || c == '\"'
+ || c == '|' || c == '&'
+ || c == '<' || c == '>'
+ || c == '$' || c == '!'
+ || c == '(' || c == ')') {
+ builder.append('\\');
+ }
+
+ builder.append(c);
+ }
+
+ return builder.toString();
+ }
+
+ /**
+ * The prefix to insert before the script to produce a command line that will execute the
+ * script within a shell.
+ */
+ private final ImmutableList<String> commandLinePrefix;
+
+ /**
+ * The list of tokens making up the script, they were escaped where necessary before they
+ * were added to the list.
+ */
+ private final List<String> escapedTokens;
+
+ private ScriptBuilder(ImmutableList<String> commandLinePrefix) {
+ this.commandLinePrefix = commandLinePrefix;
+ escapedTokens = new ArrayList<>();
+ }
+
+ /**
+ * Set the working directory in the target shell before running the command.
+ */
+ public ScriptBuilder workingDirectory(File workingDirectory) {
+ escapedTokens.add("cd");
+ escapedTokens.add(escape(workingDirectory.getPath()));
+ escapedTokens.add("&&");
+ return this;
+ }
+
+ /**
+ * Set inline environment variables on the target shell that will affect the command.
+ */
+ public void env(Map<String, String> env) {
+ for (Map.Entry<String, String> entry : env.entrySet()) {
+ String name = entry.getKey();
+ String value = entry.getValue();
+ escapedTokens.add(name + "=" + escape(value));
+ }
+ }
+
+ /**
+ * Add tokens to the script.
+ *
+ * <p>Each token is escaped before adding it to the list. This method is suitable for adding
+ * the command line name and arguments.
+ */
+ public ScriptBuilder tokens(List<String> tokens) {
+ for (String token : tokens) {
+ escapedTokens.add(escape(token));
+ }
+ return this;
+ }
+
+ /**
+ * Add tokens to the script.
+ *
+ * <p>Each token is escaped before adding it to the list. This method is suitable for adding
+ * the command line name and arguments.
+ */
+ public ScriptBuilder tokens(String... tokens) {
+ for (String token : tokens) {
+ escapedTokens.add(escape(token));
+ }
+ return this;
+ }
+
+ /**
+ * Construct a command line to execute the script in the target shell.
+ */
+ public List<String> commandLine() {
+ // Group the tokens into a single String argument. This is necessary for running in
+ // a local shell where the first argument after the -c option is the script and the
+ // remainder are treated as arguments to the script. This has no effect on either
+ // adb or ssh shells as they both concatenate all their arguments into one single
+ // string before parsing.
+ String grouped = SCRIPT_JOINER.join(escapedTokens);
+ return new ImmutableList.Builder<String>()
+ .addAll(commandLinePrefix)
+ .add(grouped)
+ .build();
+ }
+ }
}
diff --git a/src/vogar/android/AdbTarget.java b/src/vogar/android/AdbTarget.java
index edc7481..48473fd 100644
--- a/src/vogar/android/AdbTarget.java
+++ b/src/vogar/android/AdbTarget.java
@@ -31,6 +31,9 @@
public final class AdbTarget extends Target {
+ private static final ImmutableList<String> TARGET_PROCESS_PREFIX =
+ ImmutableList.of("adb", "shell");
+
private final Log log;
private final DeviceFilesystem deviceFilesystem;
@@ -49,8 +52,8 @@
return new File("/data/local/tmp/vogar");
}
- @Override public List<String> targetProcessPrefix() {
- return ImmutableList.of("adb", "shell");
+ @Override protected ImmutableList<String> targetProcessPrefix() {
+ return TARGET_PROCESS_PREFIX;
}
@Override public void await(File directory) {
diff --git a/src/vogar/android/DeviceRuntime.java b/src/vogar/android/DeviceRuntime.java
index 5cac207..14e7841 100644
--- a/src/vogar/android/DeviceRuntime.java
+++ b/src/vogar/android/DeviceRuntime.java
@@ -84,16 +84,13 @@
@Override public VmCommandBuilder newVmCommandBuilder(Action action, File workingDirectory) {
List<String> vmCommand = new ArrayList<String>();
- vmCommand.addAll(run.target.targetProcessPrefix());
- vmCommand.add("cd");
- vmCommand.add(workingDirectory.getAbsolutePath());
- vmCommand.add("&&");
- vmCommand.add(run.getAndroidData());
Iterables.addAll(vmCommand, run.invokeWith());
vmCommand.add(run.vmCommand);
// If you edit this, see also HostRuntime...
VmCommandBuilder vmCommandBuilder = new VmCommandBuilder(run.log)
+ .env("ANDROID_DATA", run.getAndroidDataPath())
+ .workingDirectory(workingDirectory)
.vmCommand(vmCommand)
.vmArgs("-Duser.home=" + run.deviceUserHome)
.maxLength(1024);
diff --git a/src/vogar/android/HostRuntime.java b/src/vogar/android/HostRuntime.java
index 91ceae1..f5bb8ec 100644
--- a/src/vogar/android/HostRuntime.java
+++ b/src/vogar/android/HostRuntime.java
@@ -111,22 +111,21 @@
}
List<String> vmCommand = new ArrayList<String>();
- vmCommand.addAll(run.target.targetProcessPrefix());
- vmCommand.add("ANDROID_PRINTF_LOG=tag");
- vmCommand.add("ANDROID_LOG_TAGS=*:i");
- vmCommand.add("ANDROID_DATA=" + dalvikCache().getParent());
- vmCommand.add("ANDROID_ROOT=" + hostOut);
- vmCommand.add("LD_LIBRARY_PATH=" + libDir);
- vmCommand.add("DYLD_LIBRARY_PATH=" + libDir);
- // This is needed on the host so that the linker loads core.oat at the necessary address.
- vmCommand.add("LD_USE_LOAD_BIAS=1");
Iterables.addAll(vmCommand, run.invokeWith());
vmCommand.add(hostOut + "/bin/" + run.vmCommand);
- VmCommandBuilder builder = new VmCommandBuilder(run.log);
-
// If you edit this, see also DeviceRuntime...
- builder.vmCommand(vmCommand)
+ VmCommandBuilder builder = new VmCommandBuilder(run.log)
+ .env("ANDROID_PRINTF_LOG", "tag")
+ .env("ANDROID_LOG_TAGS", "*:i")
+ .env("ANDROID_DATA", dalvikCache().getParent())
+ .env("ANDROID_ROOT", hostOut)
+ .env("LD_LIBRARY_PATH", libDir)
+ .env("DYLD_LIBRARY_PATH", libDir)
+ // This is needed on the host so that the linker loads core.oat at the necessary
+ // address.
+ .env("LD_USE_LOAD_BIAS", "1")
+ .vmCommand(vmCommand)
.vmArgs("-Xbootclasspath:" + bootClasspath.toString())
.vmArgs("-Duser.language=en")
.vmArgs("-Duser.region=US");
diff --git a/src/vogar/commands/Command.java b/src/vogar/commands/Command.java
index a1b9b8a..bd347a1 100644
--- a/src/vogar/commands/Command.java
+++ b/src/vogar/commands/Command.java
@@ -17,6 +17,7 @@
package vogar.commands;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
@@ -55,7 +56,7 @@
public Command(Log log, String... args) {
this.log = log;
- this.args = processArgs(Arrays.asList(args));
+ this.args = ImmutableList.copyOf(args);
this.env = Collections.emptyMap();
this.permitNonZeroExitStatus = false;
this.tee = null;
@@ -63,7 +64,7 @@
private Command(Builder builder) {
this.log = builder.log;
- this.args = processArgs(builder.args);
+ this.args = ImmutableList.copyOf(builder.args);
this.env = builder.env;
this.permitNonZeroExitStatus = builder.permitNonZeroExitStatus;
this.tee = builder.tee;
@@ -76,27 +77,6 @@
}
}
- private static List<String> processArgs(List<String> args) {
- // Translate ["sh", "-c", "ls", "/tmp"] into ["sh", "-c", "ls /tmp"].
- // This is needed for host execution.
- ArrayList<String> actual = new ArrayList<String>();
- int i = 0;
- while (i < args.size()) {
- String arg = args.get(i++);
- actual.add(arg);
- if (arg.equals("-c")) break;
- }
- if (i < args.size()) {
- String cmd = "";
- for (; i < args.size(); ++i) {
- cmd += args.get(i) + " ";
- }
- actual.add(cmd);
- }
-
- return actual;
- }
-
public void start() throws IOException {
if (isStarted()) {
throw new IllegalStateException("Already started!");
diff --git a/src/vogar/commands/VmCommandBuilder.java b/src/vogar/commands/VmCommandBuilder.java
index e82befd..c532984 100644
--- a/src/vogar/commands/VmCommandBuilder.java
+++ b/src/vogar/commands/VmCommandBuilder.java
@@ -27,6 +27,9 @@
import java.util.Map;
import vogar.Classpath;
import vogar.Log;
+import vogar.Target;
+
+import static com.google.common.base.Preconditions.checkNotNull;
/**
* Builds a virtual machine command.
@@ -45,6 +48,7 @@
private List<String> vmCommand = Collections.singletonList("java");
private List<String> vmArgs = new ArrayList<String>();
private List<String> args = new ArrayList<String>();
+ private File workingDirectory;
private Map<String, String> env = new LinkedHashMap<String, String>();
public VmCommandBuilder(Log log) {
@@ -56,6 +60,14 @@
return this;
}
+ /**
+ * Set the working directory of the target process.
+ */
+ public VmCommandBuilder workingDirectory(File workingDirectory) {
+ this.workingDirectory = workingDirectory;
+ return this;
+ }
+
public VmCommandBuilder temp(File temp) {
this.temp = temp;
return this;
@@ -85,8 +97,11 @@
return this;
}
+ /**
+ * Add a setting for an environment variable in the target process.
+ */
public VmCommandBuilder env(String key, String value) {
- this.env.put(key, value);
+ env.put(key, value);
return this;
}
@@ -128,42 +143,50 @@
return this;
}
- public Command build() {
- Command.Builder builder = new Command.Builder(log);
+ public Command build(Target target) {
+ // Make sure that the main class to run has been specified.
+ checkNotNull(mainClass, "mainClass may not be null");
- for (Map.Entry<String, String> entry : env.entrySet()) {
- builder.env(entry.getKey(), entry.getValue());
+ Target.ScriptBuilder builder = target.newScriptBuilder();
+
+ if (workingDirectory != null) {
+ builder.workingDirectory(workingDirectory);
}
- builder.args(vmCommand);
+ builder.env(env);
+
+ builder.tokens(vmCommand);
if (classpathViaProperty) {
- builder.args("-Djava.class.path=" + classpath);
+ builder.tokens("-Djava.class.path=" + classpath);
} else {
- builder.args("-classpath", classpath.toString());
+ builder.tokens("-classpath", classpath.toString());
}
// Only output this if there's something on the boot classpath,
// otherwise dalvikvm gets upset.
if (!bootClasspath.isEmpty()) {
- builder.args("-Xbootclasspath/a:" + bootClasspath);
+ builder.tokens("-Xbootclasspath/a:" + bootClasspath);
}
if (userDir != null) {
- builder.args("-Duser.dir=" + userDir);
+ builder.tokens("-Duser.dir=" + userDir);
}
if (temp != null) {
- builder.args("-Djava.io.tmpdir=" + temp);
+ builder.tokens("-Djava.io.tmpdir=" + temp);
}
if (debugPort != null) {
- builder.args("-Xrunjdwp:transport=dt_socket,address="
+ builder.tokens("-Xrunjdwp:transport=dt_socket,address="
+ debugPort + ",server=y,suspend=y");
}
- builder.args(vmArgs);
- builder.args(mainClass);
- builder.args(args);
- builder.tee(output);
- builder.maxLength(maxLength);
- return builder.build();
+ builder.tokens(vmArgs);
+ builder.tokens(mainClass);
+ builder.tokens(args);
+
+ return new Command.Builder(log)
+ .args(builder.commandLine())
+ .tee(output)
+ .maxLength(maxLength)
+ .build();
}
}
diff --git a/src/vogar/tasks/RunActionTask.java b/src/vogar/tasks/RunActionTask.java
index c296c72..91e46d8 100644
--- a/src/vogar/tasks/RunActionTask.java
+++ b/src/vogar/tasks/RunActionTask.java
@@ -157,7 +157,7 @@
.vmArgs(run.additionalVmArgs)
.mainClass(TestRunner.class.getName())
.args(run.targetArgs)
- .build();
+ .build(run.target);
}
/**
diff --git a/test/vogar/AllTests.java b/test/vogar/AllTests.java
index 930554b..6ff9fd2 100644
--- a/test/vogar/AllTests.java
+++ b/test/vogar/AllTests.java
@@ -20,7 +20,8 @@
DeviceRuntimeAdbTargetTest.class,
DeviceRuntimeSshTargetTest.class,
HostRuntimeLocalTargetTest.class,
- JUnitRunnerTest.class
+ JUnitRunnerTest.class,
+ ScriptBuilderEscapingTest.class
})
@RunWith(Suite.class)
public class AllTests {
diff --git a/test/vogar/ScriptBuilderEscapingTest.java b/test/vogar/ScriptBuilderEscapingTest.java
new file mode 100644
index 0000000..6965251
--- /dev/null
+++ b/test/vogar/ScriptBuilderEscapingTest.java
@@ -0,0 +1,58 @@
+package vogar;
+
+import junit.framework.Test;
+import junit.framework.TestCase;
+import junit.framework.TestSuite;
+import vogar.Target.ScriptBuilder;
+
+/**
+ * Tests the {@link ScriptBuilder#escape(String)} method.
+ */
+public class ScriptBuilderEscapingTest {
+
+ public static Test suite() {
+ TestSuite suite = new TestSuite(ScriptBuilderEscapingTest.class.getName());
+ char[] chars = " '\"<>&|$".toCharArray();
+ for (char c : chars) {
+ suite.addTest(new SingleCharacterEscapeTest(c));
+ }
+ suite.addTest(new MixedCharacterEscapeTest());
+ return suite;
+ }
+
+ private static class SingleCharacterEscapeTest extends TestCase {
+
+ private final String uc;
+ private final String qc;
+
+ public SingleCharacterEscapeTest(char c) {
+ this.uc = Character.toString(c);
+ this.qc = "\\" + c;
+ setName("Escape '" + uc + "' as '" + qc + "'");
+ }
+
+ @Override
+ protected void runTest() throws Throwable {
+ assertEquals(qc, ScriptBuilder.escape(uc));
+ assertEquals("a" + qc, ScriptBuilder.escape("a" + uc));
+ assertEquals(qc + "b", ScriptBuilder.escape(uc + "b"));
+ assertEquals("a" + qc + "b", ScriptBuilder.escape("a" + uc + "b"));
+ assertEquals(qc + "a" + qc + qc + qc + "b" + qc,
+ ScriptBuilder.escape(uc + "a" + uc + uc + uc + "b" + uc));
+
+ }
+ }
+
+ private static class MixedCharacterEscapeTest extends TestCase {
+
+ public MixedCharacterEscapeTest() {
+ super("mixed character escape test");
+ }
+
+ @Override
+ protected void runTest() throws Throwable {
+ assertEquals("\\ \\'\\\"\\<\\>\\&\\|\\$",
+ ScriptBuilder.escape(" '\"<>&|$"));
+ }
+ }
+}
\ No newline at end of file
diff --git a/test/vogar/android/DeviceRuntimeAdbTargetTest.java b/test/vogar/android/DeviceRuntimeAdbTargetTest.java
index 9fffe43..c516113 100644
--- a/test/vogar/android/DeviceRuntimeAdbTargetTest.java
+++ b/test/vogar/android/DeviceRuntimeAdbTargetTest.java
@@ -60,23 +60,25 @@
classpath.addAll(new File("classes"));
VmCommandBuilder builder = newVmCommandBuilder(deviceRuntime)
.classpath(classpath)
- .mainClass("mainclass");
- Command command = builder.build();
+ .mainClass("mainclass")
+ .args("-x", "a b");
+ Command command = builder.build(run.target);
List<String> args = command.getArgs();
assertEquals(Arrays.asList(
- "adb", "shell",
- "cd", "/work",
- "&&",
- "ANDROID_DATA=runner",
- "dalvikvm32",
- "-classpath",
- "classes",
- "-Duser.home=runner/dir/user.home",
- "-Duser.name=fred",
- "-Duser.language=en",
- "-Duser.region=US",
- "-Xcheck:jni",
- "-Xjnigreflimit:2000",
- "mainclass"), args);
+ "adb", "shell", ""
+ + "cd /work"
+ + " &&"
+ + " ANDROID_DATA=runner"
+ + " dalvikvm32"
+ + " -classpath"
+ + " classes"
+ + " -Duser.home=runner/dir/user.home"
+ + " -Duser.name=fred"
+ + " -Duser.language=en"
+ + " -Duser.region=US"
+ + " -Xcheck:jni"
+ + " -Xjnigreflimit:2000"
+ + " mainclass"
+ + " -x a\\ b"), args);
}
}
diff --git a/test/vogar/android/DeviceRuntimeSshTargetTest.java b/test/vogar/android/DeviceRuntimeSshTargetTest.java
index b0ef77a..0878d8d 100644
--- a/test/vogar/android/DeviceRuntimeSshTargetTest.java
+++ b/test/vogar/android/DeviceRuntimeSshTargetTest.java
@@ -52,23 +52,25 @@
VmCommandBuilder builder = newVmCommandBuilder(deviceRuntime)
.classpath(classpath)
- .mainClass("mainclass");
- Command command = builder.build();
+ .mainClass("mainclass")
+ .args("-x", "a b");
+ Command command = builder.build(run.target);
List<String> args = command.getArgs();
assertEquals(Arrays.asList(
- "ssh", "-p", "99", "host", "-C",
- "cd", "/work",
- "&&",
- "ANDROID_DATA=runner",
- "dalvikvm32",
- "-classpath",
- "classes",
- "-Duser.home=runner/dir/user.home",
- "-Duser.name=fred",
- "-Duser.language=en",
- "-Duser.region=US",
- "-Xcheck:jni",
- "-Xjnigreflimit:2000",
- "mainclass"), args);
+ "ssh", "-p", "99", "host", "-C", ""
+ + "cd /work"
+ + " &&"
+ + " ANDROID_DATA=runner"
+ + " dalvikvm32"
+ + " -classpath"
+ + " classes"
+ + " -Duser.home=runner/dir/user.home"
+ + " -Duser.name=fred"
+ + " -Duser.language=en"
+ + " -Duser.region=US"
+ + " -Xcheck:jni"
+ + " -Xjnigreflimit:2000"
+ + " mainclass"
+ + " -x a\\ b"), args);
}
}
diff --git a/test/vogar/android/HostRuntimeLocalTargetTest.java b/test/vogar/android/HostRuntimeLocalTargetTest.java
index cad4392..5b9b659 100644
--- a/test/vogar/android/HostRuntimeLocalTargetTest.java
+++ b/test/vogar/android/HostRuntimeLocalTargetTest.java
@@ -55,8 +55,9 @@
classpath.addAll(new File("classes"));
VmCommandBuilder builder = newVmCommandBuilder(hostRuntime)
.classpath(classpath)
- .mainClass("mainclass");
- Command command = builder.build();
+ .mainClass("mainclass")
+ .args("-x", "a b");
+ Command command = builder.build(run.target);
List<String> args = command.getArgs();
assertEquals(Arrays.asList(
"sh", "-c", ""
@@ -78,6 +79,7 @@
+ " -Duser.region=US"
+ " -Xcheck:jni"
+ " -Xjnigreflimit:2000"
- + " mainclass "), args);
+ + " mainclass"
+ + " -x a\\ b"), args);
}
}
