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

expand: parameter expansions with spaces in the middle of words are mishandled #886

Closed
dkegel-fastly opened this issue Jun 25, 2022 · 6 comments · Fixed by #890
Closed

Comments

@dkegel-fastly
Copy link

dkegel-fastly commented Jun 25, 2022

It feels like gosh trims leading and trailing spaces from variables:

$ cat x.sh
#!/bin/sh
FOO=" Q "
echo a${FOO}b
export FOO
env | grep FOO | sed 's/$/$/'
$ sh x.sh
a Q b
FOO= Q $
$ go run main.go x.sh
aQb
FOO= Q $

I would have expected gosh x.sh and sh x.sh to produce identical output, but gosh outputs "aQb" while sh outputs "a Q b".

Looks like the variable has the spaces, but they are trimmed during variable expansion?

@dkegel-fastly
Copy link
Author

dkegel-fastly commented Jun 25, 2022

The problem can be reproduced with this unit test case in interp/interp_test.go:
[incorrect proposed test case deleted]

@dkegel-fastly
Copy link
Author

Not all expansions have the problem -- only unquoted ones that aren't on the right hand side of an assignment?

$ go run cmd/gosh/main.go -c 'FOO=" Q "; echo a${FOO}b'
aQb
$ go run cmd/gosh/main.go -c 'FOO=" Q "; echo a"${FOO}"b'
a Q b
$ go run cmd/gosh/main.go -c 'FOO=" Q "; BAR=a${FOO}b; echo $BAR'
a Q b

@dkegel-fastly
Copy link
Author

dkegel-fastly commented Jun 26, 2022

Shortest repro, perhaps:

$ export FOO=" Q "
$ go run cmd/gosh/main.go -c 'echo x${FOO}y'
xQy

@dkegel-fastly
Copy link
Author

So this rescues that one test case:

--- a/expand/expand.go
+++ b/expand/expand.go
@@ -617,7 +618,8 @@ func (cfg *Config) wordFields(wps []syntax.WordPart) ([][]fieldPart, error) {
                        if err != nil {
                                return nil, err
                        }
-                       splitAdd(val)
+                       curField = append(curField, fieldPart{val: val})
+                       //splitAdd(val)
                case *syntax.CmdSubst:
                        val, err := cfg.cmdSubst(x)
                        if err != nil {

It also breaks lots of other stuff.

@mvdan
Copy link
Owner

mvdan commented Jun 27, 2022

Your original bug report is valid - you correctly show that our interpreter prints something different than Bash. That does sound like a bug.

However, note that your second reproducer is invalid, as currently our interpreter outputs the same as Bash - note the lack of leading or trailing whitespace:

$ echo $0
/bin/bash
$ a=" foo_interp_missing "; echo $a
foo_interp_missing

I believe the bug exists when an unquoted parameter expansion exists in the middle of a word, like foo${bar}baz, and its value contains whitespace. Your attempted fix is in the right part of the codebase, though the correct fix might be more subtle than that :)

@mvdan mvdan changed the title Whitespace incorrectly trimmed from variables? expand: parameter expansions with spaces in the middle of words are mishandled Jun 27, 2022
@dkegel-fastly
Copy link
Author

I wouldn't call it an attempted fix, more like a depth charge :-)

mvdan added a commit that referenced this issue Jun 28, 2022
That is:

	$ a=" b c "
	$ echo foo${a}bar
	foo b c bar

rather than our previous incorrect answer:

	$ a=" b c "
	$ echo foo${a}bar
	foob cbar

Our error was relying on strings.FieldsFunc, which performs exactly as
documented, but is not enough for us - we need to know whether the
expanded string has leading or trailing IFS characters.

Fixes #886.
mvdan added a commit that referenced this issue Jun 29, 2022
That is:

	$ a=" b c "
	$ echo foo${a}bar
	foo b c bar

rather than our previous incorrect answer:

	$ a=" b c "
	$ echo foo${a}bar
	foob cbar

Our error was relying on strings.FieldsFunc, which performs exactly as
documented, but is not enough for us - we need to know whether the
expanded string has leading or trailing IFS characters.

Fixes #886.
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 a pull request may close this issue.

2 participants