Re: [PATCH] gitweb: fix problem causing erroneous project list

From: Jakub Narębski <jnareb@gmail.com>
Date: Jun 7, 2013, 20:39 UTC
Message-ID: <CANQwDwfiuNSk+woFheqfi527rfutZ2YYH1h3tjuWh0ziwmU+uQ@mail.gmail.com>
In-Reply-To: <7vd2s01i6f.fsf@alter.siamese.dyndns.org>

On Wed, Jun 5, 2013 at 9:17 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Charles McGarvey <chazmcgarvey@brokenzipper.com> writes:
>
>> The bug is manifest when running gitweb in a persistent process (e.g.
>> FastCGI, PSGI), and it's easy to reproduce.  If a gitweb request
>> includes the searchtext parameter (i.e. s), subsequent requests using
>> the project_list action--which is the default action--and without
>> a searchtext parameter will be filtered by the searchtext value of the
>> first request.  This is because the value of the $search_regexp global
>> (the value of which is based on the searchtext parameter) is currently
>> being persisted between requests.
>>
>> Instead, clear $search_regexp before dispatching each request.
>>
>> Signed-off-by: Charles McGarvey <chazmcgarvey@brokenzipper.com>

Acked-by: Jakub Narebski <jnareb@gmail.com>

>> ---
>> I don't think there are currently any persistent-process gitweb tests to
>> copy from, so writing a test for this seems to be non-trivial.

Well, there is Plack::Test, and gitweb supports running as PSGI app. Though all such test would have to be under new PLACK or PSGI prerequisite.

Show 8 quoted lines

> The problem description makes sense to me, and clearing with "undef"
> is in line with the case where the CGI is run for the first time, so
> I think this patch is correct.
>
> Will wait for a while to give Jakub some time to comment on (like:
> Ack ;-) and then apply.  Thanks.
>
> By the way, I looked at how $search_regexp is used in the code:

How $search_regexp is used does not matter. What was intended (but was not implemented) is for $search_regexp to matter and to be used only if $searchtext is defined. $searchtext is reset on each request, so $search_regexp should be also reset... like in Charles's patch.

Anyway in the analysis below we need to check if code checks $searchtext first.

Show 9 quoted lines

>  * esc_html_match_hl and esc_html_match_hl__chopped (called from
>    git_project_list_rows, for example) want to have "undef"; an
>    empty string will not do.
>
>  * search_projects_list (called from git_project_list_body)
>
>  x git_search_files and git_search_grep_body assume that
>    $search_regexp can be interpolated in m//, which is not very
>    nice.  They want an empty string.

But both git_search_files() and git_search_grep_body() are run from git_search(), which "dies" (returns HTTP 400 "Text field is empty" error) if $searchtext is not defined; if $searchtext is defined then $search_regexp is string and is never undef.

Show 20 quoted lines

> So as an independent fix, the two subs may want to be fixed if we
> want to be undef clean.  Or am I missing something?  Jakub, isn't
> this kind of "undef" reference t9500 was designed to catch?
>
>>  gitweb/gitweb.perl | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 80950c0..8d69ada 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -1086,7 +1086,7 @@ sub evaluate_and_validate_params {
>>       our $search_use_regexp = $input_params{'search_use_regexp'};
>>
>>       our $searchtext = $input_params{'searchtext'};
>> -     our $search_regexp;
>> +     our $search_regexp = undef;
>>       if (defined $searchtext) {
>>               if (length($searchtext) < 2) {
>>                       die_error(403, "At least two characters are required for search parameter");

-- 
Jakub Narebski