threads / patch / 49901

patchtests: avoid syntax triggering old dash bug

Subject: [PATCH] tests: avoid syntax triggering old dash bug

## tl;dr

10 messages between Nov 27, 2018 and Feb 14, 2019. Diffs are folded; open one to read it.

replies: 9people: 6as markdown or json

Ævar Arnfjörð Bjarmason· Nov 27, 2018, 16:42 UTC · lore

Avoid a bug in dash that's been fixed ever since its ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first released with dash v0.5.7 in July 2011.

This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding" before URL encoding", 2016-02-09).

This particular test has been failing since 5f9674243d ("config: add --expiry-date", 2017-11-18).

1. https://git.kernel.org/pub/scm/utils/dash/dash.git/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1300-config.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to t/t1300-config.sh +2 −1
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 9652b241c7..7690b518b8 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '
 	1510348087
 	0
 	EOF
+	date_valid1=$(git config --expiry-date date.valid1) &&
 	{
-		echo "$rel_out $(git config --expiry-date date.valid1)"
+		echo "$rel_out $date_valid1"
 		git config --expiry-date date.valid2 &&
 		git config --expiry-date date.valid3 &&
 		git config --expiry-date date.valid4 &&
-- 
2.20.0.rc1.379.g1dd7ef354c
Eric Sunshine· Nov 27, 2018, 19:16 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] tests: avoid syntax triggering old dash bug

On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011.

