Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words

2012-11-18 Thread Felipe Contreras 
On Sun, Nov 18, 2012 at 12:40 AM, Jonathan Nieder  wrote:
> SZEDER Gábor wrote:
>
>>  The breakage can
>> be simply bogus possible completion words, but it can also be a
>> failure:
>>
>>   $ git branch '${foo.bar}'
>>   $ git checkout 
>>   bash: ${foo.bar}: bad substitution
>
> Or arbitrary code execution:
>
> $ git branch '$(>kilroy-was-here)'
> $ git checkout 
> $ ls -l kilroy-was-here
> -rw-rw-r-- 1 jrn jrn 0 nov 17 15:40 kilroy-was-here
>
> The final version of this patch should go to maint.  Thanks for
> catching it.

Shouldn't this go to the commit message?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words

2012-11-18 Thread Felipe Contreras 
On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor  wrote:

> @@ -155,6 +156,12 @@ test_expect_success '__gitcomp - suffix' '
> test_cmp expected out
>  '
>
> +test_expect_failure '__gitcomp - doesnt fail because of invalid variable 
> name' '
> +   (
> +   __gitcomp "$invalid_variable_name"
> +   )
> +'

Why in a subshell?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words

2012-11-17 Thread Jonathan Nieder 
SZEDER Gábor wrote:

>  The breakage can
> be simply bogus possible completion words, but it can also be a
> failure:
>
>   $ git branch '${foo.bar}'
>   $ git checkout 
>   bash: ${foo.bar}: bad substitution

Or arbitrary code execution:

$ git branch '$(>kilroy-was-here)'
$ git checkout 
$ ls -l kilroy-was-here
-rw-rw-r-- 1 jrn jrn 0 nov 17 15:40 kilroy-was-here

The final version of this patch should go to maint.  Thanks for
catching it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/7] completion: add tests for invalid variable name among completion words

2012-11-17 Thread SZEDER Gábor 
The compgen Bash-builtin performs expansion on all words in the
wordlist given to its -W option, breaking Git's completion script in
various ways if one of those words can be expanded.  The breakage can
be simply bogus possible completion words, but it can also be a
failure:

  $ git branch '${foo.bar}'
  $ git checkout 
  bash: ${foo.bar}: bad substitution

${foo.bar} is an invalid variable name, which triggers the failure
when compgen attempts to expand it, completely breaking refs
completion.  The same applies to e.g. completing the :
notation when a filename contains a similar expandable substring.

Since both __gitcomp() and __gitcomp_nl() rely on compgen, both are
affected by this issue.  So add a simple test for each of them to
check that such a word doesn't cause failures (but don't check the
resulting possible completion words for now, because that should be
quoted properly, and that's a separate topic).

Reported-by: Jeroen Meijer 
Signed-off-by: SZEDER Gábor 
---
 t/t9902-completion.sh | 13 +
 1 file changed, 13 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 32b3e8c4..a108ec1c 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -71,6 +71,7 @@ test_completion_long ()
 }
 
 newline=$'\n'
+invalid_variable_name="${foo.bar}"
 
 test_expect_success '__gitcomp - trailing space - options' '
sed -e "s/Z$//" >expected <<-\EOF &&
@@ -155,6 +156,12 @@ test_expect_success '__gitcomp - suffix' '
test_cmp expected out
 '
 
+test_expect_failure '__gitcomp - doesnt fail because of invalid variable name' 
'
+   (
+   __gitcomp "$invalid_variable_name"
+   )
+'
+
 test_expect_success '__gitcomp_nl - trailing space' '
sed -e "s/Z$//" >expected <<-\EOF &&
maint Z
@@ -239,6 +246,12 @@ test_expect_success '__gitcomp_nl - no suffix' '
test_cmp expected out
 '
 
+test_expect_failure '__gitcomp_nl - doesnt fail because of invalid variable 
name' '
+   (
+   __gitcomp_nl "$invalid_variable_name"
+   )
+'
+
 test_expect_success 'basic' '
run_completion git "" &&
# built-in
-- 
1.8.0.220.g4d14ece

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html