| Message ID | 20200514210518.56101-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t0300: workaround bug in FreeBSD < 10 sh | expand |
On Thu, May 14, 2020 at 02:05:18PM -0700, Carlo Marcelo Arenas Belón wrote: > 4c5971e18a (credential: treat "?" and "#" in URLs as end of host, > 2020-04-14) introduces check_host_and_path to t0300 and some tests that > use it, but fail in at least FreeBSD 9.3. > > The variables in the here-doc fail to be expanded until they are used as > part of the eval in check(), resulting in (ex: url=fill) instead of what > was expected. Wow, that's very surprising. Just to be clear, if you run: foo() { for i in "$@"; do echo "arg:$i" done sed s/^/stdin:/ } set -- outer foo inner <<EOF $1 EOF do you get: arg:inner stdin:inner ? (on dash and bash, I get stdin:outer as expected). I don't think the fact that check() uses eval() should matter, because we'd be interpreting that here-doc earlier as part of read_chunk(). > While at it, make sure all of the parameters which potentially sensitive > characters (ex: ?#), are quote protected. I don't mind more quoting, but... > -test_expect_success 'url parser handles bare query marker' ' > - check_host_and_path https://example.com?foo.git example.com ?foo.git > -' > +test_expect_success 'url parser handles bare query marker' " > + check_host_and_path 'https://example.com?foo.git' \ > + example.com '?foo.git' > +" ...please don't invert the double and single quotes. In either case the metacharacter is subject to double-quote expansion, and by putting the double-quotes on the outside, you've left a trap for somebody adding more lines to the test (the shell snippet will now be interpolated before being eval'd, which is contrary to how most of our tests run). -Peff
On Thu, May 14, 2020 at 6:03 PM Jeff King <peff@peff.net> wrote: > Just to be clear, if you run: > > foo() { > for i in "$@"; do > echo "arg:$i" > done > sed s/^/stdin:/ > } > set -- outer > foo inner <<EOF > $1 > EOF > > do you get: > > arg:inner > stdin:inner That is indeed the result with the FreeBSD 9.3 shell. Seems buggy. Assigning $1, $2, ... to variables (local or not) as Carlo's patch does seems to work around the buggy behavior.
On 2020-05-14 at 21:05:18, Carlo Marcelo Arenas Belón wrote: > 4c5971e18a (credential: treat "?" and "#" in URLs as end of host, > 2020-04-14) introduces check_host_and_path to t0300 and some tests that > use it, but fail in at least FreeBSD 9.3. From FreeBSD's website, it looks like the production releases are 11.3 and 12.1. 9.3 is EOL and has been since 2019. Since FreeBSD is not supporting this release with security updates, nobody should be using it. In light of that, do we need this patch?
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2020-05-14 at 21:05:18, Carlo Marcelo Arenas Belón wrote: >> 4c5971e18a (credential: treat "?" and "#" in URLs as end of host, >> 2020-04-14) introduces check_host_and_path to t0300 and some tests that >> use it, but fail in at least FreeBSD 9.3. > > From FreeBSD's website, it looks like the production releases are 11.3 > and 12.1. 9.3 is EOL and has been since 2019. Since FreeBSD is not > supporting this release with security updates, nobody should be using > it. In light of that, do we need this patch? Perfect comment given on the day I am cutting -rc0---the fewer patches I need to worry about, the better ;-).
On Thu, May 14, 2020 at 06:03:46PM -0400, Jeff King wrote: > On Thu, May 14, 2020 at 02:05:18PM -0700, Carlo Marcelo Arenas Belón wrote: > > > 4c5971e18a (credential: treat "?" and "#" in URLs as end of host, > > 2020-04-14) introduces check_host_and_path to t0300 and some tests that > > use it, but fail in at least FreeBSD 9.3. > > > > The variables in the here-doc fail to be expanded until they are used as > > part of the eval in check(), resulting in (ex: url=fill) instead of what > > was expected. > > Wow, that's very surprising. And luckily, also a problem that is no longer present with newer versions of the shell and neither NetBSD's or OpenBSD's AFAIK. > Just to be clear, if you run: > > foo() { > for i in "$@"; do > echo "arg:$i" > done > sed s/^/stdin:/ > } > set -- outer > foo inner <<EOF > $1 > EOF > > do you get: > > arg:inner > stdin:inner correct, so the problem was in the here-doc, not the eval; thanks for your explanation and reproduction, will update it in the reroll. $ foo() { > for i in "$@"; do > echo "arg:$i" > done > sed s/^/stdin:/ > } $ set -- outer $ foo inner <<EOF > $1 > EOF arg:inner stdin:inner > ? (on dash and bash, I get stdin:outer as expected). I don't think the > fact that check() uses eval() should matter, because we'd be > interpreting that here-doc earlier as part of read_chunk(). indeed, apologies for the extra quoting which was part of my original debugging and wasn't really needed. will keep it without the inversion, only because I think it looks also clearer with the added indentantion. Carlo
On Thu, May 14, 2020 at 04:04:39PM -0700, Junio C Hamano wrote: > "brian m. carlson" <sandals@crustytoothpaste.net> writes: > > > On 2020-05-14 at 21:05:18, Carlo Marcelo Arenas Belón wrote: > >> 4c5971e18a (credential: treat "?" and "#" in URLs as end of host, > >> 2020-04-14) introduces check_host_and_path to t0300 and some tests that > >> use it, but fail in at least FreeBSD 9.3. > > > > From FreeBSD's website, it looks like the production releases are 11.3 > > and 12.1. 9.3 is EOL and has been since 2019. Since FreeBSD is not > > supporting this release with security updates, nobody should be using > > it. In light of that, do we need this patch? Not urgently, and my rationale was that it might get reverted and dropped in the near future, like we did for those workarounds for defunct versions of dash. Which is why it was clearly marked as a bug and the version it affected. FWIW there are some people still building git in macOS 10.5 (the last with powerpc support) that was EOL in 2011. > Perfect comment given on the day I am cutting -rc0---the fewer > patches I need to worry about, the better ;-). I was considering it will be worth pu (in case someone might need to pull it), but definitely not needed in master for this release. Carlo
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index bc2d74098f..e0b7c001f1 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -622,37 +622,43 @@ test_expect_success 'credential system refuses to work with missing protocol' ' # usage: check_host_and_path <url> <expected-host> <expected-path> check_host_and_path () { + local url=$1 + local host=$2 + local path=$3 + # we always parse the path component, but we need this to make sure it # is passed to the helper test_config credential.useHTTPPath true && - check fill "verbatim user pass" <<-EOF - url=$1 + check fill 'verbatim user pass' <<-EOF + url=$url -- protocol=https - host=$2 - path=$3 + host=$host + path=$path username=user password=pass -- verbatim: get verbatim: protocol=https - verbatim: host=$2 - verbatim: path=$3 + verbatim: host=$host + verbatim: path=$path EOF } -test_expect_success 'url parser handles bare query marker' ' - check_host_and_path https://example.com?foo.git example.com ?foo.git -' +test_expect_success 'url parser handles bare query marker' " + check_host_and_path 'https://example.com?foo.git' \ + example.com '?foo.git' +" -test_expect_success 'url parser handles bare fragment marker' ' - check_host_and_path https://example.com#foo.git example.com "#foo.git" -' +test_expect_success 'url parser handles bare fragment marker' " + check_host_and_path 'https://example.com#foo.git' \ + example.com '#foo.git' +" -test_expect_success 'url parser not confused by encoded markers' ' - check_host_and_path https://example.com%23%3f%2f/foo.git \ - "example.com#?/" foo.git -' +test_expect_success 'url parser not confused by encoded markers' " + check_host_and_path 'https://example.com%23%3f%2f/foo.git' \ + 'example.com#?/' foo.git +" test_expect_success 'credential config with partial URLs' ' echo "echo password=yep" | write_script git-credential-yep &&
4c5971e18a (credential: treat "?" and "#" in URLs as end of host, 2020-04-14) introduces check_host_and_path to t0300 and some tests that use it, but fail in at least FreeBSD 9.3. The variables in the here-doc fail to be expanded until they are used as part of the eval in check(), resulting in (ex: url=fill) instead of what was expected. Transfer the 3 parameters to local variables which will be used instead and that will be in scope without conflicting names. While at it, make sure all of the parameters which potentially sensitive characters (ex: ?#), are quote protected. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- t/t0300-credentials.sh | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-)