Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:36619
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 85.214.94.56 as permitted sender)
Date: Thu, 27 Mar 2008 19:17:55 +0100
Reply-To: Marcus Boerger <helly@php.net>
Message-ID: <1948764807.20080327191755@marcus-boerger.de>
To: Dmitry Stogov <dmitry@zend.com>
CC: Gregory Beaver <greg@chiaraquartet.net>, internals@lists.php.net
In-Reply-To: <47EB7813.8010509@zend.com>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] REMINDER - stream wrappers in include_path
From: helly@php.net (Marcus Boerger)

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