Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Escape tilde more gracefully #16560

Closed
wants to merge 2 commits into from
Closed

Escape tilde more gracefully #16560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f45082
Escape tilde more gracefully
comius Oct 26, 2022
2bf8a60
Add tests with `/~`
comius Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/main/java/com/google/devtools/build/lib/util/ShellEscaper.java
View file
Open in desktop
Expand Up @@ -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.
Expand Down Expand Up @@ -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) + "'";
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/test/java/com/google/devtools/build/lib/util/ShellEscaperTest.java
View file
Open in desktop
Expand Up @@ -41,6 +41,9 @@ public void shellEscape() throws Exception {
assertThat(escapeString("\\'foo\\'")).isEqualTo("'\\'\\''foo\\'\\'''");
assertThat(escapeString("${filename%.c}.o")).isEqualTo("'${filename%.c}.o'");
assertThat(escapeString("<html!>")).isEqualTo("'<html!>'");
assertThat(escapeString("~not_home")).isEqualTo("'~not_home'");
assertThat(escapeString("external/protobuf~3.19.6/src/google")).isEqualTo("external/protobuf~3.19.6/src/google");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be safe, could you add a test case for external/~install_dev_dependencies~foo/pkg? This is a path that Bzlmod can generate and it's slightly more special as the tilde is the first character of a path segment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a unit tests, but this doesn't tell us much. I tested manually:

~$ mkdir tmp
~$ mkdir tmp/~tilde
~$ cd tmp/~tilde/
~/tmp/~tilde$ cd ..
~/tmp$ cd ~tilde
bash: cd: /usr/local/google/home/tilde: No such file or directory
i~/tmp$ cd '~tilde'
~/tmp/~tilde$

Copy link
Collaborator

@fmeum fmeum Oct 26, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Luckily, I don't think that Bazel will generate paths where the first segment starts with a tilde in any situation. At the moment that is, I think that this may change with --experimental_sibling_repository_layout, where paths can start with repository names. These paths would not work without escaping.

@Wyverald Do you see other cases where this could be problematic?

Edit: This actually means that the output of $(rlocationpath //label) from #16428 may end up being escaped.

@Wyverald Could we ensure that repositories generated by the main module are prefixed with _main~ rather than just ~? That would probably solve this problem for good, even with --experimental_sibling_repository_layout.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bash seems to consider paths starting with '~' special: https://www.gnu.org/software/bash/manual/html_node/Tilde-Expansion.html

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#16564 should ensure that paths emitted by Bazel never start with ~.

assertThat(escapeString("external/~install_dev_dependencies~foo/pkg")).isEqualTo("external/~install_dev_dependencies~foo/pkg");
}

@Test
Expand Down