threads / patch / 29781

patchgitweb: Handle invalid regexp in regexp search

Subject: [PATCH (BUGFIX)] gitweb: Handle invalid regexp in regexp search

## tl;dr

18 messages between Feb 28, 2012 and Mar 6, 2012. Diffs are folded; open one to read it.

replies: 17people: 3as markdown or json

Jakub Narebski· Feb 28, 2012, 18:41 UTC · lore

When using regexp search ('sr' parameter / $search_use_regexp variable is true), check first that regexp is valid.

Without this patch we would get an error from Perl during search (if searching is performed by gitweb), or highlighting matches substring (if applicable), if user provided invalid regexp... which means broken HTML, with error page (including HTTP headers) generated after gitweb already produced some output.

Add test that illustrates such error: for example for regexp "*\.git" we would get the following error:

  Quantifier follows nothing in regex; marked by <-- HERE in m/* <-- HERE \.git/
  at /var/www/cgi-bin/gitweb.cgi line 3084.
Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
See "Re: gitweb: (potential) problems with new installation"
http://thread.gmane.org/gmane.comp.version-control.git/191746
 gitweb/gitweb.perl                       |   11 ++++++++++-
 t/t9501-gitweb-standalone-http-status.sh |   10 ++++++++++
 2 files changed, 20 insertions(+), 1 deletions(-)
Show changes to 2 files +20 −1

gitweb/gitweb.perl, t/t9501-gitweb-standalone-http-status.sh

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1fc5361..22ad279 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1081,7 +1081,16 @@ sub evaluate_and_validate_params {
 		if (length($searchtext) < 2) {
 			die_error(403, "At least two characters are required for search parameter");
 		}
-		$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
+		if ($search_use_regexp) {
+			$search_regexp = $searchtext;
+			if (!eval { qr/$search_regexp/; 1; }) {
+				(my $error = $@) =~ s/ at \S+ line \d+.*\n?//;
+				die_error(400, "Invalid search regexp '$search_regexp'",
+				          esc_html($error));
+			}
+		} else {
+			$search_regexp = quotemeta $searchtext;
+		}
 	}
 }
 
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 26102ee..31076ed 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -134,4 +134,14 @@ our $maxload = undef;
 EOF
 
 
+# ----------------------------------------------------------------------
+# invalid arguments
+
+test_expect_success 'invalid arguments: invalid regexp (in project search)' '
+	gitweb_run "a=project_list;s=*\.git;sr=1" &&
+	grep "Status: 400" gitweb.headers &&
+	grep "400 - Invalid.*regexp" gitweb.body
+'
+test_debug 'cat gitweb.headers'
+
 test_done
Junio C Hamano· Feb 28, 2012, 19:45 UTC · re: Jakub Narebski · lore

Re: [PATCH (BUGFIX)] gitweb: Handle invalid regexp in regexp search

Jakub Narebski <jnareb@gmail.com> writes:
> When using regexp search ('sr' parameter / $search_use_regexp variable
> is true), check first that regexp is valid.
Thanks.
How old is this bug?  Should it go to older maitenance tracks like 1.7.6?
Jakub Narebski· Feb 29, 2012, 15:56 UTC · re: Junio C Hamano · lore

Re: [PATCH (BUGFIX)] gitweb: Handle invalid regexp in regexp search

Junio C Hamano wrote:
Show 8 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > When using regexp search ('sr' parameter / $search_use_regexp variable
> > is true), check first that regexp is valid.
> 
> Thanks.
> 
> How old is this bug?  Should it go to older maitenance tracks like 1.7.6?
>From what I examined this bug is from the very beginning when gitweb
started to distinguish regexp search and fixed string search in
  0e55991 (gitweb: Clearly distinguish regexp / exact match searches, 2008-02-26)

It was present in 1.5.5 (including beginnings of match highlighting, which trigger this bug).

This bug was present so long without detection because circumstances must be quite specific: you have to select regexp search, and to provide invalid regexp. If you know what regexp is, you probably write correct ones... but there always room for mistake.

-- 
Jakub Narebski
Poland
Ramsay Jones· Mar 2, 2012, 19:44 UTC · re: Jakub Narebski · lore

Re: [PATCH (BUGFIX)] gitweb: Handle invalid regexp in regexp search

Jakub Narebski wrote:
Show 20 quoted lines
> When using regexp search ('sr' parameter / $search_use_regexp variable
> is true), check first that regexp is valid.
> 
> Without this patch we would get an error from Perl during search (if
> searching is performed by gitweb), or highlighting matches substring
> (if applicable), if user provided invalid regexp... which means broken
> HTML, with error page (including HTTP headers) generated after gitweb
> already produced some output.
> 
> Add test that illustrates such error: for example for regexp "*\.git"
> we would get the following error:
> 
>   Quantifier follows nothing in regex; marked by <-- HERE in m/* <-- HERE \.git/
>   at /var/www/cgi-bin/gitweb.cgi line 3084.
> 
> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> See "Re: gitweb: (potential) problems with new installation"
> http://thread.gmane.org/gmane.comp.version-control.git/191746

This patch solves the problem for me when using a regex search (re checkbox checked), but *not* for a non-regex search.

If you have a leading '*' or '+', in the non-regex case, then you still get the above complaint (and xml error page etc.), although the line number has changed slightly from that given above.

ATB, Ramsay Jones

Jakub Narebski· Mar 2, 2012, 22:34 UTC · re: Ramsay Jones · lore

[PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

Use $search_regexp, where regex metacharacters are quoted, for searching projects list, rather than $searchtext, which contains original search term.

Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I think this bug was here from the very beginning of adding project
search, i.e. from  v1.6.0.2-446-g0d1d154 (gitweb: Support for simple
project search form, 2008-10-03)  which was present since 1.6.1
On Fri, 2 Mar 2012, Ramsay Jones wrote:
> Jakub Narebski wrote:
Show 27 quoted lines
> > When using regexp search ('sr' parameter / $search_use_regexp variable
> > is true), check first that regexp is valid.
> > 
> > Without this patch we would get an error from Perl during search (if
> > searching is performed by gitweb), or highlighting matches substring
> > (if applicable), if user provided invalid regexp... which means broken
> > HTML, with error page (including HTTP headers) generated after gitweb
> > already produced some output.
> > 
> > Add test that illustrates such error: for example for regexp "*\.git"
> > we would get the following error:
> > 
> >   Quantifier follows nothing in regex; marked by <-- HERE in m/* <-- HERE \.git/
> >   at /var/www/cgi-bin/gitweb.cgi line 3084.
> > 
> > Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> > See "Re: gitweb: (potential) problems with new installation"
> > http://thread.gmane.org/gmane.comp.version-control.git/191746
> 
> This patch solves the problem for me when using a regex search
> (re checkbox checked), but *not* for a non-regex search.
> 
> If you have a leading '*' or '+', in the non-regex case, then you
> still get the above complaint (and xml error page etc.), although
> the line number has changed slightly from that given above.

Ramsay, please provide those line number in the future, together with line and if possible some context.

The line is different because it is different bug: this is about not using quotemeta'ed string for search for fixed-string search.

 gitweb/gitweb.perl |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)
Show changes to gitweb/gitweb.perl +11 −11
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 22ad279..7398be1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3072,16 +3072,16 @@ sub filter_forks_from_projects_list {
 # for 'descr_long' and 'ctags' to be filled
 sub search_projects_list {
 	my ($projlist, %opts) = @_;
-	my $tagfilter  = $opts{'tagfilter'};
-	my $searchtext = $opts{'searchtext'};
+	my $tagfilter = $opts{'tagfilter'};
+	my $search_re = $opts{'search_regexp'};
 
 	return @$projlist
-		unless ($tagfilter || $searchtext);
+		unless ($tagfilter || $search_re);
 
 	# searching projects require filling to be run before it;
 	fill_project_list_info($projlist,
-	                       $tagfilter  ? 'ctags' : (),
-	                       $searchtext ? ('path', 'descr') : ());
+	                       $tagfilter ? 'ctags' : (),
+	                       $search_re ? ('path', 'descr') : ());
 	my @projects;
  PROJECT:
 	foreach my $pr (@$projlist) {
@@ -3092,10 +3092,10 @@ sub search_projects_list {
 				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
 		}
 
-		if ($searchtext) {
+		if ($search_re) {
 			next unless
-				$pr->{'path'} =~ /$searchtext/ ||
-				$pr->{'descr_long'} =~ /$searchtext/;
+				$pr->{'path'} =~ /$search_re/ ||
+				$pr->{'descr_long'} =~ /$search_re/;
 		}
 
 		push @projects, $pr;
@@ -5498,9 +5498,9 @@ sub git_project_list_body {
 		if ($check_forks);
 	# search_projects_list pre-fills required info
 	@projects = search_projects_list(\@projects,
-	                                 'searchtext' => $searchtext,
-	                                 'tagfilter'  => $tagfilter)
-		if ($tagfilter || $searchtext);
+	                                 'search_regexp' => $search_regexp,
+	                                 'tagfilter' => $tagfilter)
+		if ($tagfilter || $search_regexp);
 	# fill the rest
 	@projects = fill_project_list_info(\@projects);
 

-- 
1.7.9
Junio C Hamano· Mar 3, 2012, 00:08 UTC · re: Jakub Narebski · lore

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

Jakub Narebski <jnareb@gmail.com> writes:
Show 16 quoted lines
> Use $search_regexp, where regex metacharacters are quoted, for
> searching projects list, rather than $searchtext, which contains
> original search term.
>
> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> I think this bug was here from the very beginning of adding project
> search, i.e. from  v1.6.0.2-446-g0d1d154 (gitweb: Support for simple
> project search form, 2008-10-03)  which was present since 1.6.1
>
> On Fri, 2 Mar 2012, Ramsay Jones wrote:
> 
>> This patch solves the problem for me when using a regex search
>> (re checkbox checked), but *not* for a non-regex search.
>>

This patch depends on the more recent changes than the regexp fix, no? I was hoping that we could merge the earlier fix for the regexp case to older maintenance tracks later, but if we were going to do so, we would want to do the same for a fix for fixed-string case.

I am fine with not to worrying too much about older maintenance tracks, and applying this directly to 'master', but just wanted to see what your preference is.

Thanks.
Show 65 quoted lines
>> If you have a leading '*' or '+', in the non-regex case, then you
>> still get the above complaint (and xml error page etc.), although
>> the line number has changed slightly from that given above.
>
> Ramsay, please provide those line number in the future, together with
> line and if possible some context.
>
> The line is different because it is different bug: this is about not
> using quotemeta'ed string for search for fixed-string search.
>
>  gitweb/gitweb.perl |   22 +++++++++++-----------
>  1 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 22ad279..7398be1 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3072,16 +3072,16 @@ sub filter_forks_from_projects_list {
>  # for 'descr_long' and 'ctags' to be filled
>  sub search_projects_list {
>  	my ($projlist, %opts) = @_;
> -	my $tagfilter  = $opts{'tagfilter'};
> -	my $searchtext = $opts{'searchtext'};
> +	my $tagfilter = $opts{'tagfilter'};
> +	my $search_re = $opts{'search_regexp'};
>  
>  	return @$projlist
> -		unless ($tagfilter || $searchtext);
> +		unless ($tagfilter || $search_re);
>  
>  	# searching projects require filling to be run before it;
>  	fill_project_list_info($projlist,
> -	                       $tagfilter  ? 'ctags' : (),
> -	                       $searchtext ? ('path', 'descr') : ());
> +	                       $tagfilter ? 'ctags' : (),
> +	                       $search_re ? ('path', 'descr') : ());
>  	my @projects;
>   PROJECT:
>  	foreach my $pr (@$projlist) {
> @@ -3092,10 +3092,10 @@ sub search_projects_list {
>  				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
>  		}
>  
> -		if ($searchtext) {
> +		if ($search_re) {
>  			next unless
> -				$pr->{'path'} =~ /$searchtext/ ||
> -				$pr->{'descr_long'} =~ /$searchtext/;
> +				$pr->{'path'} =~ /$search_re/ ||
> +				$pr->{'descr_long'} =~ /$search_re/;
>  		}
>  
>  		push @projects, $pr;
> @@ -5498,9 +5498,9 @@ sub git_project_list_body {
>  		if ($check_forks);
>  	# search_projects_list pre-fills required info
>  	@projects = search_projects_list(\@projects,
> -	                                 'searchtext' => $searchtext,
> -	                                 'tagfilter'  => $tagfilter)
> -		if ($tagfilter || $searchtext);
> +	                                 'search_regexp' => $search_regexp,
> +	                                 'tagfilter' => $tagfilter)
> +		if ($tagfilter || $search_regexp);
>  	# fill the rest
>  	@projects = fill_project_list_info(\@projects);
Jakub Narebski· Mar 3, 2012, 10:55 UTC · re: Junio C Hamano · lore

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

On Sat, 3 Mar 2012, Junio C Hamano wrote:
Show 23 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Use $search_regexp, where regex metacharacters are quoted, for
>> searching projects list, rather than $searchtext, which contains
>> original search term.
>>
>> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>> ---
>> I think this bug was here from the very beginning of adding project
>> search, i.e. from  v1.6.0.2-446-g0d1d154 (gitweb: Support for simple
>> project search form, 2008-10-03)  which was present since 1.6.1
>>
>> On Fri, 2 Mar 2012, Ramsay Jones wrote:
>> 
>>> This patch solves the problem for me when using a regex search
>>> (re checkbox checked), but *not* for a non-regex search.
>>> 
> 
> This patch depends on the more recent changes than the regexp fix, no?  I
> was hoping that we could merge the earlier fix for the regexp case to
> older maintenance tracks later, but if we were going to do so, we would
> want to do the same for a fix for fixed-string case.
The regexp and non-regexp bugs and fixes are different.

The regexp "bug" was just us forgetting that regexp is provided by user input, and should be validated. The bug as reported by Ramsay was here from the very beginning, i.e. commit 0e55991 (gitweb: Clearly distinguish regexp / exact match searches, 2008-02-26), which was present in v1.5.1 if I have checked correctly. The fix is about adding new code and should apply cleanly to 'maint' and even to older versions; the only trouble with older version might be whitespace issue related to refactoring code into subroutines.

The non-regexp project search bug was using $searchtext instead of $search_regexp as search regexp in gitweb. The bug was present from the very addition of project search, namely commit 0d1d154 (gitweb: Support for simple project search form, 2008-10-03), which was present in v1.5.1 if I have checked correctly. Unfortunately the fix affects code that was changed recently in a1e1b2d (gitweb: improve usability of projects search form, 2012-01-31); I'll try to come up with equivalent patch to 'maint' soon (if the current one does not apply, and I guess it doesn't).

-- 
Jakub Narebski
Poland
Jakub Narebski· Mar 4, 2012, 09:35 UTC · re: Jakub Narebski · lore

[PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

On Sat, 3 Mar 2012, Jakub Narebski wrote:
Show 26 quoted lines
> On Sat, 3 Mar 2012, Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>>> Use $search_regexp, where regex metacharacters are quoted, for
>>> searching projects list, rather than $searchtext, which contains
>>> original search term.
>>>
>>> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>>> ---
>>> I think this bug was here from the very beginning of adding project
>>> search, i.e. from  v1.6.0.2-446-g0d1d154 (gitweb: Support for simple
>>> project search form, 2008-10-03)  which was present since 1.6.1
>>>
>>> On Fri, 2 Mar 2012, Ramsay Jones wrote:
>>> 
>>>> This patch solves the problem for me when using a regex search
>>>> (re checkbox checked), but *not* for a non-regex search.
>>>> 
>> 
>> This patch depends on the more recent changes than the regexp fix, no?  I
>> was hoping that we could merge the earlier fix for the regexp case to
>> older maintenance tracks later, but if we were going to do so, we would
>> want to do the same for a fix for fixed-string case.
> 
> The regexp and non-regexp bugs and fixes are different.
[...]
Show 9 quoted lines
> The non-regexp project search bug was using $searchtext instead of
> $search_regexp as search regexp in gitweb.  The bug was present from
> the very addition of project search, namely commit 0d1d154 (gitweb:
> Support for simple project search form, 2008-10-03), which was present
> in v1.5.1 if I have checked correctly.  Unfortunately the fix affects
> code that was changed recently in a1e1b2d (gitweb: improve usability
> of projects search form, 2012-01-31); I'll try to come up with equivalent
> patch to 'maint' soon (if the current one does not apply, and I guess it
> doesn't).
And here is the patch for maint
-->8-- -------------------------------------------------------- -->8--
Subject: gitweb: Fix fixed string (non-regexp) project search

Use $search_regexp, where regex metacharacters are quoted, for searching projects list, rather than $searchtext, which contains original search term.

Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)
Show changes to gitweb/gitweb.perl +11 −9
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d5dbd64..e248792 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2968,11 +2968,11 @@ sub filter_forks_from_projects_list {
 # for 'descr_long' and 'ctags' to be filled
 sub search_projects_list {
 	my ($projlist, %opts) = @_;
-	my $tagfilter  = $opts{'tagfilter'};
-	my $searchtext = $opts{'searchtext'};
+	my $tagfilter = $opts{'tagfilter'};
+	my $search_re = $opts{'search_regexp'};
 
 	return @$projlist
-		unless ($tagfilter || $searchtext);
+		unless ($tagfilter || $search_re);
 
 	my @projects;
  PROJECT:
@@ -2984,10 +2984,10 @@ sub search_projects_list {
 				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
 		}
 
-		if ($searchtext) {
+		if ($search_re) {
 			next unless
-				$pr->{'path'} =~ /$searchtext/ ||
-				$pr->{'descr_long'} =~ /$searchtext/;
+				$pr->{'path'} =~ /$search_re/ ||
+				$pr->{'descr_long'} =~ /$search_re/;
 		}
 
 		push @projects, $pr;
@@ -5290,9 +5290,11 @@ sub git_project_list_body {
 	@projects = fill_project_list_info(\@projects);
 	# searching projects require filling to be run before it
 	@projects = search_projects_list(\@projects,
-	                                 'searchtext' => $searchtext,
-	                                 'tagfilter'  => $tagfilter)
-		if ($tagfilter || $searchtext);
+	                                 'search_regexp' => $search_regexp,
+	                                 'tagfilter' => $tagfilter)
+		if ($tagfilter || $search_regexp);
+	# fill the rest
+	@projects = fill_project_list_info(\@projects);
 
 	$order ||= $default_projects_order;
 	$from = 0 unless defined $from;

-- 
1.7.9
Jakub Narebski· Mar 4, 2012, 18:00 UTC · re: Jakub Narebski · lore

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

Jakub Narebski wrote:
Show 45 quoted lines
> On Sat, 3 Mar 2012, Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>>> Use $search_regexp, where regex metacharacters are quoted, for
>>> searching projects list, rather than $searchtext, which contains
>>> original search term.
>>>
>>> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>>> ---
>>> I think this bug was here from the very beginning of adding project
>>> search, i.e. from  v1.6.0.2-446-g0d1d154 (gitweb: Support for simple
>>> project search form, 2008-10-03)  which was present since 1.6.1
>>>
>>> On Fri, 2 Mar 2012, Ramsay Jones wrote:
>>> 
>>>> This patch solves the problem for me when using a regex search
>>>> (re checkbox checked), but *not* for a non-regex search.
>>>> 
>> 
>> This patch depends on the more recent changes than the regexp fix, no?  I
>> was hoping that we could merge the earlier fix for the regexp case to
>> older maintenance tracks later, but if we were going to do so, we would
>> want to do the same for a fix for fixed-string case.
> 
> The regexp and non-regexp bugs and fixes are different.
> 
> The regexp "bug" was just us forgetting that regexp is provided by user
> input, and should be validated.  The bug as reported by Ramsay was here
> from the very beginning, i.e. commit 0e55991 (gitweb: Clearly distinguish
> regexp / exact match searches, 2008-02-26), which was present in v1.5.1
> if I have checked correctly.  The fix is about adding new code and should
> apply cleanly to 'maint' and even to older versions; the only trouble
> with older version might be whitespace issue related to refactoring
> code into subroutines.
> 
> The non-regexp project search bug was using $searchtext instead of
> $search_regexp as search regexp in gitweb.  The bug was present from
> the very addition of project search, namely commit 0d1d154 (gitweb:
> Support for simple project search form, 2008-10-03), which was present
> in v1.5.1 if I have checked correctly.  Unfortunately the fix affects
> code that was changed recently in a1e1b2d (gitweb: improve usability
> of projects search form, 2012-01-31); I'll try to come up with equivalent
> patch to 'maint' soon (if the current one does not apply, and I guess it
> doesn't).

In other words: while "*foo" is invalid regular expression, it is perfectly valid fixed string search term (which translates to "\*foo" regexp).

-- 
Jakub Narebski
Poland
Junio C Hamano· Mar 4, 2012, 23:08 UTC · re: Jakub Narebski · lore

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

Jakub Narebski <jnareb@gmail.com> writes:
> ....  The fix is about adding new code and should
> apply cleanly to 'maint' and even to older versions; the only trouble
> with older version might be whitespace issue related to refactoring
> code into subroutines.

OK, so the global $searchtext is what came from form submit from the end user, while the global $search_regexp is what the code should be using for matching throughout the program, prepared by eval-and-validate-params.

Here is a hand-ported version of your patch that should apply to 1.7.6.6; does it look sane?

 gitweb/gitweb.perl |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to gitweb/gitweb.perl +6 −6
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 50a835a..d1698b7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2905,10 +2905,10 @@ sub filter_forks_from_projects_list {
 sub search_projects_list {
 	my ($projlist, %opts) = @_;
 	my $tagfilter  = $opts{'tagfilter'};
-	my $searchtext = $opts{'searchtext'};
+	my $search_re = $opts{'search_regexp'};
 
 	return @$projlist
-		unless ($tagfilter || $searchtext);
+		unless ($tagfilter || $search_re);
 
 	my @projects;
  PROJECT:
@@ -2920,10 +2920,10 @@ sub search_projects_list {
 				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
 		}
 
-		if ($searchtext) {
+		if ($search_re) {
 			next unless
-				$pr->{'path'} =~ /$searchtext/ ||
-				$pr->{'descr_long'} =~ /$searchtext/;
+				$pr->{'path'} =~ /$search_re/ ||
+				$pr->{'descr_long'} =~ /$search_re/;
 		}
 
 		push @projects, $pr;
@@ -5097,7 +5097,7 @@ sub git_project_list_body {
 	@projects = fill_project_list_info(\@projects);
 	# searching projects require filling to be run before it
 	@projects = search_projects_list(\@projects,
-	                                 'searchtext' => $searchtext,
+	                                 'search_regexp' => $search_regexp,
 	                                 'tagfilter'  => $tagfilter)
 		if ($tagfilter || $searchtext);
Junio C Hamano· Mar 5, 2012, 05:16 UTC · re: Jakub Narebski · lore

Re: [PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

Jakub Narebski <jnareb@gmail.com> writes:
Show 30 quoted lines
> And here is the patch for maint
> -->8-- -------------------------------------------------------- -->8--
> Subject: gitweb: Fix fixed string (non-regexp) project search
>
> Use $search_regexp, where regex metacharacters are quoted, for
> searching projects list, rather than $searchtext, which contains
> original search term.
>
> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
>  gitweb/gitweb.perl |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d5dbd64..e248792 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5290,9 +5290,11 @@ sub git_project_list_body {
>  	@projects = fill_project_list_info(\@projects);
>  	# searching projects require filling to be run before it
>  	@projects = search_projects_list(\@projects,
> -	                                 'searchtext' => $searchtext,
> -	                                 'tagfilter'  => $tagfilter)
> -		if ($tagfilter || $searchtext);
> +	                                 'search_regexp' => $search_regexp,
> +	                                 'tagfilter' => $tagfilter)
> +		if ($tagfilter || $search_regexp);
> +	# fill the rest
> +	@projects = fill_project_list_info(\@projects);

Hmph, didn't you already call fill_project_list_info(\@projects) before search_projects_list() already?

Jakub Narebski· Mar 5, 2012, 08:59 UTC · re: Junio C Hamano · lore

Re: [PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

Junio C Hamano wrote:
Show 35 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > And here is the patch for maint
> > -->8-- -------------------------------------------------------- -->8--
> > Subject: gitweb: Fix fixed string (non-regexp) project search
> >
> > Use $search_regexp, where regex metacharacters are quoted, for
> > searching projects list, rather than $searchtext, which contains
> > original search term.
> >
> > Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> >  gitweb/gitweb.perl |   20 +++++++++++---------
> >  1 files changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index d5dbd64..e248792 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -5290,9 +5290,11 @@ sub git_project_list_body {
> >  	@projects = fill_project_list_info(\@projects);
> >  	# searching projects require filling to be run before it
> >  	@projects = search_projects_list(\@projects,
> > -	                                 'searchtext' => $searchtext,
> > -	                                 'tagfilter'  => $tagfilter)
> > -		if ($tagfilter || $searchtext);
> > +	                                 'search_regexp' => $search_regexp,
> > +	                                 'tagfilter' => $tagfilter)
> > +		if ($tagfilter || $search_regexp);
> > +	# fill the rest
> > +	@projects = fill_project_list_info(\@projects);
> 
> Hmph, didn't you already call fill_project_list_info(\@projects) before
> search_projects_list() already?
True.  Sorry about that. 
Can you fix that, or should I resend?
-- 
Jakub Narebski
Poland
Jakub Narebski· Mar 5, 2012, 09:03 UTC · re: Junio C Hamano · lore

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

Junio C Hamano wrote:
Show 57 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > ....  The fix is about adding new code and should
> > apply cleanly to 'maint' and even to older versions; the only trouble
> > with older version might be whitespace issue related to refactoring
> > code into subroutines.
> 
> OK, so the global $searchtext is what came from form submit from the end
> user, while the global $search_regexp is what the code should be using
> for matching throughout the program, prepared by eval-and-validate-params.
> 
> Here is a hand-ported version of your patch that should apply to 1.7.6.6;
> does it look sane?
> 
>  gitweb/gitweb.perl |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 50a835a..d1698b7 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2905,10 +2905,10 @@ sub filter_forks_from_projects_list {
>  sub search_projects_list {
>  	my ($projlist, %opts) = @_;
>  	my $tagfilter  = $opts{'tagfilter'};
> -	my $searchtext = $opts{'searchtext'};
> +	my $search_re = $opts{'search_regexp'};
>  
>  	return @$projlist
> -		unless ($tagfilter || $searchtext);
> +		unless ($tagfilter || $search_re);
>  
>  	my @projects;
>   PROJECT:
> @@ -2920,10 +2920,10 @@ sub search_projects_list {
>  				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
>  		}
>  
> -		if ($searchtext) {
> +		if ($search_re) {
>  			next unless
> -				$pr->{'path'} =~ /$searchtext/ ||
> -				$pr->{'descr_long'} =~ /$searchtext/;
> +				$pr->{'path'} =~ /$search_re/ ||
> +				$pr->{'descr_long'} =~ /$search_re/;
>  		}
>  
>  		push @projects, $pr;
> @@ -5097,7 +5097,7 @@ sub git_project_list_body {
>  	@projects = fill_project_list_info(\@projects);
>  	# searching projects require filling to be run before it
>  	@projects = search_projects_list(\@projects,
> -	                                 'searchtext' => $searchtext,
> +	                                 'search_regexp' => $search_regexp,
>  	                                 'tagfilter'  => $tagfilter)
>  		if ($tagfilter || $searchtext);
>  
It looks sane, though 
  	@projects = search_projects_list(\@projects,
 -	                                 'searchtext' => $searchtext,
 +	                                 'search_regexp' => $search_regexp,
  	                                 'tagfilter'  => $tagfilter)
  		if ($tagfilter || $searchtext);
should be better written as
  	@projects = search_projects_list(\@projects,
 -	                                 'searchtext' => $searchtext,
 +	                                 'search_regexp' => $search_regexp,
  	                                 'tagfilter'  => $tagfilter)
 - 		if ($tagfilter || $searchtext);
 + 		if ($tagfilter || $search_regexp);

It is functionally the same, because $search_regexp is derived from $searchtext, but IMHO it is more clear.

-- 
Jakub Narebski
Poland
Junio C Hamano· Mar 5, 2012, 17:01 UTC · re: Jakub Narebski · lore

Re: [PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

Jakub Narebski <jnareb@gmail.com> writes:
Show 15 quoted lines
> Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>> > And here is the patch for maint
>> ...
>> > +		if ($tagfilter || $search_regexp);
>> > +	# fill the rest
>> > +	@projects = fill_project_list_info(\@projects);
>> 
>> Hmph, didn't you already call fill_project_list_info(\@projects) before
>> search_projects_list() already?
>
> True.  Sorry about that. 
>
> Can you fix that, or should I resend?
Could you check the following two diffs?
$ git show debd1c2

This is jn/maint-do-not-match-with-unsanitized-searchtext that should be merged to maintenance track that lack the lazy filling.

And then
$ git show --first-parent d4b52c2

This is how the above was merged to 'pu' and the conflict resolution should be the same when we merge it to 'master'. As our @projects may still be only sparsely filled when search_projects_list() returns, we do call fill_project_list_info(\@projects) ourselves with the lazy filling codebase.

There are a few places I noticed that check $searchtext to see if we are running a search, and techinically $search_regexp might be a more correct thing to use, but I do not think it matters that much.

Thanks.
Ramsay Jones· Mar 5, 2012, 19:06 UTC · re: Jakub Narebski · lore

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

Jakub Narebski wrote:
Show 9 quoted lines
>> This patch solves the problem for me when using a regex search
>> (re checkbox checked), but *not* for a non-regex search.
>>
>> If you have a leading '*' or '+', in the non-regex case, then you
>> still get the above complaint (and xml error page etc.), although
>> the line number has changed slightly from that given above.
> 
> Ramsay, please provide those line number in the future, together with
> line and if possible some context.

Yeah, sorry about that; when I wrote that I didn't have the information readily available (I would have had to shutdown Windows, boot Linux, ...) and I was about to go out. So, the choice was to wait about 24hrs to report with full info, or provide the feedback earlier; I chose the latter. ;-)

>  gitweb/gitweb.perl |   22 +++++++++++-----------
>  1 files changed, 11 insertions(+), 11 deletions(-)
[patch snipped]
This patch works great for me. Thanks!

ATB, Ramsay Jones

Junio C Hamano· Mar 5, 2012, 23:27 UTC · re: Junio C Hamano · lore

Re: [PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

Junio C Hamano <gitster@pobox.com> writes:
Show 25 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
>
>>> Hmph, didn't you already call fill_project_list_info(\@projects) before
>>> search_projects_list() already?
>>
>> True.  Sorry about that. 
>>
>> Can you fix that, or should I resend?
>
> Could you check the following two diffs?
>
> $ git show debd1c2
>
> This is jn/maint-do-not-match-with-unsanitized-searchtext that
> should be merged to maintenance track that lack the lazy filling.
>
> And then
>
> $ git show --first-parent d4b52c2
>
> This is how the above was merged to 'pu' and the conflict resolution
> should be the same when we merge it to 'master'. As our @projects may
> still be only sparsely filled when search_projects_list() returns,
> we do call fill_project_list_info(\@projects) ourselves with the
> lazy filling codebase.
The latter is now
$ git show --first-parent 657c6d0
on today's 'pu'.
Thanks.
Jakub Narebski· Mar 6, 2012, 11:59 UTC · re: Junio C Hamano · lore

Re: [PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

Junio C Hamano wrote:
Show 34 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>> Jakub Narebski <jnareb@gmail.com> writes:
>>
>>>> Hmph, didn't you already call fill_project_list_info(\@projects) before
>>>> search_projects_list() already?
>>>
>>> True.  Sorry about that. 
>>>
>>> Can you fix that, or should I resend?
>>
>> Could you check the following two diffs?
>>
>> $ git show debd1c2
>>
>> This is jn/maint-do-not-match-with-unsanitized-searchtext that
>> should be merged to maintenance track that lack the lazy filling.
>>
>> And then
>>
>> $ git show --first-parent d4b52c2
>>
>> This is how the above was merged to 'pu' and the conflict resolution
>> should be the same when we merge it to 'master'. As our @projects may
>> still be only sparsely filled when search_projects_list() returns,
>> we do call fill_project_list_info(\@projects) ourselves with the
>> lazy filling codebase.
> 
> The latter is now
> 
> $ git show --first-parent 657c6d0
> 
> on today's 'pu'.
> 
> Thanks.

Both look all right (the only difference in diff is use of $searchtext vs $search_regexp global variable to check if search is on, but for that purpose those variables are equivalent).

Thanks.
-- 
Jakub Narebski
Poland
Jakub Narebski· Mar 6, 2012, 12:40 UTC · re: Ramsay Jones · lore

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

Ramsay Jones wrote:
> Jakub Narebski wrote:
Show 13 quoted lines
>>> This patch solves the problem for me when using a regex search
>>> (re checkbox checked), but *not* for a non-regex search.
>>>
>>> If you have a leading '*' or '+', in the non-regex case, then you
>>> still get the above complaint (and xml error page etc.), although
>>> the line number has changed slightly from that given above.
>> 
>> Ramsay, please provide those line number in the future, together with
>> line and if possible some context.
> 
> Yeah, sorry about that; when I wrote that I didn't have the information
> readily available (I would have had to shutdown Windows, boot Linux, ...)
> and I was about to go out.
You could have told us _that_...
> So, the choice was to wait about 24hrs to report 
> with full info, or provide the feedback earlier; I chose the latter. ;-)
That's understandable.
Thanks again for reporting these issues.
-- 
Jakub Narebski
Poland

← back to recent threads