# REMINDER - stream wrappers in include_path

- Thread: https://externals.io/message/36228
- Original archive: https://news-web.php.net/php.internals/36228
- Started by Gregory Beaver on 2008-03-20T23:53:30+00:00
- Messages: 39
- Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL)
- Next page: https://externals.io/message/36228.md?page=2
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #36228 — Gregory Beaver — 2008-03-20T23:53:30+00:00

URL: https://externals.io/message/36228#36228

Hi,

Please look at the stream wrappers in include_path patch I posted last week.

Thanks,
Greg

---

## #36229 — Steph Fox — 2008-03-21T04:43:10+00:00

URL: https://externals.io/message/36228#36229 · In reply to: #36228

Hi Greg,

> Please look at the stream wrappers in include_path patch I posted last 
> week.

I went to try this, but the patch doesn't apply to 5_3 any more since the 
re2c change. Could you post an updated version please?

Thanks,

- Steph

---

## #36231 — Dmitry Stogov — 2008-03-21T09:13:38+00:00

URL: https://externals.io/message/36228#36231 · In reply to: #36228

Hi Greg,

I've fixed your patch to work with all functions (fopen, file_get_contente).
Please verify it with ext/phar and then I'll commit it.

Thanks. Dmitry.

Gregory Beaver wrote:
> Hi,
> 
> Please look at the stream wrappers in include_path patch I posted last week.
> 
> Thanks,
> Greg

---

## #36238 — Gregory Beaver — 2008-03-21T16:53:05+00:00

URL: https://externals.io/message/36228#36238 · In reply to: #36231

Dmitry Stogov wrote:
> Hi Greg,
>
> I've fixed your patch to work with all functions (fopen,
> file_get_contente).
> Please verify it with ext/phar and then I'll commit it.
>
> Thanks. Dmitry.
>
> Gregory Beaver wrote:
> > […]
Hi Dmitry,

the patch has a few major problems and a few minor ones.  First off,
there are spaces instead of tabs, which is not a huge deal, I've
corrected that in the attached patch.

It is, however, logically void to do stream wrapper operations in
plain_wrapper.c.  I removed that code entirely.  Instead, the path
resolve is now done in php_stream_open_wrapper_ex where it belongs,
before determining which stream wrapper to open.  This will result in a
double call to the function when calling include, but because the 2nd
call will simply return after determining it has received a full path.

I've attached 2 patches (apparently, Zend is not included in cvs diff -u
of php5), 1 for adding path resolution to include/require (this is a
significant performance improvement over the existing code).  The second
patch adds stream wrapper support to both php_resolve_path and
_php_stream_open_wrapper_ex, which automatically gives stream wrapper in
include_path support to all current and future functions that use
include_path.

Greg

---

## #36243 — Dmitry Stogov — 2008-03-21T17:16:51+00:00

URL: https://externals.io/message/36228#36243 · In reply to: #36238

Gregory Beaver wrote:
> Dmitry Stogov wrote:
> > […]
> Hi Dmitry,
> 
> the patch has a few major problems and a few minor ones.  First off,
> there are spaces instead of tabs, which is not a huge deal, I've
> corrected that in the attached patch.

thanks.

> It is, however, logically void to do stream wrapper operations in
> plain_wrapper.c.  I removed that code entirely.  Instead, the path
> resolve is now done in php_stream_open_wrapper_ex where it belongs,
> before determining which stream wrapper to open.

this is probably better, but then we should probably remove include_path 
from plain_file_wrapper at all.

> This will result in a
> double call to the function when calling include, but because the 2nd
> call will simply return after determining it has received a full path.

I'll need to look into it more careful.

> I've attached 2 patches (apparently, Zend is not included in cvs diff -u
> of php5), 1 for adding path resolution to include/require (this is a
> significant performance improvement over the existing code).  The second
> patch adds stream wrapper support to both php_resolve_path and
> _php_stream_open_wrapper_ex, which automatically gives stream wrapper in
> include_path support to all current and future functions that use
> include_path.

