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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 5, 2013 at 9:17 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Charles McGarvey <chazmcgarvey@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>

Acked-by: Jakub Narebski <jnareb@xxxxxxxxx>

>> ---
>> 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.

> 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.

>  * 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.

> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]