Re: [PATCH] gitweb: refactor input parameters parse/validation

["Giuseppe Bilotta" <giuseppe.bilotta@gmail.com>]

On Tue, Oct 7, 2008 at 4:39 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Giuseppe Bilotta wrote:
>> On Tue, Oct 7, 2008 at 12:57 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> On Fri, 3 Oct 2008, Giuseppe Bilotta wrote:
>
>>>> +     # find which part of PATH_INFO is project
>>>> +     my $project = $path_info;
>>>
>>> Hmmm... now $project is local (lexically) here.
>>
>> Yes, itt's only used temporarily here, to see if  a proper $project
>> can be defined. It gets redefined outside. It just made sense to name
>> it like this 8-)
>
> Well, if $project is local in evaluate_path_info(), so could be
> $path_info...

But $path_info in evaluate_path_info() has the same purpose and the
same (initial) data as $path_info outside of evaluate_path_info(), the
same is not true for $project.

>>>> +     $project =~ s,/+$,,;
>>>> +     while ($project && !check_head_link("$projectroot/$project")) {
>>>> +             $project =~ s,/*[^/]*$,,;
>>>> +     }
>>>> +     # validate project
>>>> +     $project = validate_project($project);
>>>
>>> I'm not sure if it is worth worrying over, but I think you repeat
>>> check_head_link() check here.
>>>
>>> [After examining code further].  But I think you do double validation;
>>> once you do it here, and once you do it copying to global variables
>>> such as $action or $project, and double checking check_head_link()
>>> won't be easy to avoid; fortunately it is cheap filesystem-level check
>>> (might be slow only when stat is extremely slow, and is not cached).
>>
>> I know. This is actually the reason why I had interleaved path_info
>> definition and global validation in my previous version of the patch.
>> The big issue here is that path_info evaluation _needs_ (partial)
>> validation.
>>
>> A possible alternative could be to only put validated parameters into
>> %input_params. This would completely separate the validation for cgi
>> and path_info (modulo shared subs).
>>
>> Of course, the check_head_link would still be repeated inside
>> evaluate_path_info, but the other params could skip a double
>> validation.
>
> Wouldn't it be simpler and as good solution to just leave validation
> off evaluate_path_info() (well, of course except check_head_link() test),
> and allow it to be validated when assigning global 'params' variables?
> check_head_link() would be repeated for path_info links, but that
> should not affect performance much.

Well, it does have a performance hit in the case of invalid $project
since it spends time working on the rest of the URL before bailing
out, but it's probably the cleanest solution. I'll do it this way.

-- 
Giuseppe "Oblomov" Bilotta