Perhaps enhance the commit message to explain the nature of the bug itself. It is not at all obvious from reading the above or from looking at the diff itself what the actual problem is that the patch is fixing. (And it wasn't even immediately obvious by looking at the commit message of ec2c84d in the dash repository.) To help readers of this patch avoid re-introducing this problem or diagnose such a failure, it might be a good idea to give an example of the syntax which trips up old dash (i.e. a here-doc followed immediately by a {...} expression) and the actual error message 'Syntax error: "}" unexpected'.

Thanks.
Show 8 quoted lines
> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).
>
> This particular test has been failing since 5f9674243d ("config: add
> --expiry-date", 2017-11-18).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Ævar Arnfjörð Bjarmason· Nov 27, 2018, 19:37 UTC · re: Eric Sunshine · lore

Re: [PATCH] tests: avoid syntax triggering old dash bug

On Tue, Nov 27 2018, Eric Sunshine wrote:
Show 16 quoted lines
> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011.
>
> Perhaps enhance the commit message to explain the nature of the bug
> itself. It is not at all obvious from reading the above or from
> looking at the diff itself what the actual problem is that the patch
> is fixing. (And it wasn't even immediately obvious by looking at the
> commit message of ec2c84d in the dash repository.) To help readers of
> this patch avoid re-introducing this problem or diagnose such a
> failure, it might be a good idea to give an example of the syntax
> which trips up old dash (i.e. a here-doc followed immediately by a
> {...} expression) and the actual error message 'Syntax error: "}"
> unexpected'.

I haven't taken the time to understand the bug either. Our entire test suite had one instance of this, so I think it's obscure enough that it's fine to just fix it as a one-off and not spend any more time on making sure it doesn't happen again or add some lint for detecting it.

Show 8 quoted lines
>> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
>> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
>> before URL encoding", 2016-02-09).
>>
>> This particular test has been failing since 5f9674243d ("config: add
>> --expiry-date", 2017-11-18).
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Junio C Hamano· Nov 28, 2018, 04:45 UTC · re: Eric Sunshine · lore

Re: [PATCH] tests: avoid syntax triggering old dash bug

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 16 quoted lines
> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011.
>
> Perhaps enhance the commit message to explain the nature of the bug
> itself. It is not at all obvious from reading the above or from
> looking at the diff itself what the actual problem is that the patch
> is fixing. (And it wasn't even immediately obvious by looking at the
> commit message of ec2c84d in the dash repository.) To help readers of
> this patch avoid re-introducing this problem or diagnose such a
> failure, it might be a good idea to give an example of the syntax
> which trips up old dash (i.e. a here-doc followed immediately by a
> {...} expression) and the actual error message 'Syntax error: "}"
> unexpected'.

Indeed. From the patch text, I would not have even guessed. I was wondering if there were interactions with "" and $() inside it.

If having {...} immediately after a here-doc is a problem, then the patch should not touch existing code at all, but instead insert a new line, perhaps like

	: avoid open brace immediately after here-doc for old dash
immediately before {...}; that would have made it easier to grok.
Show 11 quoted lines
>@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '
> 	1510348087
> 	0
> 	EOF
>+	date_valid1=$(git config --expiry-date date.valid1) &&
> 	{
>-		echo "$rel_out $(git config --expiry-date date.valid1)"
>+		echo "$rel_out $date_valid1"
> 		git config --expiry-date date.valid2 &&
> 		git config --expiry-date date.valid3 &&
> 		git config --expiry-date date.valid4 &&
Ævar Arnfjörð Bjarmason· Feb 13, 2019, 11:59 UTC · re: Junio C Hamano · lore

[PATCH v2] tests: avoid syntax triggering old dash bug

Avoid a bug in dash that's been fixed ever since its ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first released with dash v0.5.7 in July 2011. This failing test was introduced in 5f9674243d ("config: add --expiry-date", 2017-11-18).

This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding" before URL encoding", 2016-02-09).

The dash bug is triggered by this test because the heredoc contains a command embedded in "$()" with a "{}" block coming right after it. Refactoring the "$()" to e.g. be a variable that was set earlier will also work around it, but let's instead break up the "EOF" and the "{}".

An earlier version of this patch[2] mitigated the issue by breaking the "$()" out of the "{}" block, that worked, but just because it broke up the "EOF" and "{}" block. Putting e.g. "echo &&" between the two would also work.

1. https://git.kernel.org/pub/scm/utils/dash/dash.git/
2. https://public-inbox.org/git/20181127164253.9832-1-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
On Wed, Nov 28 2018, Junio C Hamano wrote:
Show 27 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>> Avoid a bug in dash that's been fixed ever since its
>>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>>> released with dash v0.5.7 in July 2011.
>>
>> Perhaps enhance the commit message to explain the nature of the bug
>> itself. It is not at all obvious from reading the above or from
>> looking at the diff itself what the actual problem is that the patch
>> is fixing. (And it wasn't even immediately obvious by looking at the
>> commit message of ec2c84d in the dash repository.) To help readers of
>> this patch avoid re-introducing this problem or diagnose such a
>> failure, it might be a good idea to give an example of the syntax
>> which trips up old dash (i.e. a here-doc followed immediately by a
>> {...} expression) and the actual error message 'Syntax error: "}"
>> unexpected'.
>
> Indeed.  From the patch text, I would not have even guessed.  I was
> wondering if there were interactions with "" and $() inside it.
>
> If having {...} immediately after a here-doc is a problem, then the
> patch should not touch existing code at all, but instead insert a
> new line, perhaps like
>
> 	: avoid open brace immediately after here-doc for old dash

Late re-roll. Now using a variant of that suggestion, and with an updated commit message explaining what the issue in dash is exactly and why it was triggered.

This isn't a 2.21 regression, but sending it in the rc window anyway in case you'd like to queue an obviously working minor portability fix.

 t/t1300-config.sh | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/t1300-config.sh +1 −0
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 9652b241c7..428177c390 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -892,6 +892,7 @@ test_expect_success 'get --expiry-date' '
 	1510348087
 	0
 	EOF
+	: "work around heredoc parsing bug fixed in dash 0.5.7 (in ec2c84d)" &&
 	{
 		echo "$rel_out $(git config --expiry-date date.valid1)"
 		git config --expiry-date date.valid2 &&
-- 
2.20.1.611.gfbb209baf1
Junio C Hamano· Feb 13, 2019, 21:43 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2] tests: avoid syntax triggering old dash bug

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011. This failing test was
> introduced in 5f9674243d ("config: add --expiry-date", 2017-11-18).

Thanks for not forgetting an year-old topic. The offending commit predates 2.16.0 that is a while ago ;-)

Show 19 quoted lines
> This isn't a 2.21 regression, but sending it in the rc window anyway
> in case you'd like to queue an obviously working minor portability
> fix.
>
>  t/t1300-config.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 9652b241c7..428177c390 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -892,6 +892,7 @@ test_expect_success 'get --expiry-date' '
>  	1510348087
>  	0
>  	EOF
> +	: "work around heredoc parsing bug fixed in dash 0.5.7 (in ec2c84d)" &&
>  	{
>  		echo "$rel_out $(git config --expiry-date date.valid1)"
>  		git config --expiry-date date.valid2 &&
SZEDER Gábor· Feb 13, 2019, 21:48 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2] tests: avoid syntax triggering old dash bug

On Wed, Feb 13, 2019 at 12:59:51PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 6 quoted lines
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011. This failing test was
> introduced in 5f9674243d ("config: add --expiry-date", 2017-11-18).
> 
> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other

Do I understand this "1/2" right? There are two tests failing on Lenny and Squeeze, and this fixes one of those bugs?

Show 15 quoted lines
> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).
> 
> The dash bug is triggered by this test because the heredoc contains a
> command embedded in "$()" with a "{}" block coming right after
> it. Refactoring the "$()" to e.g. be a variable that was set earlier
> will also work around it, but let's instead break up the "EOF" and the
> "{}".
> 
> An earlier version of this patch[2] mitigated the issue by breaking
> the "$()" out of the "{}" block, that worked, but just because it
> broke up the "EOF" and "{}" block. Putting e.g. "echo &&" between the
> two would also work.
> 
> 1. https://git.kernel.org/pub/scm/utils/dash/dash.git/
Could you please link directly to the commit fixing that issue?
  https://git.kernel.org/pub/scm/utils/dash/dash.git/commit/?id=ec2c84d3c4dba4b74440d72bdd1de416a9acd2a9