I'm not sure about performance improvement. This patch might make 
performance degradation especially for PHP with opcode caches.

I'll need to look into the patches and benchmark them.
Sorry, but I'll able to do it only on next week.
May be you'll get some ideas and improve the patch before I'll start.

Thanks. Dmitry.

> Greg

---

## #36420 — Dmitry Stogov — 2008-03-24T11:39:13+00:00

URL: https://externals.io/message/36228#36420 · In reply to: #36238

Hi Greg,

In your second patch you forgot "break", so it couldn't work.
I don't see any reason in second patch at all, because you call 
php_resolve_patch() from _php_stream_open_wrapper_ex() anyway.

The bad thing with the first part that it calls `realpath()` function 
twice for each include("relative_path.php") and it makes several 
file-system accesses (look into `strace ... 2>&1| grep lstat`). With 
your patch I see 6 syscalls more.

So I would prefer my patch that is more efficient for regular files.
The same patch with fixed white-spaces is attached.

Thanks. Dmitry.

Gregory Beaver wrote:
> Dmitry Stogov wrote:
> > […]
> Hi Dmitry,
> 
> the patch has a few major problems and a few minor ones.  First off,
> there are spaces instead of tabs, which is not a huge deal, I've
> corrected that in the attached patch.
> 
> It is, however, logically void to do stream wrapper operations in
> plain_wrapper.c.  I removed that code entirely.  Instead, the path
> resolve is now done in php_stream_open_wrapper_ex where it belongs,
> before determining which stream wrapper to open.  This will result in a
> double call to the function when calling include, but because the 2nd
> call will simply return after determining it has received a full path.
> 
> I've attached 2 patches (apparently, Zend is not included in cvs diff -u
> of php5), 1 for adding path resolution to include/require (this is a
> significant performance improvement over the existing code).  The second
> patch adds stream wrapper support to both php_resolve_path and
> _php_stream_open_wrapper_ex, which automatically gives stream wrapper in
> include_path support to all current and future functions that use
> include_path.
> 
> Greg

---

## #36423 — Gregory Beaver — 2008-03-24T13:28:00+00:00

URL: https://externals.io/message/36228#36423 · In reply to: #36420

Dmitry Stogov wrote:
> Hi Greg,
>
> In your second patch you forgot "break", so it couldn't work.
> I don't see any reason in second patch at all, because you call
> php_resolve_patch() from _php_stream_open_wrapper_ex() anyway.
>
> The bad thing with the first part that it calls `realpath()` function
> twice for each include("relative_path.php") and it makes several
> file-system accesses (look into `strace ... 2>&1| grep lstat`). With
> your patch I see 6 syscalls more.
>
> So I would prefer my patch that is more efficient for regular files.
> The same patch with fixed white-spaces is attached. 

Your patch is broken, it causes an endless loop in
tests/include_path.phpt when running phar tests.

Incidentally, I was unaware of the strace command (I've seen it bandied
about on irc, but thought it was something else), so thank you for
edumacatin me there.

The attached patch has no additional syscalls over the current case for
both existing and non-existing files, and actually works for phar as
well :).  It's also simpler (I removed the include stuff from
zend_vm_def.h), although you may want to add it back in and bench it to
see if it makes any difference in performance, since I found such a
dramatic reduction in operations here with it.

Greg

---

## #36424 — Marcus Boerger — 2008-03-24T13:39:52+00:00

URL: https://externals.io/message/36228#36424 · In reply to: #36423

Hello Gregory,

Monday, March 24, 2008, 2:28:00 PM, you wrote:

