Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 07:15:45PM -0700, Elijah Newren wrote:
> Crazy idea: maybe we could defang it a little more thoroughly with
> something like the following (apologies in advance if gmail whitespace
> damages this):
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 28315706be..7fda08a90a 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -675,7 +675,7 @@ test_run_ () {
> trace=
> # 117 is magic because it is unlikely to match the exit
> # code of other programs
> - if test "OK-117" != "$(test_eval_ "(exit 117) &&
> $1${LF}${LF}echo OK-\$?" 3>&1)"
> + if test "OK-117" != "$(test_eval_ "cd() { return 0; }
> && PATH=/dev/null && export PATH && (exit 117) && $1${LF}${LF}echo
> OK-\$?" 3>&1)"
Clever. We'd still run shell builtins, which is why you need the cd()
above. There may be others, but at least it narrows things down. Unless
the shell is busybox or something, and implements everything as a
builtin. :)
I agree on the point elsewhere of returning non-zero (and the items
missing from PATH should do that, which is good).
-Peff
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 05:13:05PM -0400, Eric Sunshine wrote:
> On Tue, Jun 26, 2018 at 5:01 PM Jeff King wrote:
> > On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
> > > Some of these dangers can be de-thoothed during the linting phase by
> > > defining do-nothing shell functions:
> > >
> > > cp () { :; }
> > > mv () { :; }
> > > ln () { :; }
> > >
> > > That, at least, makes the scariest case ("rm") much less so.
> >
> > Now that's an interesting idea. We can't catch every dangerous action
> > (notably ">" would be hard to override), but it should be pretty cheap
> > to cover some obvious ones.
>
> Taking the idea a bit further, the 'sed' script could also throw away
> strings of "../" inside subshells, which would help defang the more
> difficult cases, like "echo x >../git.c". There are pathological
> cases, of course, which it wouldn't catch:
>
> P=../git.c
> test_expect_success 'foo' '
> (
> cd dir &&
> echo x >$P
> )
> '
>
> but it does help mitigate the issue for the most typical cases.
It seems like the dangerous thing there is ">", not necessarily "..".
Could we just s/>/x/g ?
That "breaks" the commands in a sense, but the whole point is that these
commands shouldn't ever be run in the first place.
-Peff
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Wed, Jun 27, 2018 at 2:27 AM Johannes Sixt wrote:
> Am 27.06.2018 um 04:15 schrieb Elijah Newren:
> > On Tue, Jun 26, 2018 at 2:01 PM, Jeff King wrote:
> >> On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
> >>> Some of these dangers can be de-thoothed during the linting phase by
> >>> defining do-nothing shell functions:
> >>> cp () { :; }
> >>> That, at least, makes the scariest case ("rm") much less so.
> >>
> >> Now that's an interesting idea. We can't catch every dangerous action
> >> (notably ">" would be hard to override), but it should be pretty cheap
> >> to cover some obvious ones.
> >
> > Crazy idea: maybe we could defang it a little more thoroughly with
> > something like the following (apologies in advance if gmail whitespace
> > damages this):
> >
> > - if test "OK-117" != "$(test_eval_ "(exit 117) &&
> > $1${LF}${LF}echo OK-\$?" 3>&1)"
> > + if test "OK-117" != "$(test_eval_ "cd() { return 0; }
> > && PATH=/dev/null && export PATH && (exit 117) && $1${LF}${LF}echo
> > OK-\$?" 3>&1)"
Interesting idea (coupled with Hannes's point below)...
> I'd define all these functions as { return 1; } because we want to stop
> any && chain as early as possible (and with an exit code that is not the
> sentinel value).
A very sensible suggestion.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Am 27.06.2018 um 04:15 schrieb Elijah Newren:
On Tue, Jun 26, 2018 at 2:01 PM, Jeff King wrote:
On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
Some of these dangers can be de-thoothed during the linting phase by
defining do-nothing shell functions:
cp () { :; }
mv () { :; }
ln () { :; }
That, at least, makes the scariest case ("rm") much less so.
Now that's an interesting idea. We can't catch every dangerous action
(notably ">" would be hard to override), but it should be pretty cheap
to cover some obvious ones.
-Peff
Crazy idea: maybe we could defang it a little more thoroughly with
something like the following (apologies in advance if gmail whitespace
damages this):
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 28315706be..7fda08a90a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -675,7 +675,7 @@ test_run_ () {
trace=
# 117 is magic because it is unlikely to match the exit
# code of other programs
- if test "OK-117" != "$(test_eval_ "(exit 117) &&
$1${LF}${LF}echo OK-\$?" 3>&1)"
+ if test "OK-117" != "$(test_eval_ "cd() { return 0; }
&& PATH=/dev/null && export PATH && (exit 117) && $1${LF}${LF}echo
OK-\$?" 3>&1)"
then
error "bug in the test script: broken &&-chain
or run-away HERE-DOC: $1"
fi
I'd define all these functions as { return 1; } because we want to stop
any && chain as early as possible (and with an exit code that is not the
sentinel value).
-- Hannes
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 2:01 PM, Jeff King wrote:
> On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
>> Some of these dangers can be de-thoothed during the linting phase by
>> defining do-nothing shell functions:
>>
>> cp () { :; }
>> mv () { :; }
>> ln () { :; }
>>
>> That, at least, makes the scariest case ("rm") much less so.
>
> Now that's an interesting idea. We can't catch every dangerous action
> (notably ">" would be hard to override), but it should be pretty cheap
> to cover some obvious ones.
>
> -Peff
Crazy idea: maybe we could defang it a little more thoroughly with
something like the following (apologies in advance if gmail whitespace
damages this):
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 28315706be..7fda08a90a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -675,7 +675,7 @@ test_run_ () {
trace=
# 117 is magic because it is unlikely to match the exit
# code of other programs
- if test "OK-117" != "$(test_eval_ "(exit 117) &&
$1${LF}${LF}echo OK-\$?" 3>&1)"
+ if test "OK-117" != "$(test_eval_ "cd() { return 0; }
&& PATH=/dev/null && export PATH && (exit 117) && $1${LF}${LF}echo
OK-\$?" 3>&1)"
then
error "bug in the test script: broken &&-chain
or run-away HERE-DOC: $1"
fi
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 5:33 PM Elijah Newren wrote: > On Tue, Jun 26, 2018 at 1:22 PM, Jeff King wrote: > > Another option is to not enable this slightly-more-dangerous linting by > > default. But that would probably rob it of its usefulness, since it > > would just fall to some brave soul to later crank up the linting and fix > > everybody else's mistakes. > > This may be a dumb question, but why can't we run under errexit? If > we could do that, we wouldn't need the &&-chaining, and bash would > parse the shell for us and exit whenever one command failed. (Is the > reason for this documented somewhere? I couldn't find it...) I'm not sure if it's documented anywhere, but it has been discussed. In particular, see [1], especially [2], and [3]. Peff summed up by saying: So I dunno. I think "set -e" is kind of a dangerous lure. It works so well _most_ of the time that you start to rely on it, but it really does have some funny corner cases (even on modern shells, and for all I know, the behavior above is mandated by POSIX). [1]: https://public-inbox.org/git/xmqq384zha6s@gitster.dls.corp.google.com/ [2]: https://public-inbox.org/git/20150320172406.ga15...@peff.net/ [3]: https://public-inbox.org/git/xmqqoannfu84@gitster.dls.corp.google.com/
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 1:22 PM, Jeff King wrote: > On Tue, Jun 26, 2018 at 04:17:08PM -0400, Jeff King wrote: > >> I'm not sure if there's a good solution, though. Even if you retained >> the subshells and instead did a chain-lint inside each subshell, like >> this: > > So obviously that means "I don't think there's a good solution with this > approach". > > That whole final patch simultaneously impresses and nauseates me. Your > commit message says "no attempt is made at properly parsing shell code", > but we come pretty darn close. I almost wonder if we'd be better off > just parsing some heuristic subset and making sure (via review or > linting) that our tests conform. > > Another option is to not enable this slightly-more-dangerous linting by > default. But that would probably rob it of its usefulness, since it > would just fall to some brave soul to later crank up the linting and fix > everybody else's mistakes. This may be a dumb question, but why can't we run under errexit? If we could do that, we wouldn't need the &&-chaining, and bash would parse the shell for us and exit whenever one command failed. (Is the reason for this documented somewhere? I couldn't find it...)
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 5:01 PM Jeff King wrote:
> On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
> > Some of these dangers can be de-thoothed during the linting phase by
> > defining do-nothing shell functions:
> >
> > cp () { :; }
> > mv () { :; }
> > ln () { :; }
> >
> > That, at least, makes the scariest case ("rm") much less so.
>
> Now that's an interesting idea. We can't catch every dangerous action
> (notably ">" would be hard to override), but it should be pretty cheap
> to cover some obvious ones.
Taking the idea a bit further, the 'sed' script could also throw away
strings of "../" inside subshells, which would help defang the more
difficult cases, like "echo x >../git.c". There are pathological
cases, of course, which it wouldn't catch:
P=../git.c
test_expect_success 'foo' '
(
cd dir &&
echo x >$P
)
'
but it does help mitigate the issue for the most typical cases.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Jeff King writes: > One way this series might be worse in practice is that we tend not to > change process state too much outside of the subshells. > ... > Whereas once you start collapsing subshells into the main logic chain, > there's a very high chance that the subshell is doing a "cd", since > that's typically the main reason for the subshell in the first place. Exactly. I should have mentioned this when I responded to save a round-trip.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
> > I'm not sure if there's a good solution, though. Even if you retained
> > the subshells and instead did a chain-lint inside each subshell, like
> > this:
> >
> > (exit 117) &&
> > one &&
> > (
> > (exit 117) &&
> > cd foo
> > two
> > ) &&
> > three
>
> I thought of that too, but the inner (exit 117) doesn't even get
> invoked unless there is &&-chain breakage somewhere above that point
> (for instance, if "one" lacks "&&"), so the inner (exit 117) doesn't
> participate in the linting process at all.
Oh, right. Not only does it not fix the problem, it's totally
unworkable. :)
> Some of these dangers can be de-thoothed during the linting phase by
> defining do-nothing shell functions:
>
> cp () { :; }
> mv () { :; }
> ln () { :; }
>
> That, at least, makes the scariest case ("rm") much less so.
Now that's an interesting idea. We can't catch every dangerous action
(notably ">" would be hard to override), but it should be pretty cheap
to cover some obvious ones.
-Peff
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 4:22 PM Jeff King wrote: > So obviously that means "I don't think there's a good solution with this > approach". > > That whole final patch simultaneously impresses and nauseates me. Your > commit message says "no attempt is made at properly parsing shell code", > but we come pretty darn close. I almost wonder if we'd be better off > just parsing some heuristic subset and making sure (via review or > linting) that our tests conform. I'm not sure I agree with "come pretty darn close", but your idea is an interesting one. It would sidestep the concern with "rm -fr" and friends (though it will probably still nauseate you). Let me cogitate about it a bit... > Another option is to not enable this slightly-more-dangerous linting by > default. But that would probably rob it of its usefulness, since it > would just fall to some brave soul to later crank up the linting and fix > everybody else's mistakes. I considered that, as well, and came to the same conclusion.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 4:17 PM Jeff King wrote:
> On Tue, Jun 26, 2018 at 03:52:54PM -0400, Eric Sunshine wrote:
> > So, this isn't a new problem introduced by this series, though this
> > series may exacerbate it.
>
> Whereas once you start collapsing subshells into the main logic chain,
> there's a very high chance that the subshell is doing a "cd", since
> that's typically the main reason for the subshell in the first place.
> And with the current --chain-lint logic, that subshell is either
> executed or not executed as a unit.
>
> Obviously that's a bit of a hand-waving argument. If you've fixed all of
> the existing cases without accidentally deleting your home directory,
> then maybe it's not so likely to be a problem after all.
Indeed, it could be that the "rm -fr" worry is tending toward the
hypothetical. Seasoned developers tend to be pretty careful and
usually avoid indiscriminately loose "rm -fr" invocations, so I'm
somewhat less worried about them. I do share the concern, though, that
newcomers crafting or extending tests could shoot themselves in the
foot with this. However, newcomers are also the ones most likely to
use the "cd foo && bar && cd .." idiom, so they are already at risk.
(As for not blasting my home directory when fixing all the existing
tests, I did run into a few cases where one or two "foreign" files
were deposited into the "t/" directory, but nothing was deleted or
overwritten.)
> I'm not sure if there's a good solution, though. Even if you retained
> the subshells and instead did a chain-lint inside each subshell, like
> this:
>
> (exit 117) &&
> one &&
> (
> (exit 117) &&
> cd foo
> two
> ) &&
> three
I thought of that too, but the inner (exit 117) doesn't even get
invoked unless there is &&-chain breakage somewhere above that point
(for instance, if "one" lacks "&&"), so the inner (exit 117) doesn't
participate in the linting process at all.
> that doesn't really help. The fundamental issue is that we may skip the
> "cd" inside the subshell. Whether it's in a subshell or not, that's
> dangerous. True, we don't run "three" in this case, which is slightly
> better. But it didn't expect to be in a different directory anyway. It's
> running "two" that is dangerous.
Just thinking aloud...
Aside from "rm -fr", there are numerous ways to clobber files
unexpectedly when the "cd" is skipped:
echo x >../git.c
cp x ../git.c
mv x ../git.c
ln [-s] x ../git.c
/bin/rm ../git.c
some-cmd -o ../git.c
Some of these dangers can be de-thoothed during the linting phase by
defining do-nothing shell functions:
cp () { :; }
mv () { :; }
ln () { :; }
That, at least, makes the scariest case ("rm") much less so.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 04:17:08PM -0400, Jeff King wrote: > I'm not sure if there's a good solution, though. Even if you retained > the subshells and instead did a chain-lint inside each subshell, like > this: So obviously that means "I don't think there's a good solution with this approach". That whole final patch simultaneously impresses and nauseates me. Your commit message says "no attempt is made at properly parsing shell code", but we come pretty darn close. I almost wonder if we'd be better off just parsing some heuristic subset and making sure (via review or linting) that our tests conform. Another option is to not enable this slightly-more-dangerous linting by default. But that would probably rob it of its usefulness, since it would just fall to some brave soul to later crank up the linting and fix everybody else's mistakes. -Peff
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 03:52:54PM -0400, Eric Sunshine wrote: > The existing --chain-lint already suffers the same shortcoming. Older > (or even new poorly-written) tests, even without subshells, can fall > victim already: > > (exit $sentinel) && > mkdir -p a/b/c && > cd a/b/c > rm -fr ../../* && > cd ../../.. && > statement4 > > As in your example, 'mkdir' and 'cd' are skipped, but 'rm -fr ../../*' is not. > > This snippet from the commit message of bb79af9d09 (t/test-lib: > introduce --chain-lint option, 2015-03-20): > > When we encounter a failure of this check, we abort the test > script entirely. For one thing, we have no clue which subset > of the commands in the test snippet were actually run. > > suggests that the issue was considered, in some form, even then > (though, it doesn't say explicitly that Peff had the 'rm -fr' case in > mind). > > So, this isn't a new problem introduced by this series, though this > series may exacerbate it. One way this series might be worse in practice is that we tend not to change process state too much outside of the subshells. So if we skip some early commands and execute a later "rm", for example, it tends to be in the same directory (and I think as time goes on we have been cleaning up old tests which did a "cd foo && bar && cd .." into using a subshell). Whereas once you start collapsing subshells into the main logic chain, there's a very high chance that the subshell is doing a "cd", since that's typically the main reason for the subshell in the first place. And with the current --chain-lint logic, that subshell is either executed or not executed as a unit. Obviously that's a bit of a hand-waving argument. If you've fixed all of the existing cases without accidentally deleting your home directory, then maybe it's not so likely to be a problem after all. I'm not sure if there's a good solution, though. Even if you retained the subshells and instead did a chain-lint inside each subshell, like this: (exit 117) && one && ( (exit 117) && cd foo two ) && three that doesn't really help. The fundamental issue is that we may skip the "cd" inside the subshell. Whether it's in a subshell or not, that's dangerous. True, we don't run "three" in this case, which is slightly better. But it didn't expect to be in a different directory anyway. It's running "two" that is dangerous. -Peff
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 3:15 PM Junio C Hamano wrote: > so, with --chain-lint, we would transform this > > mkdir -p a/b/c && > ( > cd a/b/c > rm -fr ../../* > ) && > statement 4 > > into this sequence > > (exit $sentinel) && > mkdir -p a/b/c && > cd a/b/c > rm -fr ../../* && > statement 4 > > and then rely on the non-zero exit to cancel all the remainder? > > We didn't create nor cd to the t/trash$num/a/b/c thanks to the && > chain, and end up running rm -fr ../../* from inside t/trash$num? Yes, I did take that into account and, no, I don't have a good answer to the issue. The existing --chain-lint already suffers the same shortcoming. Older (or even new poorly-written) tests, even without subshells, can fall victim already: (exit $sentinel) && mkdir -p a/b/c && cd a/b/c rm -fr ../../* && cd ../../.. && statement4 As in your example, 'mkdir' and 'cd' are skipped, but 'rm -fr ../../*' is not. This snippet from the commit message of bb79af9d09 (t/test-lib: introduce --chain-lint option, 2015-03-20): When we encounter a failure of this check, we abort the test script entirely. For one thing, we have no clue which subset of the commands in the test snippet were actually run. suggests that the issue was considered, in some form, even then (though, it doesn't say explicitly that Peff had the 'rm -fr' case in mind). So, this isn't a new problem introduced by this series, though this series may exacerbate it. Thanks for thinking critically about it.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Eric Sunshine writes: > The --chain-lint option detects broken &&-chains by forcing the test to > exit early (as the very first step) with a sentinel value. If that > sentinel is the test's overall exit code, then the &&-chain is intact; > if not, then the chain is broken. Unfortunately, this detection does not > extend to &&-chains within subshells even when the subshell itself is > properly linked into the outer &&-chain. > > Address this shortcoming by eliminating the subshell during the > "linting" phase and incorporating its body directly into the surrounding > &&-chain. To keep this transformation cheap, no attempt is made at > properly parsing shell code. Instead, the manipulations are purely > textual. For example: > > statement1 && > ( > statement2 && > statement3 > ) && > statement4 > > is transformed to: > > statement1 && > statement2 && > statement3 && > statement4 so, with --chain-lint, we would transform this mkdir -p a/b/c && ( cd a/b/c rm -fr ../../* ) && statement 4 into this sequence (exit $sentinel) && mkdir -p a/b/c && cd a/b/c rm -fr ../../* && statement 4 and then rely on the non-zero exit to cancel all the remainder? We didn't create nor cd to the t/trash$num/a/b/c thanks to the && chain, and end up running rm -fr ../../* from inside t/trash$num? Hm
