Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:36611
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: <47EB7813.8010509@zend.com>
Date: Thu, 27 Mar 2008 13:33:55 +0300
User-Agent: Thunderbird 2.0.0.9 (Windows/20071031)
MIME-Version: 1.0
To: Gregory Beaver <greg@chiaraquartet.net>
CC: Marcus Boerger <helly@php.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>
In-Reply-To: <47EAABAB.4040604@chiaraquartet.net>
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 Greg,

in php_resolve_path()

- you removed (p - filename > 1) check but it means that c://tmp will be 
  used as stream wrapper

- 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