> Index: main/fopen_wrappers.c
> ===================================================================
> RCS file: /repository/php-src/main/fopen_wrappers.c,v
> retrieving revision 1.175.2.3.2.13.2.9
> diff -u -r1.175.2.3.2.13.2.9 fopen_wrappers.c
> --- main/fopen_wrappers.c       24 Mar 2008 09:30:41 -0000      1.175.2.3.2.13.2.9
> +++ main/fopen_wrappers.c       24 Mar 2008 13:24:34 -0000
> @@ -473,7 +473,15 @@
>  
>         ptr = path;
>         while (ptr && *ptr) {
> -               end = strchr(ptr, DEFAULT_DIR_SEPARATOR);
> +               /* Check for stream wrapper */
> +               int is_stream_wrapper = 0;
> +
> +               for (p = ptr; isalnum((int)*p) || *p == '+' || *p == '-' || *p == '.'; p++);
> +               if ((*p == ':') && (p - ptr > 1) && (p[1] == '/') && (p[2] == '/')) {
However how about:
if ((p > ptr) && !memcmp(p, "://", 3)) {
> +                       p += 2;
> +                       is_stream_wrapper = 1;
> +               }
[...]
> @@ -511,6 +538,29 @@
>                     exec_fname_length + 1 + filename_length + 1 < MAXPATHLEN) {
>                         memcpy(trypath, exec_fname, exec_fname_length + 1);
>                         memcpy(trypath+exec_fname_length + 1, filename, filename_length+1);
> +
> +                       /* Check for stream wrapper */
> +                       for (p = trypath; isalnum((int)*p) || *p == '+' || *p == '-' || *p == '.'; p++);
> +                       if ((*p == ':') && (p - trypath > 1) && (p[1] == '/') && (p[2] == '/')) {
Same as above
> +                               char *actual;
> Index: main/php_streams.h
> ===================================================================
> RCS file: /repository/php-src/main/php_streams.h,v
> retrieving revision 1.103.2.1.2.4.2.2
> diff -u -r1.103.2.1.2.4.2.2 php_streams.h
> --- main/php_streams.h  31 Dec 2007 07:17:17 -0000      1.103.2.1.2.4.2.2
> +++ main/php_streams.h  24 Mar 2008 13:24:34 -0000
> @@ -511,6 +511,12 @@
>  /* don't check allow_url_fopen and allow_url_include */
>  #define STREAM_DISABLE_URL_PROTECTION   0x00002000
>  
> +/* assume the path passed in exists and is fully expanded, avoiding syscalls */
> +#define ASSUME_REALPATH                 0x00004000
> +
> +/* assume the path passed in does not exist, and fail (plain wrapper only) */
> +#define REALPATH_FAILED                 0x00008000
> +
I think prefixinf with STREAMS_ would be good.
>  /* Antique - no longer has meaning */
>  #define IGNORE_URL_WIN 0
>  

Best regards,
 Marcus

---

## #36425 — Gregory Beaver — 2008-03-24T13:47:25+00:00

URL: https://externals.io/message/36228#36425 · In reply to: #36424

Marcus Boerger wrote:
> Hello Gregory,
>
> Monday, March 24, 2008, 2:28:00 PM, you wrote:
>
>   
> > […]
> However how about:
> if ((p > ptr) && !memcmp(p, "://", 3)) {
this would unfortunately break UNC paths on unix, which start with //. 
We have to verify that the stuff between : and : in the previous thing
is a valid stream wrapper, which means it can only be
alphanumeric/+/-/.  However, I imagine the memcmp could be applied as if
((*p == ':') && (p - ptr > 1) && !memcmp(p, "://", 3))

Would that really be faster?
> I think prefixinf with STREAMS_ would be good.
OK, updated and attached

Greg

---

## #36426 — Marcus Boerger — 2008-03-24T13:51:06+00:00

URL: https://externals.io/message/36228#36426 · In reply to: #36425

Hello Gregory,

Monday, March 24, 2008, 2:47:25 PM, you wrote:

> Marcus Boerger wrote:
> > […]
> this would unfortunately break UNC paths on unix, which start with //. 
> We have to verify that the stuff between : and : in the previous thing
> is a valid stream wrapper, which means it can only be
> alphanumeric/+/-/.  However, I imagine the memcmp could be applied as if
> ((*p == ':') && (p - ptr > 1) && !memcmp(p, "://", 3))

I must be missign something here. First p - ptr > 1 === p > ptr
Second your do all && so I can reorder and get three consecutive char
checks. Hence memcmp is the better option and clearer to read imo.

> Would that really be faster?
> > […]
> OK, updated and attached

> Greg



Best regards,
 Marcus

---

## #36427 — Dmitry Stogov — 2008-03-24T14:05:22+00:00

URL: https://externals.io/message/36228#36427 · In reply to: #36426

The direct comparison must be faster than memcmp().
memcmp() might be optimized into the same code but probably won't be.

Thanks. Dmitry.

Marcus Boerger wrote:
> Hello Gregory,
> 
> Monday, March 24, 2008, 2:47:25 PM, you wrote:
> 
> > […]
> 
> I must be missign something here. First p - ptr > 1 === p > ptr
> Second your do all && so I can reorder and get three consecutive char
> checks. Hence memcmp is the better option and clearer to read imo.
> 
> > […]
> 
> > […]
> 
> 
> 
> Best regards,
>  Marcus

---

## #36429 — Dmitry Stogov — 2008-03-24T14:20:12+00:00

URL: https://externals.io/message/36228#36429 · In reply to: #36423

REALPATH_FAILED looks like a hack :(
PHP itself doesn't need it at all, according to your patch 
php_stream_open() may just return `NULL` immediately instead of setting of 
this flag.
However I am not sure that it will work for all cases. The fact that we 
cannot resolve the path doesn't mean that we cannot open the file. (On 
some systems `getcwd()` may fail on some reasons).

I didn't test the patch with pecl/phar.
Could you explain why php goes into the endless loop?

Thanks. Dmitry.

Gregory Beaver wrote:
> Dmitry Stogov wrote:
> > […]
> 
> Your patch is broken, it causes an endless loop in
> tests/include_path.phpt when running phar tests.
> 
> Incidentally, I was unaware of the strace command (I've seen it bandied
> about on irc, but thought it was something else), so thank you for
> edumacatin me there.
> 
> The attached patch has no additional syscalls over the current case for
> both existing and non-existing files, and actually works for phar as
> well :).  It's also simpler (I removed the include stuff from
> zend_vm_def.h), although you may want to add it back in and bench it to
> see if it makes any difference in performance, since I found such a
> dramatic reduction in operations here with it.
> 
> Greg

---

## #36431 — Gregory Beaver — 2008-03-24T14:41:32+00:00

URL: https://externals.io/message/36228#36431 · In reply to: #36429

Dmitry Stogov wrote:
> REALPATH_FAILED looks like a hack :(
> PHP itself doesn't need it at all, according to your patch
> php_stream_open() may just return `NULL` immediately instead of setting
> of this flag.
> However I am not sure that it will work for all cases. The fact that
> we cannot resolve the path doesn't mean that we cannot open the file.
> (On some systems `getcwd()` may fail on some reasons).
I remember Solaris having issues with this, now that you mention it.

However, the place that I added this already returned `NULL` if realpath
fails, it doesn't change the behavior, just moves the realpath to an
earlier position and eliminates an unnecessary double call.  However, if
we are comfortable having the double realpath on files not found in
include_path (this would only have a performance affect on apps with
lots of conditional loading of drivers where the majority are not
found), this is fine with me.  I've attached a patch that removes
REALPATH_FAILED so there are options.

Another option would be to determine which OSes this can fail, and
simply not use REALPATH_FAILED on those OSes.
> I didn't test the patch with pecl/phar.
> Could you explain why php goes into the endless loop?
because the cut/paste to plain_wrapper.c:1419 needs a ptr = end, unlike
the code in fopen_wrappers.c.  Once you fix this problem, then the real
logic problem rears its head, which is that we go into plain_wrapper to
open an external stream wrapper.  It is far less efficient (about 4-6%
by my measurements) to do this, which is why I moved the include_path
parsing into streams.c

This bug only happens when the last item in include_path is a stream
wrapper and the file is not found in include_path.

Greg

---

## #36432 — Dmitry Stogov — 2008-03-24T15:06:21+00:00

URL: https://externals.io/message/36228#36432 · In reply to: #36431

Gregory Beaver wrote:
> Dmitry Stogov wrote:
> > […]
> I remember Solaris having issues with this, now that you mention it.
> 
> However, the place that I added this already returned `NULL` if realpath
> fails, it doesn't change the behavior, just moves the realpath to an
> earlier position and eliminates an unnecessary double call.  However, if
> we are comfortable having the double realpath on files not found in
> include_path (this would only have a performance affect on apps with
> lots of conditional loading of drivers where the majority are not
> found), this is fine with me.  I've attached a patch that removes
> REALPATH_FAILED so there are options.
> 
> Another option would be to determine which OSes this can fail, and
> simply not use REALPATH_FAILED on those OSes.
> > […]
> because the cut/paste to plain_wrapper.c:1419 needs a ptr = end, unlike
> the code in fopen_wrappers.c.

You are right.

> Once you fix this problem, then the real
> logic problem rears its head, which is that we go into plain_wrapper to
> open an external stream wrapper.  It is far less efficient (about 4-6%
> by my measurements) to do this, which is why I moved the include_path
> parsing into streams.c

Did you measure pecl/phar or regular files?

> This bug only happens when the last item in include_path is a stream
> wrapper and the file is not found in include_path.

In general I don't have objections except of changing 
zend_resolve_path() (which is a callback for ZE) into 
php_resolve_path(). Anyway I'll look into patch once again with fresh head.

I would also ask Stas and other interested people to look into it.

Thanks. Dmitry.

---

## #36433 — Gregory Beaver — 2008-03-24T15:08:03+00:00

URL: https://externals.io/message/36228#36433 · In reply to: #36432

Dmitry Stogov wrote:
>
>
> Gregory Beaver wrote:
> > […]
>
> You are right.
>
> > […]
>
> Did you measure pecl/phar or regular files?
I only measured regular files, my concern is that we don't affect any
existing users, 100% of whom use regular files at the moment (obviously :).
>
> > […]
>
> In general I don't have objections except of changing
> zend_resolve_path() (which is a callback for ZE) into
> php_resolve_path(). Anyway I'll look into patch once again with fresh
> head.
>
> I would also ask Stas and other interested people to look into it. 

Thanks Dmitry,

Greg

---

## #36441 — Stanislav Malyshev — 2008-03-24T17:09:34+00:00

URL: https://externals.io/message/36228#36441 · In reply to: #36425

> this would unfortunately break UNC paths on unix, which start with //. 

Unix has UNC paths?
-- 
Stanislav Malyshev, Zend Software Architect
stas@zend.com   http://www.zend.com/
(408)253-8829   MSN: stas@zend.com

---

## #36448 — Gregory Beaver — 2008-03-24T19:20:29+00:00

URL: https://externals.io/message/36228#36448 · In reply to: #36441

Stanislav Malyshev wrote:
> > […]
> 
> Unix has UNC paths?

Beats me, I'm sure posix-based systems don't, but it is called "Uniform
Naming Convention" so it's possible somebody might implement it
(http://en.wikipedia.org/wiki/Path_(computing)#Uniform_Naming_Convention).

:)
Greg

---

## #36450 — Stanislav Malyshev — 2008-03-24T19:28:49+00:00

URL: https://externals.io/message/36228#36450 · In reply to: #36448

> Beats me, I'm sure posix-based systems don't, but it is called "Uniform
> Naming Convention" so it's possible somebody might implement it

The fact that it's called "uniform" doesn't mean it works in anything 
but Windows :) In UNIX IIRC if one needs to access SMB volume the 
regular smb:// URLs are used, or tool-specific syntax. If you have 
something like automount, then you have regular /-based URLs, but never 
headr of // having any special meaning different from just /.

-- 
Stanislav Malyshev, Zend Software Architect
stas@zend.com   http://www.zend.com/
(408)253-8829   MSN: stas@zend.com

---

## #36464 — Andi Gutmans — 2008-03-24T22:16:26+00:00

URL: https://externals.io/message/36228#36464 · In reply to: #36231

Can we please use strlcpy() instead of strncpy()? This is a coding
standard we implemented years ago.

Thanks.

Andi

> -----Original Message-----
> From: Dmitry Stogov [mailto:dmitry@zend.com]
> Sent: Friday, March 21, 2008 2:14 AM
> To: Gregory Beaver
> Cc: internals Mailing List; Stas Malyshev
> Subject: Re: [PHP-DEV] REMINDER - stream wrappers in include_path
> 
> Hi Greg,
> 
> I've fixed your patch to work with all functions (fopen,
> file_get_contente).
> Please verify it with ext/phar and then I'll commit it.
> 
> Thanks. Dmitry.
> 
> Gregory Beaver wrote:
> > […]
> last week.
> > […]

---

## #36466 — Greg Beaver — 2008-03-24T22:59:40+00:00

URL: https://externals.io/message/36228#36466 · In reply to: #36464

Andi Gutmans wrote:
> Can we please use strlcpy() instead of strncpy()? This is a coding
> standard we implemented years ago.

obviously an easy change.  FYI - this also needs to be fixed in 
fopen_with_path_rel in PHP_5_2, as I copied most of the code from that 
function.

Greg

---

## #36476 — Dmitry Stogov — 2008-03-25T09:20:47+00:00

URL: https://externals.io/message/36228#36476 · In reply to: #36466

I hope it's the last iteration, but check me anyway.
The patch is based on latest Gregory's patch.

- optimized out strncpy() calls
- zend_resolve_path() replaced with php_resolve_path()
- improved php_resolve_path() to resolve "file://..."
- fixed possible double-free issue in _php_stream_open_wrapper_ex()

Thanks. Dmitry.

Greg Beaver wrote:
> Andi Gutmans wrote:
> > […]
> 
> obviously an easy change.  FYI - this also needs to be fixed in 
> fopen_with_path_rel in PHP_5_2, as I copied most of the code from that 
> function.
> 
> Greg

---

## #36498 — Gregory Beaver — 2008-03-25T14:13:26+00:00

URL: https://externals.io/message/36228#36498 · In reply to: #36476

Dmitry Stogov wrote:
> I hope it's the last iteration, but check me anyway.
> The patch is based on latest Gregory's patch.
>
> - optimized out strncpy() calls
> - zend_resolve_path() replaced with php_resolve_path()
> - improved php_resolve_path() to resolve "file://..."
> - fixed possible double-free issue in _php_stream_open_wrapper_ex()
>
> Thanks. Dmitry.
>
> Greg Beaver wrote:
> > […]
works great here - commit with all due haste :)

