Skip to content

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

Closed
JoeKar wants to merge 1 commit intozyedidia:masterfrom
JoeKar:fix/crash-windows-unquote
Closed

unquote: Fix possible out of bounds access in splitWord() under Windows#1
JoeKar wants to merge 1 commit intozyedidia:masterfrom
JoeKar:fix/crash-windows-unquote

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.

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