[PATCH] gitweb: fix error when highlight is enabled

[PATCH] gitweb: fix error when highlight is enabled

[Orgad Shaneh]

Use of uninitialized value in substitution iterator at gitweb.cgi line 1560

Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
 gitweb/gitweb.perl |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0f207f2..862b9cd 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1556,7 +1556,7 @@ sub sanitize {
 	return undef unless defined $str;
 
 	$str = to_utf8($str);
-	$str =~ s|([[:cntrl:]])|($1 =~ /[\t\n\r]/ ? $1 : quot_cec($1))|eg;
+	$str =~ s|([[:cntrl:]])|($1 =~ /([\t\n\r])/ ? $1 : quot_cec($1))|eg;
 	return $str;
 }
 
-- 
1.7.10.4

Re: [PATCH] gitweb: fix error when highlight is enabled

[Junio C Hamano]

Orgad Shaneh <orgads@gmail.com> writes:

> Use of uninitialized value in substitution iterator at gitweb.cgi line 1560

This is not just about squelching an error message, but more
importantly, attempting to fix an information lossage, no?

The statement captures each control character in the string to $1,
then matches a class of known/safe control chars against that
control character we just have seen. If matches, it just wants to
use that control character, otherwise it wants to apply quot_cec()
on that control character.  It forgets that "$1" is reset
immediately when =~ matches with the class of known/safe control
chars, and your version attempts to fix it by recapturing it.

What if you are looking at a non-safe control, say "\001"?  It is
matched and is captured by ([[;cntrl:]]), making $1 -eq "\001", and
then the replacement side of s///e operator, tries to match and
capture it with ([\t\n\r]), but it does *not* match.

What does that "$1" you are feeding quot_cec() contain at that
point?  I _think_ "$1" is left intact when the inner match fails and
you are correctly feeding "\001" to quot_cec(), but it is not
immediately obvious.  Perl regexp, especially s///e, is a yucky
language X-<.

I wonder if there is a better way to express what goes inside the
replacement side of this s///e construct in a more obvious way. The
updated one may be correct but it looks too subtle to my taste..

> Signed-off-by: Orgad Shaneh <orgads@gmail.com>
> ---
>  gitweb/gitweb.perl |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 0f207f2..862b9cd 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1556,7 +1556,7 @@ sub sanitize {
>  	return undef unless defined $str;
>  
>  	$str = to_utf8($str);
> -	$str =~ s|([[:cntrl:]])|($1 =~ /[\t\n\r]/ ? $1 : quot_cec($1))|eg;
> +	$str =~ s|([[:cntrl:]])|($1 =~ /([\t\n\r])/ ? $1 : quot_cec($1))|eg;
>  	return $str;
>  }