Greg

---

## #36501 — Dmitry Stogov — 2008-03-25T15:05:25+00:00

URL: https://externals.io/message/36228#36501 · In reply to: #36498

I'm going to commit it tomorrow.

Thanks. Dmitry.

Gregory Beaver wrote:
> Dmitry Stogov wrote:
> > […]
> works great here - commit with all due haste :)
> 
> Greg

---

## #36503 — Stefan Walk — 2008-03-25T15:13:19+00:00

URL: https://externals.io/message/36228#36503 · In reply to: #36450

On Monday 24 March 2008 20:28:49 Stanislav Malyshev wrote:
> > […]
>
> The fact that it's called "uniform" doesn't mean it works in anything
> but Windows :) In UNIX IIRC if one needs to access SMB volume the
> regular smb:// URLs are used, or tool-specific syntax. If you have
> something like automount, then you have regular /-based URLs, but never
> headr of // having any special meaning different from just /.

a) // is valid for windows too
b) cygwin uses //

Regards,
Stefan

---

## #36504 — Dmitry Stogov — 2008-03-25T15:22:00+00:00

URL: https://externals.io/message/36228#36504 · In reply to: #36503

:(
Any ideas how to support this?

Dmitry.

Stefan Walk wrote:
> On Monday 24 March 2008 20:28:49 Stanislav Malyshev wrote:
> > […]
> 
> a) // is valid for windows too
> b) cygwin uses //
> 
> Regards,
> Stefan

---

Next page: https://externals.io/message/36228.md?page=2