Does anybody see a problem with this patch (which is currently
against the 5.1.2 release)?
expand_filepath will eventual do a realpath, but it also uses the
realpath cache before calling realpath. VCWD_REALPATH just maps
directly to realpath and doesn't use the realpath cache so for every
request you get a realpath call (and all the syscalls that go with
it) on the primary file.
I think I need to pass TSRMLS_CC, but I am not sure.
Any problems committing it to PHP_5_1 and HEAD?
Brian
--- ../php-5.1.2.orig/main/main.c Sun Jan 1 04:50:17 2006
+++ main/main.c Thu Mar 9 20:00:47 2006
@@ -1684,7 +1684,7 @@
char realfile[MAXPATHLEN];
int realfile_len;
int dummy = 1;
-
if (VCWD_REALPATH(primary_file->filename,
realfile)) {
-
if (expand_filepath(primary_file->filename,
realfile TSRMLS_CC)) {
realfile_len = strlen(realfile);
zend_hash_add(&EG(included_files),
realfile, realfile_len+1, (void *)&dummy, sizeof(int), NULL);
if (strncmp(realfile, primary_file-
filename, realfile_len)) {
Are you sure VCWD_REALPATH doesn't use the realpath cache? It did
last time I checked and I think is still the right method to use there...
At 09:13 PM 3/9/2006, Brian J. France wrote:
Does anybody see a problem with this patch (which is currently
against the 5.1.2 release)?expand_filepath will eventual do a realpath, but it also uses the
realpath cache before calling realpath. VCWD_REALPATH just maps
directly to realpath and doesn't use the realpath cache so for every
request you get a realpath call (and all the syscalls that go with
it) on the primary file.I think I need to pass TSRMLS_CC, but I am not sure.
Any problems committing it to PHP_5_1 and HEAD?
Brian
--- ../php-5.1.2.orig/main/main.c Sun Jan 1 04:50:17 2006
+++ main/main.c Thu Mar 9 20:00:47 2006
@@ -1684,7 +1684,7 @@
char realfile[MAXPATHLEN];
int realfile_len;
int dummy = 1;
if (VCWD_REALPATH(primary_file->filename,realfile)) {
if (expand_filepath(primary_file->filename,realfile TSRMLS_CC)) {
realfile_len = strlen(realfile);
zend_hash_add(&EG(included_files),
realfile, realfile_len+1, (void *)&dummy, sizeof(int), NULL);
if (strncmp(realfile,
primary_file- >filename, realfile_len)) {
Andi Gutmans wrote:
Are you sure VCWD_REALPATH doesn't use the realpath cache? It did last
time I checked and I think is still the right method to use there...
quite
#define VCWD_REALPATH(path, real_path) realpath(path, real_path)
-Rasmus
Rasmus Lerdorf wrote:
Andi Gutmans wrote:
Are you sure VCWD_REALPATH doesn't use the realpath cache? It did last
time I checked and I think is still the right method to use there...quite
#define VCWD_REALPATH(path, real_path) realpath(path, real_path)
By the way, I agree that expand_filepath() is not the right fix here
since the first thing it does is a getcwd. We have just done a chdir
and haven't executed any script yet so we know the cwd. Feed that
directly to virtual_file_ex() there perhaps? I haven't really dug into it.
-Rasmus
Rasmus Lerdorf wrote:
Andi Gutmans wrote:
Are you sure VCWD_REALPATH doesn't use the realpath cache? It did
last time I checked and I think is still the right method to use
there...
quite
#define VCWD_REALPATH(path, real_path) realpath(path, real_path)By the way, I agree that expand_filepath() is not the right fix
here since the first thing it does is a getcwd. We have just done
a chdir and haven't executed any script yet so we know the cwd.
Feed that directly to virtual_file_ex() there perhaps? I haven't
really dug into it.
Were you seeing two getcwd calls? I can remove one, but I don't see
away around the second one. I changed to virtual_realpath which does
remove the getcwd in php_execute_script function, but it seems that
is shows up a little later when it uses the stream open which called
expand_filepath:
#0 0x203ab410 in __getcwd () from /usr/lib/libc.so.4
#1 0x203b2ab6 in getcwd () from /usr/lib/libc.so.4
#2 0x2060de86 in expand_filepath (filepath=0x2cbc08 "<path>/
bench_main.php", real_path=0x0)
at main/fopen_wrappers.c:521
#3 0x2061c1bd in _php_stream_fopen (filename=0x2cbc08 "<path>/
bench_main.php", mode=0x206f1fe3 "rb",
opened_path=0x9fbff7cc, options=133)
at /main/streams/plain_wrapper.c:855
#4 0x2061c974 in _php_stream_fopen_with_path (filename=0x2cbc08
"<path>/bench_main.php",
mode=0x206f1fe3 "rb", path=0x12b640 ".:/<path>/pear",
opened_path=0x9fbff7cc, options=133)
at /main/streams/plain_wrapper.c:1238
#5 0x2061c338 in php_plain_files_stream_opener (wrapper=0x2077c008,
path=0x2cbc08
"<path>/bench_main.php", mode=0x206f1fe3 "rb", options=133,
opened_path=0x9fbff7cc, context=0x0)
at/main/streams/plain_wrapper.c:931
#6 0x2061962e in _php_stream_open_wrapper_ex (path=0x2cbc08
"<path>bench_main.php",
mode=0x206f1fe3 "rb", options=141, opened_path=0x9fbff7cc,
context=0x0)
at main/streams/streams.c:1771
#7 0x2060943b in php_stream_open_for_zend (filename=0x2cbc08 "<path>/
bench_main.php",
handle=0x9fbff7c4) at main/main.c:901
#8 0x2064a866 in zend_stream_open (filename=0x2cbc08 "<path>/
bench_main.php",
handle=0x9fbff7c4) at Zend/zend_stream.c:47
#9 0x2064a8dc in zend_stream_fixup (file_handle=0x9fbff7c4) at Zend/
zend_stream.c:62
#10 0x20623ee7 in open_file_for_scanning (file_handle=0x9fbff7c4) at
Zend/zend_language_scanner.c:2982
#11 0x20623fce in compile_file (file_handle=0x9fbff7c4, type=2) at
Zend/zend_language_scanner.c:3068
#12 0x2063cd1c in zend_execute_scripts (type=8, retval=0x0,
file_count=3) at Zend/zend.c:1093
#13 0x2060a653 in php_execute_script (primary_file=0x9fbff7c4) at
main/main.c:1724
Brian
After poking this a bit more, it looks like this whole block of code in
main.c is redundant. Here is the story:
We start our journey on a dark and stormy night in June 2002 with this
bug: http://bugs.php.net/17720
This led to this fix:
http://cvs.php.net/viewcvs.cgi/php-src/main/main.c?r1=1.461&r2=1.462
which had this commit message:
Main script should not be parsed when (include|require)_once()'ed, #17720
Derick, MFH?
Now, at the time this was the correct fix. However, fast forward to
March 2005 and we have a patch from Stas to zend_execute_scripts:
http://cvs.php.net/viewcvs.cgi/ZendEngine2/zend.c?view=diff&r1=1.297&r2=1.298
which adds the opened file to the included_files list. This means that
we are now trying to add the main script to the included_file list twice
which in itself is bad.
We'll test this a bit more, but it looks safe to remove that block of
code. The test case in bug 17720 works fine without that block of code
in main.c and I see no other reason to have it there.
-Rasmus
Brian J. France wrote:
Rasmus Lerdorf wrote:
Andi Gutmans wrote:
Are you sure VCWD_REALPATH doesn't use the realpath cache? It did
last time I checked and I think is still the right method to use
there...
quite
#define VCWD_REALPATH(path, real_path) realpath(path, real_path)By the way, I agree that expand_filepath() is not the right fix here
since the first thing it does is a getcwd. We have just done a chdir
and haven't executed any script yet so we know the cwd. Feed that
directly to virtual_file_ex() there perhaps? I haven't really dug
into it.Were you seeing two getcwd calls? I can remove one, but I don't see
away around the second one. I changed to virtual_realpath which does
remove the getcwd in php_execute_script function, but it seems that is
shows up a little later when it uses the stream open which called
expand_filepath:#0 0x203ab410 in __getcwd () from /usr/lib/libc.so.4
#1 0x203b2ab6 in getcwd () from /usr/lib/libc.so.4
#2 0x2060de86 in expand_filepath (filepath=0x2cbc08
"<path>/bench_main.php", real_path=0x0)
at main/fopen_wrappers.c:521
#3 0x2061c1bd in _php_stream_fopen (filename=0x2cbc08
"<path>/bench_main.php", mode=0x206f1fe3 "rb",
opened_path=0x9fbff7cc, options=133)
at /main/streams/plain_wrapper.c:855
#4 0x2061c974 in _php_stream_fopen_with_path (filename=0x2cbc08
"<path>/bench_main.php",
mode=0x206f1fe3 "rb", path=0x12b640 ".:/<path>/pear",
opened_path=0x9fbff7cc, options=133)
at /main/streams/plain_wrapper.c:1238
#5 0x2061c338 in php_plain_files_stream_opener (wrapper=0x2077c008,
path=0x2cbc08
"<path>/bench_main.php", mode=0x206f1fe3 "rb", options=133,
opened_path=0x9fbff7cc, context=0x0)
at/main/streams/plain_wrapper.c:931
#6 0x2061962e in _php_stream_open_wrapper_ex (path=0x2cbc08
"<path>bench_main.php",
mode=0x206f1fe3 "rb", options=141, opened_path=0x9fbff7cc, context=0x0)
at main/streams/streams.c:1771
#7 0x2060943b in php_stream_open_for_zend (filename=0x2cbc08
"<path>/bench_main.php",
handle=0x9fbff7c4) at main/main.c:901
#8 0x2064a866 in zend_stream_open (filename=0x2cbc08
"<path>/bench_main.php",
handle=0x9fbff7c4) at Zend/zend_stream.c:47
#9 0x2064a8dc in zend_stream_fixup (file_handle=0x9fbff7c4) at
Zend/zend_stream.c:62
#10 0x20623ee7 in open_file_for_scanning (file_handle=0x9fbff7c4) at
Zend/zend_language_scanner.c:2982
#11 0x20623fce in compile_file (file_handle=0x9fbff7c4, type=2) at
Zend/zend_language_scanner.c:3068
#12 0x2063cd1c in zend_execute_scripts (type=8, retval=0x0,
file_count=3) at Zend/zend.c:1093
#13 0x2060a653 in php_execute_script (primary_file=0x9fbff7c4) at
main/main.c:1724Brian
Rasmus Lerdorf wrote:
After poking this a bit more, it looks like this whole block of code in
main.c is redundant. Here is the story:We start our journey on a dark and stormy night in June 2002 with this
bug: http://bugs.php.net/17720This led to this fix:
http://cvs.php.net/viewcvs.cgi/php-src/main/main.c?r1=1.461&r2=1.462
which had this commit message:
Main script should not be parsed when (include|require)_once()'ed, #17720Derick, MFH?
Now, at the time this was the correct fix. However, fast forward to
March 2005 and we have a patch from Stas to zend_execute_scripts:
http://cvs.php.net/viewcvs.cgi/ZendEngine2/zend.c?view=diff&r1=1.297&r2=1.298which adds the opened file to the included_files list. This means that
we are now trying to add the main script to the included_file list twice
which in itself is bad.We'll test this a bit more, but it looks safe to remove that block of
code. The test case in bug 17720 works fine without that block of code
in main.c and I see no other reason to have it there.
Just as a final follow-up to this. Further testing showed that in CLI
mode where we pass in a ZEND_HANDLE_FP instead of a ZEND_HANDLE_FILENAME
we still need this block of code. So the fix is to check and only skip
this block if we are passed a ZEND_HANDLE_FILENAME. Commit coming up.
-Rasmus
Seems that needs fixing then (non-TSRM). We should support the
realpath cache also in non-TSRM mode.
At 10:31 PM 3/9/2006, Rasmus Lerdorf wrote:
Andi Gutmans wrote:
Are you sure VCWD_REALPATH doesn't use the realpath cache? It did
last time I checked and I think is still the right method to use there...quite
#define VCWD_REALPATH(path, real_path) realpath(path, real_path)
-Rasmus