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

OSH: Incompatible behavior of eval "sed -n \"\$script\"" #1449

Closed
zackw opened this issue Jan 13, 2023 · 7 comments
Closed

OSH: Incompatible behavior of eval "sed -n \"\$script\"" #1449

zackw opened this issue Jan 13, 2023 · 7 comments

Comments

@zackw
Copy link

zackw commented Jan 13, 2023

Autoconf uses a construct like

sedinputs="file1 file2 file3"
sedscript='
   ... several lines of sed commands here ...
'
case `eval "sed -n \"\$sedscript\" $sedinputs"` in 
   ... patterns ...
esac

at one point in its generated config.status files. With both bash and dash, the effect of the eval is to word-split the expansion of sedinputs but not word-split the expansion of sedscript. (It's not obvious to me why the eval is necessary here, but this is the code used in the wild for many years now.)

With osh 0.13.1, on the other hand, the expansion of sedscript is word-split, which causes sed to throw an error because its first command-line argument is not a complete sed script by itself.

I don't know what, if anything, POSIX specifies here, but it seems to me that the bash/dash behavior is what a human would expect.

Complete reproducer script is attached. With bash or dash, the output is ok and exit code 0. With osh the output is

sed: -e expression #1, char 13: missing command
fail

and exit code 1.

improper-word-splitting-bug.sh.gz

@zackw zackw changed the title Incompatible behavior of eval "sed -n \"\$script\"" OSH: Incompatible behavior of eval "sed -n \"\$script\"" Jan 13, 2023
andychu pushed a commit that referenced this issue Jan 14, 2023
The \" is somehow getting lost with backtick command sub, but not $()
@andychu
Copy link
Contributor

andychu commented Jan 14, 2023

OK I reproduced this -- it's actually not an IFS bug as I initially suspected

With $(), the equivalent command is parsed and evaluated consistently with dash / bash

But NOT with backticks. It appears it's somehow losing the \" with backticks. (Backticks are a tricky case of dynamic parsing, i.e. it didn't quite fit our model)

I will look at this -- thanks for the report!

andychu pushed a commit that referenced this issue Jan 14, 2023
andychu pushed a commit that referenced this issue Jan 14, 2023
The Oil parsing model is smart enough to parse double quotes inside
backticks inside double quotes:

    echo "x `echo "hi"`"

Shells aren't, and use this syntax instead:

    echo "x `echo \"hi\"`"

This is issue #1449.
@andychu
Copy link
Contributor

andychu commented Jan 14, 2023

OK just fixed this! Fundamental issue:

The Oil parsing model is smart enough to parse double quotes inside
backticks inside double quotes, with no extra :

echo "x `echo "hi"`"

Shells aren't, and use this syntax instead:

echo "x `echo \"hi\"`"

@zackw
Copy link
Author

zackw commented Jan 14, 2023

You might be interested to know that as recently as May of 2021 there were still people who cared very much about Autoconf and related tooling continuing to support Solaris 10 /bin/sh, which, among other serious limitations, does not implement $(...) command substitution. See for instance the discussions archived at https://lists.gnu.org/archive/html/config-patches/2021-05/threads.html.

(As far as I know, Solaris 10 is the sole surviving member of the set of proprietary Unixes that froze their /bin/sh implementation as it was prior to implementing the "Unix95" -- yes, 1995 -- spec.)

(I have a plan to migrate most of autoconf's code over to $(...) anyway, but it's not a high priority.)

@zackw
Copy link
Author

zackw commented Jan 15, 2023

Anyway, thanks for the quick fix! Can I just point out, though, that the code from my original message did not have " nested inside ` inside ". Instead it had two levels of " quotation inside backticks. Your test case appears only to test " inside ` inside ".

So I'd like to propose this additional test case. It passes with bash 5.2.15(1)-release and mksh 'MIRBSD KSH R59 2022/12/01', but the last two subtests fail with osh 0.13.1.

#! /bin/sh
set -e

nargs () {
    n_args=$#
    return 0
}

check () {
    if [ "$1" = "$2" ]; then
        printf >&2 'ok: %s\n' "$3"
    else
        printf >&2 'FAIL: %s: got "%s" expected "%s"\n' "$3" "$1" "$2"
        status=1
    fi
}

a="a b"

nargs $a
check $n_args 2 'nargs $a'

nargs "$a"
check $n_args 1 'nargs "$a"'

nargs "$a" $a
check $n_args 3 'nargs "$a" $a'

n_args=`nargs "$a"; echo $n_args`
check $n_args 1 '`nargs "$a"`'

n_args=`nargs "$a" $a; echo $n_args`
check $n_args 3 '`nargs "$a" $a`'

eval "nargs \"$a\""
check $n_args 1 'eval "nargs \"$a\""'

eval "nargs \"$a\" $a"
check $n_args 3 'eval "nargs \"$a\" $a"'

n_args=`eval "nargs \"$a\""; echo $n_args`
check $n_args 1 'eval "nargs \"$a\""'

n_args=`eval "nargs \"$a\" $a"; echo $n_args`
check $n_args 3 'eval "nargs \"$a\" $a"'

exit $status

andychu pushed a commit that referenced this issue Jan 15, 2023
Run like this:

    test/gold.sh test-autoconf-backtick

And then see _tmp/shebang.txt and _tmp/osh.txt.  (This reminds me the
test/gold.sh harness is a little wonky)
@andychu
Copy link
Contributor

andychu commented Jan 15, 2023

OK, all of that works at HEAD now:

a80f56d

Run like this:

test/gold.sh test-autoconf-backtick

And then see _tmp/shebang.txt and _tmp/osh.txt. (This reminds me the
test/gold.sh harness is a little wonky)


Normally those kinds of tests would go in the spec/ folder, but I have this other harness that doesn't require writing assertions.

It just runs scripts with the shebang line and OSH, and compares the stdout and exit code

@andychu
Copy link
Contributor

andychu commented Jan 15, 2023

Feel free to add more test cases! I can help with the mechanics of that on Zulip

Tests are all published with each release:

https://www.oilshell.org/release/0.13.1/test/spec.wwz/survey/osh.html

https://www.oilshell.org/release/0.13.1/test/other.wwz/gold.txt


Also I definitely dislike backticks, but we're resigned to deal with them as every other shell does ... :-P

(It's possible there are still divergences, since our parsing model is different. We don't look for matching quotes first -- we parse everything up front in a single pass. OSH started out mostly as a parsing experiment)

I've been through a few iterations of "concede to reality" so it's OK :)

@andychu
Copy link
Contributor

andychu commented Feb 1, 2023

Released last week -- https://www.oilshell.org/release/0.14.0/

I would be very interested in other bugs from autoconf!

I have a plan for #1450 , but it would be nice to get a bunch of bugs at once

How long do the auto test suites take to run?

@andychu andychu closed this as completed Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants