Re: REMINDER - stream wrappers in include_path

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