Re: REMINDER - stream wrappers in include_path

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