From 7f4508255ce4a10859f354f06e18fa50c2c6c134 Mon Sep 17 00:00:00 2001 From: Ivo List Date: Wed, 26 Oct 2022 14:46:22 +0200 Subject: [PATCH 1/2] Escape tilde more gracefully Tilde needs to be escaped only when it's the first character after the white space. Otherwise, we can keep the string unescaped. This supports bzlmod better, because tilde is used in the directory names. Users often already escape location function, for example `SJ="$(location @bazel_tools//tools/jdk:singlejar)"; $SJ`. Without the change this becomes ``"'external/repo~name/singlejar'"` and the script fails (no file found). Location function shouldn't be escaped. But without this change we risk a lot of users will need to fix their scripts. --- .../google/devtools/build/lib/util/ShellEscaper.java | 12 +++++++++--- .../devtools/build/lib/util/ShellEscaperTest.java | 2 ++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/util/ShellEscaper.java b/src/main/java/com/google/devtools/build/lib/util/ShellEscaper.java index d54bbdaf8e5c0f..121fbb8284006d 100644 --- a/src/main/java/com/google/devtools/build/lib/util/ShellEscaper.java +++ b/src/main/java/com/google/devtools/build/lib/util/ShellEscaper.java @@ -63,6 +63,8 @@ public final class ShellEscaper extends Escaper { .or(CharMatcher.inRange('a', 'z')) // that would also accept non-ASCII digits and .or(CharMatcher.inRange('A', 'Z')) // letters. .precomputed(); + private static final CharMatcher SAFECHAR_MATCHER_WITH_TILDE = + SAFECHAR_MATCHER.or(CharMatcher.is('~')).precomputed(); /** * Escapes a string by adding strong (single) quotes around it if necessary. @@ -98,9 +100,13 @@ public String escape(String unescaped) { // gets treated as a separate argument. return "''"; } else { - return SAFECHAR_MATCHER.matchesAllOf(s) - ? s - : "'" + STRONGQUOTE_ESCAPER.escape(s) + "'"; + if (SAFECHAR_MATCHER.matchesAllOf(s)) { + return s; + } + if (SAFECHAR_MATCHER_WITH_TILDE.matchesAllOf(s) && s.charAt(0) != '~') { + return s; + } + return "'" + STRONGQUOTE_ESCAPER.escape(s) + "'"; } } diff --git a/src/test/java/com/google/devtools/build/lib/util/ShellEscaperTest.java b/src/test/java/com/google/devtools/build/lib/util/ShellEscaperTest.java index cad9ce53f24add..98cbc49a4ecb80 100644 --- a/src/test/java/com/google/devtools/build/lib/util/ShellEscaperTest.java +++ b/src/test/java/com/google/devtools/build/lib/util/ShellEscaperTest.java @@ -41,6 +41,8 @@ public void shellEscape() throws Exception { assertThat(escapeString("\\'foo\\'")).isEqualTo("'\\'\\''foo\\'\\'''"); assertThat(escapeString("${filename%.c}.o")).isEqualTo("'${filename%.c}.o'"); assertThat(escapeString("")).isEqualTo("''"); + assertThat(escapeString("~not_home")).isEqualTo("'~not_home'"); + assertThat(escapeString("external/protobuf~3.19.6/src/google")).isEqualTo("external/protobuf~3.19.6/src/google"); } @Test From 2bf8a607b4cba78b55e24840d58b0fc79a3f168a Mon Sep 17 00:00:00 2001 From: Ivo List Date: Wed, 26 Oct 2022 16:08:22 +0200 Subject: [PATCH 2/2] Add tests with `/~` --- .../com/google/devtools/build/lib/util/ShellEscaperTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/com/google/devtools/build/lib/util/ShellEscaperTest.java b/src/test/java/com/google/devtools/build/lib/util/ShellEscaperTest.java index 98cbc49a4ecb80..05c5cc676e707a 100644 --- a/src/test/java/com/google/devtools/build/lib/util/ShellEscaperTest.java +++ b/src/test/java/com/google/devtools/build/lib/util/ShellEscaperTest.java @@ -43,6 +43,7 @@ public void shellEscape() throws Exception { assertThat(escapeString("")).isEqualTo("''"); assertThat(escapeString("~not_home")).isEqualTo("'~not_home'"); assertThat(escapeString("external/protobuf~3.19.6/src/google")).isEqualTo("external/protobuf~3.19.6/src/google"); + assertThat(escapeString("external/~install_dev_dependencies~foo/pkg")).isEqualTo("external/~install_dev_dependencies~foo/pkg"); } @Test