> 2. https://public-inbox.org/git/20181127164253.9832-1-avarab@gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Ævar Arnfjörð Bjarmason· Feb 14, 2019, 09:51 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2] tests: avoid syntax triggering old dash bug

On Wed, Feb 13 2019, SZEDER Gábor wrote:
Show 10 quoted lines
> On Wed, Feb 13, 2019 at 12:59:51PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011. This failing test was
>> introduced in 5f9674243d ("config: add --expiry-date", 2017-11-18).
>>
>> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
>
> Do I understand this "1/2" right?  There are two tests failing on
> Lenny and Squeeze, and this fixes one of those bugs?
Yeah, so there's one bug left now, which I haven't tracked down.
Show 19 quoted lines
>> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
>> before URL encoding", 2016-02-09).
>>
>> The dash bug is triggered by this test because the heredoc contains a
>> command embedded in "$()" with a "{}" block coming right after
>> it. Refactoring the "$()" to e.g. be a variable that was set earlier
>> will also work around it, but let's instead break up the "EOF" and the
>> "{}".
>>
>> An earlier version of this patch[2] mitigated the issue by breaking
>> the "$()" out of the "{}" block, that worked, but just because it
>> broke up the "EOF" and "{}" block. Putting e.g. "echo &&" between the
>> two would also work.
>>
>> 1. https://git.kernel.org/pub/scm/utils/dash/dash.git/
>
> Could you please link directly to the commit fixing that issue?
>
>   https://git.kernel.org/pub/scm/utils/dash/dash.git/commit/?id=ec2c84d3c4dba4b74440d72bdd1de416a9acd2a9

Should have done that, but I'll hold off on a re-roll for such a minor cosmetic issue since I see Junio's merged it down to "next" already. The dash.git hash is noted in the commit message, so it's not a practical problem to find the commit, but yeah, would be nice if were a clickable link.

>> 2. https://public-inbox.org/git/20181127164253.9832-1-avarab@gmail.com/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
brian m. carlson· Nov 28, 2018, 01:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] tests: avoid syntax triggering old dash bug

On Tue, Nov 27, 2018 at 05:42:53PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 7 quoted lines
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011.
> 
> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).

Are people still using such versions of Debian? I only see wheezy (7) on the mirrors, not squeeze (6) or lenny (5). It might be better for us to encourage users to upgrade to an OS that has security support rather than work around bugs in obsolete OSes.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Torsten Bögershausen· Nov 28, 2018, 07:06 UTC · re: brian m. carlson · lore

Re: [PATCH] tests: avoid syntax triggering old dash bug

On Wed, Nov 28, 2018 at 01:47:41AM +0000, brian m. carlson wrote:
Show 13 quoted lines
> On Tue, Nov 27, 2018 at 05:42:53PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > Avoid a bug in dash that's been fixed ever since its
> > ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> > released with dash v0.5.7 in July 2011.
> > 
> > This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> > failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> > before URL encoding", 2016-02-09).
> 
> Are people still using such versions of Debian?  I only see wheezy (7)
> on the mirrors, not squeeze (6) or lenny (5).  It might be better for us
> to encourage users to upgrade to an OS that has security support rather
> than work around bugs in obsolete OSes.

Yes, I have an old PowerPC box to test if code handle endians right. And to ask people to upgrade does not conflict with supporting older versions (if that is as easy as this patch). I think we can have both.

← back to recent threads