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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
