{"thread":{"id":"34031","subject":"[PATCH] gitweb: fix problem causing erroneous project list","startedAt":"2013-06-05T04:44:28Z","lastAt":"2013-06-07T21:59:36Z","messageCount":4,"participants":["Charles McGarvey","Junio C Hamano","Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"219433","messageId":"20130605043524.GA2453@compy.Home","threadId":"34031","inReplyTo":null,"subject":"[PATCH] gitweb: fix problem causing erroneous project list","fromName":"Charles McGarvey","fromEmail":"chazmcgarvey@brokenzipper.com","sentAt":"2013-06-05T04:44:28Z","receivedAt":"2013-06-05T04:44:28Z","isPatch":true,"sender":{"key":"chazmcgarvey@brokenzipper.com","avatar":"https://avatars.githubusercontent.com/u/108998?v=4"},"body":"The bug is manifest when running gitweb in a persistent process (e.g.\nFastCGI, PSGI), and it's easy to reproduce.  If a gitweb request\nincludes the searchtext parameter (i.e. s), subsequent requests using\nthe project_list action--which is the default action--and without\na searchtext parameter will be filtered by the searchtext value of the\nfirst request.  This is because the value of the $search_regexp global\n(the value of which is based on the searchtext parameter) is currently\nbeing persisted between requests.\n\nInstead, clear $search_regexp before dispatching each request.\n\nSigned-off-by: Charles McGarvey <chazmcgarvey@brokenzipper.com>\n---\nI don't think there are currently any persistent-process gitweb tests to\ncopy from, so writing a test for this seems to be non-trivial.\n\n gitweb/gitweb.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 80950c0..8d69ada 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1086,7 +1086,7 @@ sub evaluate_and_validate_params {\n \tour $search_use_regexp = $input_params{'search_use_regexp'};\n \n \tour $searchtext = $input_params{'searchtext'};\n-\tour $search_regexp;\n+\tour $search_regexp = undef;\n \tif (defined $searchtext) {\n \t\tif (length($searchtext) < 2) {\n \t\t\tdie_error(403, \"At least two characters are required for search parameter\");\n-- \n1.8.1.5\n"},{"id":"219467","messageId":"7vd2s01i6f.fsf@alter.siamese.dyndns.org","threadId":"34031","inReplyTo":"20130605043524.GA2453@compy.Home","subject":"Re: [PATCH] gitweb: fix problem causing erroneous project list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-05T19:17:12Z","receivedAt":"2013-06-05T19:17:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles McGarvey <chazmcgarvey@brokenzipper.com> writes:\n\n> The bug is manifest when running gitweb in a persistent process (e.g.\n> FastCGI, PSGI), and it's easy to reproduce.  If a gitweb request\n> includes the searchtext parameter (i.e. s), subsequent requests using\n> the project_list action--which is the default action--and without\n> a searchtext parameter will be filtered by the searchtext value of the\n> first request.  This is because the value of the $search_regexp global\n> (the value of which is based on the searchtext parameter) is currently\n> being persisted between requests.\n>\n> Instead, clear $search_regexp before dispatching each request.\n>\n> Signed-off-by: Charles McGarvey <chazmcgarvey@brokenzipper.com>\n> ---\n> I don't think there are currently any persistent-process gitweb tests to\n> copy from, so writing a test for this seems to be non-trivial.\n\nThe problem description makes sense to me, and clearing with \"undef\"\nis in line with the case where the CGI is run for the first time, so\nI think this patch is correct.\n\nWill wait for a while to give Jakub some time to comment on (like:\nAck ;-) and then apply.  Thanks.\n\nBy the way, I looked at how $search_regexp is used in the code:\n\n * esc_html_match_hl and esc_html_match_hl__chopped (called from\n   git_project_list_rows, for example) want to have \"undef\"; an\n   empty string will not do.\n\n * search_projects_list (called from git_project_list_body)\n\n x git_search_files and git_search_grep_body assume that\n   $search_regexp can be interpolated in m//, which is not very\n   nice.  They want an empty string.\n\nSo as an independent fix, the two subs may want to be fixed if we\nwant to be undef clean.  Or am I missing something?  Jakub, isn't\nthis kind of \"undef\" reference t9500 was designed to catch?\n\n>  gitweb/gitweb.perl | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 80950c0..8d69ada 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1086,7 +1086,7 @@ sub evaluate_and_validate_params {\n>  \tour $search_use_regexp = $input_params{'search_use_regexp'};\n>  \n>  \tour $searchtext = $input_params{'searchtext'};\n> -\tour $search_regexp;\n> +\tour $search_regexp = undef;\n>  \tif (defined $searchtext) {\n>  \t\tif (length($searchtext) < 2) {\n>  \t\t\tdie_error(403, \"At least two characters are required for search parameter\");\n"},{"id":"219704","messageId":"CANQwDwfiuNSk+woFheqfi527rfutZ2YYH1h3tjuWh0ziwmU+uQ@mail.gmail.com","threadId":"34031","inReplyTo":"7vd2s01i6f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: fix problem causing erroneous project list","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-06-07T20:39:45Z","receivedAt":"2013-06-07T20:39:45Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, Jun 5, 2013 at 9:17 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Charles McGarvey <chazmcgarvey@brokenzipper.com> writes:\n>\n>> The bug is manifest when running gitweb in a persistent process (e.g.\n>> FastCGI, PSGI), and it's easy to reproduce.  If a gitweb request\n>> includes the searchtext parameter (i.e. s), subsequent requests using\n>> the project_list action--which is the default action--and without\n>> a searchtext parameter will be filtered by the searchtext value of the\n>> first request.  This is because the value of the $search_regexp global\n>> (the value of which is based on the searchtext parameter) is currently\n>> being persisted between requests.\n>>\n>> Instead, clear $search_regexp before dispatching each request.\n>>\n>> Signed-off-by: Charles McGarvey <chazmcgarvey@brokenzipper.com>\n\nAcked-by: Jakub Narebski <jnareb@gmail.com>\n\n>> ---\n>> I don't think there are currently any persistent-process gitweb tests to\n>> copy from, so writing a test for this seems to be non-trivial.\n\nWell, there is Plack::Test, and gitweb supports running as PSGI app.\nThough all such test would have to be under new PLACK or PSGI\nprerequisite.\n\n> The problem description makes sense to me, and clearing with \"undef\"\n> is in line with the case where the CGI is run for the first time, so\n> I think this patch is correct.\n>\n> Will wait for a while to give Jakub some time to comment on (like:\n> Ack ;-) and then apply.  Thanks.\n>\n> By the way, I looked at how $search_regexp is used in the code:\n\nHow $search_regexp is used does not matter. What was intended\n(but was not implemented) is for $search_regexp to matter and to\nbe used only if $searchtext is defined.  $searchtext is reset on each\nrequest, so $search_regexp should be also reset... like in Charles's\npatch.\n\nAnyway in the analysis below we need to check if code checks\n$searchtext first.\n\n>  * esc_html_match_hl and esc_html_match_hl__chopped (called from\n>    git_project_list_rows, for example) want to have \"undef\"; an\n>    empty string will not do.\n>\n>  * search_projects_list (called from git_project_list_body)\n>\n>  x git_search_files and git_search_grep_body assume that\n>    $search_regexp can be interpolated in m//, which is not very\n>    nice.  They want an empty string.\n\nBut both git_search_files() and git_search_grep_body() are run from\ngit_search(), which \"dies\" (returns HTTP 400 \"Text field is empty\" error)\nif $searchtext is not defined; if $searchtext is defined then $search_regexp\nis string and is never undef.\n\n> So as an independent fix, the two subs may want to be fixed if we\n> want to be undef clean.  Or am I missing something?  Jakub, isn't\n> this kind of \"undef\" reference t9500 was designed to catch?\n>\n>>  gitweb/gitweb.perl | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 80950c0..8d69ada 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -1086,7 +1086,7 @@ sub evaluate_and_validate_params {\n>>       our $search_use_regexp = $input_params{'search_use_regexp'};\n>>\n>>       our $searchtext = $input_params{'searchtext'};\n>> -     our $search_regexp;\n>> +     our $search_regexp = undef;\n>>       if (defined $searchtext) {\n>>               if (length($searchtext) < 2) {\n>>                       die_error(403, \"At least two characters are required for search parameter\");\n\n-- \nJakub Narebski\n"},{"id":"219744","messageId":"7vd2rxshtj.fsf@alter.siamese.dyndns.org","threadId":"34031","inReplyTo":"CANQwDwfiuNSk+woFheqfi527rfutZ2YYH1h3tjuWh0ziwmU+uQ@mail.gmail.com","subject":"Re: [PATCH] gitweb: fix problem causing erroneous project list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-07T21:59:36Z","receivedAt":"2013-06-07T21:59:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski <jnareb@gmail.com> writes:\n\n>>> Instead, clear $search_regexp before dispatching each request.\n>>>\n>>> Signed-off-by: Charles McGarvey <chazmcgarvey@brokenzipper.com>\n>\n> Acked-by: Jakub Narebski <jnareb@gmail.com>\n\nThanks (the ack was a few hours too late and the commit is already\nin 'next', so I won't be able to rewind it though).\n\n>> By the way, I looked at how $search_regexp is used in the code:\n>\n> How $search_regexp is used does not matter. What was intended\n> (but was not implemented) is for $search_regexp to matter and to\n> be used only if $searchtext is defined.  $searchtext is reset on each\n> request, so $search_regexp should be also reset... like in Charles's\n> patch.\n\nOh, we are in total agreement about that.  That is why the part is\nmarked with \"By the way\"---it is an orthogonal issue (which turned\nout to be a non-issue).\n\n>>  x git_search_files and git_search_grep_body assume that\n>>    $search_regexp can be interpolated in m//, which is not very\n>>    nice.  They want an empty string.\n>\n> But both git_search_files() and git_search_grep_body() are run from\n> git_search(), which \"dies\" (returns HTTP 400 \"Text field is empty\" error)\n> if $searchtext is not defined; if $searchtext is defined then $search_regexp\n> is string and is never undef.\n\nThanks; that is what I missed.\n\n>> So as an independent fix, the two subs may want to be fixed if we\n>> want to be undef clean.  Or am I missing something?\n"}]}