Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:36636
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 212.25.124.162 as permitted sender)
Message-ID: <47ECB24A.5070804@zend.com>
Date: Fri, 28 Mar 2008 11:54:34 +0300
User-Agent: Thunderbird 2.0.0.12 (X11/20080226)
MIME-Version: 1.0
To: Marcus Boerger <helly@php.net>
CC: Gregory Beaver <greg@chiaraquartet.net>, internals@lists.php.net
References: <47E2F8FA.20107@chiaraquartet.net> <47E7FEFD.8080504@chiaraquartet.net> <47E800F1.5090101@zend.com> <200803251613.20245.et@php.net> <47E91898.5050503@zend.com> <47E92D3B.7010500@chiaraquartet.net> <47E930C0.6000201@zend.com> <47E94C24.2030400@chiaraquartet.net> <1891676371.20080325201729@marcus-boerger.de> <47E9C725.4060308@chiaraquartet.net> <99395441.20080326122856@marcus-boerger.de> <47EAABAB.4040604@chiaraquartet.net> <47EB7813.8010509@zend.com> <1948764807.20080327191755@marcus-boerger.de>
In-Reply-To: <1948764807.20080327191755@marcus-boerger.de>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] REMINDER - stream wrappers in include_path
From: dmitry@zend.com (Dmitry Stogov)

Hi Marcus,

Sorry, if I broke something in latest patch, I just didn't see any
comments from others about it. And from my point of view the patch did
unnecessary things. I still think the same, becaus I mainly changed code
that checks for wrapper in given argument, but not in include_path.

BTW: I might miss something.

Greg, could you please make a patch that fix the problem that Marcus
describes (don't forget a test case).

Thanks. Dmitry.

Marcus Boerger wrote:
> Hello Dmitry,
> 
> Thursday, March 27, 2008, 11:33:55 AM, you wrote:
> 
>> Hi Greg,
> 
>> in php_resolve_path()
> 
>> - you removed (p - filename > 1) check but it means that c://tmp will be 
>>   used as stream wrapper
> 
> Basically he doesn't need to. The code with your modifiactions is wrong now.
> He changed it to verify that the potential wrapper indeed exists and if not
> he continues as normal. Checking for '.' or '..' are only very extreme and
> potential common cases. We however need to make sure that nothing that
> really is no wrapper gets used as a wrapper. That means if wrapper is NULL,
> then continue as we would have done without the check.
> 
> Right now, the following "xx://root" is seen as relative "xx" and absolute
> "//root". Becasue of your change we get xx as wrapper. That resolves to NULL
> that is not equal to &plain_wrapper so that we return NULL.
> 
> Please do not change patches that were reviewed by several people right
> before commit. Becasue each and every reviewer will assume that you apply
> as reviewed. And if you comment, then please do so before you commit and
> not after. Even if you are right you first want to make sure and convince
> the reviewers that you are. Otherwise we can spare the time for reviews.
> 
> marcus
> 
>> - you added check for !wrapper, but we don't need to do any filesystem 
>> search for filename with wrong wrapper
> 
>> I've removed this modifications.
>> I also added proper assignment to opened_path.
> 
>> I've committed the patch into PHP_5_3 and HEAD.
> 
>> Thanks. Dmitry.
> 
>> Gregory Beaver wrote:
>>> Marcus Boerger wrote:
>>>> Hello Gregory,
>>>>
>>>> +               for (p = ptr; isalnum((int)*p) || *p == '+' || *p == '-' || *p == '.'; p++);
>>>> +               /* p - ptr > 1 allows us to skip things like "C:\whatever" */
>>>> +               if ((*p == ':') && (p - ptr > 1) && (path_len - (p - path) > 2) && (p[1] == '/') && (p[2] == '/')) {
>>>> +                       /* .:// or ..:// is not a stream wrapper */
>>>> +                       if (p[-1] != '.' || p[-2] != '.' || p - 2 != ptr) {
>>>> +                               p += 3;
>>>> +                               is_stream_wrapper = 1;
>>>> +                       }
>>>> +               }
>>>>
>>>> You missed one part though. C stops execution of a boolean expression on
>>>> the first one that decides on the result. So if p[1] is '\0' then p[2] will
>>>> never be accessed. So there is no access violation at all.
>>> good point (i.e. duh on my part).  attached patch removes that
>>> unnecessary paranoia.
>>>
>>>> Analyzing the check for '..:', took a long time :-) And I  like that we
>>>> check for this common case without going to the wrapper list. And we do not
>>>> need to check for the common case '.' either as you require two chars in
>>>> front of the ':', cool!
>>> I found a few minor optimizations of this code just now, attached patch
>>> should be even better.
>>>
>>>> However with the check below:
>>>>
>>>> +       if ((*p == ':') && (filename_length - (p - filename) > 2) && (p[1] == '/') && (p[2] == '/')) {
>>>> +               wrapper = php_stream_locate_url_wrapper(filename, &actual_path, STREAM_OPEN_FOR_INCLUDE TSRMLS_CC);                     
>>>> +               if (wrapper == &php_plain_files_wrapper) {
>>>> +                       if (tsrm_realpath(actual_path, resolved_path TSRMLS_CC)) {
>>>> +                               return estrdup(resolved_path);
>>>> +                       }
>>>> +               }
>>>>                 return NULL;
>>>>
>>>> Don't we need to check for wrapper being NULL as in:
>>>> if (!wrapper || wrapper == &php_plain_files_wrapper) {
>>> Probably, I've added that in too.
>>>
>>> Greg
> 
> 
> 
> 
> Best regards,
>  Marcus
>