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.

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

unquote: Fix possible out of bounds access in splitWord() under Windows #1

Closed
wants to merge 1 commit into from

Conversation

JoeKar
Copy link

@JoeKar JoeKar commented May 13, 2024

@JoeKar
Copy link
Author

JoeKar commented Jan 1, 2025

The commit with the functional change (https://github.com/JoeKar/go-shellquote/commit/51031e9ea27db3f7abbb545385be313a14939f05) is now cherry-picked into https://github.com/micro-editor/go-shellquote/tree/micro as micro-editor@feb6c39.

Now only the dependency must be changed in micro.

@JoeKar JoeKar closed this Jan 1, 2025
@JoeKar JoeKar deleted the fix/crash-windows-unquote branch January 1, 2025 11:07
}
buf.WriteString(string(escapeChar))
input = cur
goto raw
Copy link

Choose a reason for hiding this comment

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

Just for the record: I've been thinking that since essentially what we are doing here is making shellquote.Split() treat \ as a normal, non-special character in all cases (i.e. never escape with \) on Windows, it could be better to do that in a straightforward way, like:

diff --git a/unquote.go b/unquote.go
index c52f0c5..d5644d5 100644
--- a/unquote.go
+++ b/unquote.go
@@ -41,7 +41,7 @@ func Split(input string) (words []string, err error) {
 		if strings.ContainsRune(splitChars, c) {
 			input = input[l:]
 			continue
-		} else if c == escapeChar {
+		} else if c == escapeChar && os.PathSeparator != escapeChar {
 			// Look ahead for escaped newline so we can skip over it
 			next := input[l:]
 			if len(next) == 0 {
@@ -82,13 +82,8 @@ raw:
 				buf.WriteString(input[0 : len(input)-len(cur)-l])
 				input = cur
 				goto double
-			} else if c == escapeChar {
+			} else if c == escapeChar && os.PathSeparator != escapeChar {
 				buf.WriteString(input[0 : len(input)-len(cur)-l])
-				if os.PathSeparator == escapeChar {
-					buf.WriteString(string(escapeChar))
-					input = cur
-					goto raw
-				}
 				input = cur
 				goto escape
 			} else if strings.ContainsRune(splitChars, c) {

But that is troublesome, since we also use shellquote.Join() (in RunCmd()) which inserts \ to escape special characters, and then we call shellquote.Split() again for this escaped string. So with my above code, run "'" or run '"' won't work anymore on Windows.

So this PR's trick seems to be the best we can do for now.

@@ -85,12 +85,9 @@ raw:
} else if c == escapeChar {
buf.WriteString(input[0 : len(input)-len(cur)-l])
if os.PathSeparator == escapeChar {
next := rune(cur[0])
switch next {
case singleChar, doubleChar, escapeChar, 'n':
Copy link

Choose a reason for hiding this comment

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

Just for the record: so we sacrifice the possibility to use \ to escape single or double quotes on Windows. For example: open a new unnamed file, press Ctrl-S to save it, and in the Filename: prompt, type \". Result: without this PR the file is successfully saved as ", while with this PR we get Error parsing arguments: Unterminated double-quoted string.

I think it's ok, and it's actually better if \ behaves consistently, i.e. never escapes on Windows. And the user can still escape quotes in other ways: "'" and '"'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"cd a\" raises exception (Windows)
2 participants