Everyone on the road?

php.internals

Andi Gutmans

22 years ago
Hey, It's a bit quiet here lately :) Has anyone had a chance to try, test and benchmark the realpath() patch I send to the list? Andi

Rasmus Lerdorf

22 years ago
On Sat, 24 Jul 2004, Andi Gutmans wrote:
> Has anyone had a chance to try, test and benchmark the realpath() patch I > send to the list?
I had a look the other day, but here is a more detailed look at the system calls involved in a php5 request. I have stripped out most of the stuff we can't do anything about. The script I am testing is /var/home/rasmus/php5/o which contains: <?php include '/var/www/lerdorf.com/abc'; include '/var/www/lerdorf.com/abc'; include '/var/www/lerdorf.com/def'; include '/var/www/phpics.com/ghi'; ?> The system calls required to open and read the script: getcwd("/var/home/rasmus/php5", 4096) = 22 lstat64("/var/home/rasmus/php5/o", {st_mode=S_IFREG|0644, st_size=156, ...}) = 0 setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={0, 0}}, NULL) = 0 rt_sigaction(SIGPROF, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART, 0x406ea658}, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART, 0x406ea658}, 8) = 0 rt_sigprocmask(SIG_UNBLOCK, [PROF], NULL, 8) = 0 read(3, "<?php\n\tinclude \'/var/www/lerdorf"..., 8192) = 156 read(3, "", 4096) = 0 read(3, "", 8192) = 0 close(3) = 0 ok so far. Now the first include: getcwd("/var/home/rasmus/php5", 4096) = 22 lstat64("/var", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 lstat64("/var/www", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 lstat64("/var/www/lerdorf.com", {st_mode=S_IFDIR|0777, st_size=16384, ...}) = 0 lstat64("/var/www/lerdorf.com/abc", {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 open("/var/www/lerdorf.com/abc", O_RDONLY) = 3 fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 lseek(3, 0, SEEK_CUR) = 0 read(3, "abc\n", 8192) = 4 read(3, "", 8192) = 0 read(3, "", 8192) = 0 close(3) = 0 Since nothing is cached yet we see realpath() statting the entire tree leading to the abc include file. There is an obvious double fstat on the opened descriptor there that we should track down. Then we get: fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}) = 0 old_mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x40018000 write(1, "abc\n", 4) = 4 To write out the results. The second time we include the same file the stat cache kicks in and we have: getcwd("/var/home/rasmus/php5", 4096) = 22 time(NULL) = 1090679437 open("/var/www/lerdorf.com/abc", O_RDONLY) = 3 fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 lseek(3, 0, SEEK_CUR) = 0 read(3, "abc\n", 8192) = 4 read(3, "", 8192) = 0 read(3, "", 8192) = 0 close(3) = 0 write(1, "abc\n", 4) = 4 Not sure where that time() call came from here, and again we see the double fstat. And since we have already started writing the output the results are written out immediately in a single write. Now the /var/www/lerdorf.com/def include: getcwd("/var/home/rasmus/php5", 4096) = 22 lstat64("/var", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 lstat64("/var/www", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 lstat64("/var/www/lerdorf.com", {st_mode=S_IFDIR|0777, st_size=16384, ...}) = 0 lstat64("/var/www/lerdorf.com/def", {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 time(NULL) = 1090679437 open("/var/www/lerdorf.com/def", O_RDONLY) = 3 fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 lseek(3, 0, SEEK_CUR) = 0 read(3, "def\n", 8192) = 4 read(3, "", 8192) = 0 read(3, "", 8192) = 0 close(3) = 0 write(1, "def\n", 4) = 4 We get what we expect here too. Since the stat cache only caches full paths and we are now including a different file from the same location we have to stat the whole tree again. And the final include: getcwd("/var/home/rasmus/php5", 4096) = 22 lstat64("/var", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 lstat64("/var/www", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 lstat64("/var/www/phpics.com", {st_mode=S_IFDIR|0755, st_size=8192, ...}) = 0 lstat64("/var/www/phpics.com/ghi", {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 time(NULL) = 1090679437 open("/var/www/phpics.com/ghi", O_RDONLY) = 3 fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0 lseek(3, 0, SEEK_CUR) = 0 read(3, "ghi\n", 8192) = 4 read(3, "", 8192) = 0 read(3, "", 8192) = 0 close(3) = 0 write(1, "ghi\n", 4) = 4 No surprises there either. Shutting down (cli version): close(1) = 0 close(0) = 0 setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={0, 0}}, NULL) = 0 setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={30, 0}}, NULL) = 0 rt_sigaction(SIGPROF, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART, 0x406ea658}, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART, 0x406ea658}, 8) = 0 rt_sigprocmask(SIG_UNBLOCK, [PROF], NULL, 8) = 0 setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={0, 0}}, NULL) = 0 munmap(0x40018000, 4096) = 0 exit_group(0) = ? I'd love to see a realpath() replacement function which makes use of the stat cache for partial paths as well. Chances are that on a busy ISP server, for example, you will have thousands of scripts and include files served up from the same base path and you are going to be statting your way along that tree every time you hit a file that isn't in the cache. And that double-fstat needs to go away. A very quick look for it seems to show one checking if the opened file is a regular file in fopen_wrappers.c. Ideally we would simply use the stat struct from our own realpath() implementation and neither of these fstat() calls would be needed. The goal should be to average at most 1 stat per file, whether it be the main script or include files. For the main script, if we are running under Apache, Apache has already done this stat and passed us the stat struct -Rasmus

Stefan Esser

22 years ago
Hi,
> I'd love to see a realpath() replacement function which makes use of the > stat cache for partial paths as well. Chances are that on a busy ISP > server, for example, you will have thousands of scripts and include files > served up from the same base path and you are going to be statting your > way along that tree every time you hit a file that isn't in the cache.
It is a very good idea to replace realpath() with an own function. Realpath is not only slow, it is also "dangerous" on some systems like linux where it also works on paths that are not really existing. example: include "./foo/bar/index.php/../../../../../../etc/passwd"; On the BSDs this should be invalid, but linux happily accepts it as /etc/passwd. This is just one thing that would largely benefit from an own realpath(). Stefan

Christian Schneider

22 years ago
Stefan Esser wrote:
> Realpath is not only slow, it is also "dangerous" on some systems like > linux where it also works on paths that are not really existing. > > example: include "./foo/bar/index.php/../../../../../../etc/passwd";
This is a completely legal unix path as .. in / is / again. And from a security point of view I'd say the only thing you gain by disallowing this is that the number of .. in the path have to be guessed (pretty easily) by the attacker. Or am I missing something here? Not that I can see the benefit of a home-grown realpath() :-) - Chris

Stefan Esser

22 years ago
Christian Schneider wrote:
>> example: include "./foo/bar/index.php/../../../../../../etc/passwd";
> This is a completely legal unix path as .. in / is / again. And from a > security point of view I'd say the only thing you gain by disallowing > this is that the number of .. in the path have to be guessed (pretty > easily) by the attacker.
This is no legal unix path, because index.php is a file and not a directory. And to understand the security impact: include "./foo/bar/template_".$userinput; By setting $userinput to "validtemplate.php/../../../../etc/passwd" you get to the /etc/passwd file, altough the path is completely illegal. Stefan

Christian Schneider

22 years ago
Stefan Esser wrote:
> This is no legal unix path, because index.php is a file and not a
Oops, missed that part of the path, just looked at the .. :-)
> And to understand the security impact: > include "./foo/bar/template_".$userinput;
... which I'd consider bad practice anyway but that's another story :-) - Chris

Andi Gutmans

22 years ago
At 06:30 PM 7/24/2004 +0200, Christian Schneider wrote:
>>And to understand the security impact: >>include "./foo/bar/template_".$userinput; > >... which I'd consider bad practice anyway but that's another story :-)
Yep. That person deserves to have his site hacked :) Andi

Andi Gutmans

22 years ago
Hi, Thanks for the in-depth analysis. I agree that we should nuke that extra fstat(). We need to track down where it is (probably streams :). I don't think it's a good idea to cache the stat() itself because usually you don't stat() the same file twice in the same request, and caching those in between requests could lead to weird bugglets such as Last-Modified: not changing even though you changed the script. I think the realpath() is the main performance problem and easiest to deal with as very rarely will the real path change so caching them can't do too much damage. The time() call is part of the patch to check if we should refresh the realpath cache for this specific path. If it becomes a problem then we might need to think of a less deterministic approach where we call time() only every few requests. Another idea I had was to call time() once at the beginning of the request and always use the same one for reference. I don't see any major problem with this as far as realpath is concerned. It might even be useful in other places where not the exact time is needed but the execution start time is good enough. I think it's going to be quite hard and not really worthwhile to add support for partial realpath(). In this case I like the kiss approach. It gives a great bang for the buck and is very simple. By the way, if we use the VIRTUAL_DIR support that getcwd() is also saved. Not sure if we want to convert (it might be a bit too dangerous) but it's something to keep in the back of the mind. Andi At 08:06 AM 7/24/2004 -0700, Rasmus Lerdorf wrote:

Rasmus Lerdorf

22 years ago
On Sun, 25 Jul 2004, Andi Gutmans wrote:
> I think it's going to be quite hard and not really worthwhile to add > support for partial realpath(). In this case I like the kiss approach. It > gives a great bang for the buck and is very simple.
Well, there is another approach here. There are only 2 places where we need the full paths. The first is for the included_files list to be able to do the require_once/include_once check and the second is for the open_basedir mechanism. There are a lot of people out there who do not use open_basedir and I would guess most people looking for maximum performance wouldn't be in a situation where they needed the open_basedir feature. So the only real feature in question here is the *_once checking and even there you don't necessarily need the full path although without it you could potentially run into problems if you include the same file through 2 different relative paths or more obscurely if you deliberately change your cwd halfway through your script and include a file of the same name through the same relative path but this is not actually the same file. In both cases you are basically going out of your way to trick the _once check. At Yahoo I have been running everything with relative paths for well over a year now without any problems. So, some approaches: a. compile-time switch to disable full paths completely b. ini switch to disable full paths completely unless open_basedir is enabled c. ini switch to disable full paths from include/require but enable them for include_once/require_once. That is, if you want *_once protection you use include_once/require_once. If you don't, you just use include/require and include/require would not populate the included_files list. -Rasmus

Andi Gutmans

22 years ago
I prefer using the caching to solve this problem, because according to my tests, with a fairly small amount of memory, the caching pretty much solves the problem completely. So I'm thinking that if we go with the approach of this patch, we can solve the problem with a very small amount of simple code, and get a great performance boost (without requiring any INI switches or equivalent). By the way, Sara, Wez, did you see Rasmus's email about the double stat? Do you know where it's coming from? Andi At 11:19 AM 7/25/2004 -0700, Rasmus Lerdorf wrote:

Wez Furlong

22 years ago
I'm supposed to be writing this weekend, so technically I shouldn't have time to dig into it until next weekend. That means that I probably will find some time this weekend ;-) --Wez. On Fri, 30 Jul 2004 15:05:13 -0700, Andi Gutmans <andi@zend.com> wrote:

Wez Furlong

22 years ago
Nuked duplicate fstats in HEAD. On Sat, 31 Jul 2004 10:05:12 +0100, Wez Furlong <kingwez@gmail.com> wrote:

Andi Gutmans

22 years ago
:) At 11:10 AM 7/31/2004 +0100, Wez Furlong wrote:

Rasmus Lerdorf

22 years ago
On Sun, 25 Jul 2004, Andi Gutmans wrote:
> The time() call is part of the patch to check if we should refresh the > realpath cache for this specific path. If it becomes a problem then we > might need to think of a less deterministic approach where we call time() > only every few requests. Another idea I had was to call time() once at the > beginning of the request and always use the same one for reference. I don't > see any major problem with this as far as realpath is concerned. It might > even be useful in other places where not the exact time is needed but the > execution start time is good enough.
I meant to mention this a while ago, but it slipped my mind. Most web servers do that time() call for us at the beginning of the request because they need it for logging purposes. I think the right approach here is to add a SAPI call to expose this. For Apache-1.3 it is right in the request_rec in the request_time field. Obviously the SAPI call would simply do the time() call itself if it can't fetch it from somewhere internally. -Rasmus

Sara Golemon

22 years ago
> I meant to mention this a while ago, but it slipped my mind. Most web > servers do that time() call for us at the beginning of the request because > they need it for logging purposes. I think the right approach here is to > add a SAPI call to expose this. For Apache-1.3 it is right in the > request_rec in the request_time field. Obviously the SAPI call would > simply do the time() call itself if it can't fetch it from somewhere > internally. >
It sounds good enough, but I'd worry about people using this in incompletely-thought-out benchmarking scripts. A SAPI which supports the request time record would appear to take slightly longer than a SAPI which does not (the time between processing the request in the web server and passing off to PHP {plus compile, plus execute down to that instructuion}) <note> Do <emphasis>NOT</emphasis> use this method for benchmarking between different webservers, inconsistent implementations could give unexpected results. </note> -Sara

Rasmus Lerdorf

22 years ago
On Fri, 6 Aug 2004, Sara Golemon wrote:
> > I meant to mention this a while ago, but it slipped my mind. Most web > > servers do that time() call for us at the beginning of the request because > > they need it for logging purposes. I think the right approach here is to > > add a SAPI call to expose this. For Apache-1.3 it is right in the > > request_rec in the request_time field. Obviously the SAPI call would > > simply do the time() call itself if it can't fetch it from somewhere > > internally. > > > It sounds good enough, but I'd worry about people using this in > incompletely-thought-out benchmarking scripts.
Adding a SAPI call doesn't mean exposure to scripts, so I don't really see how this is relevant. Sure, someone could write a PHP extension to benchmark different servers, but what are the chances of that? And if they are advanced enough to write an extension, they are smart enough to normalize the request start times. -Rasmus

Sara Golemon

22 years ago
> Adding a SAPI call doesn't mean exposure to scripts, so I don't really see > how this is relevant. Sure, someone could write a PHP extension to > benchmark different servers, but what are the chances of that? And if > they are advanced enough to write an extension, they are smart enough to > normalize the request start times. >
Sorry, I misread... I thought you were talking about exposing this call to userspace. -Sara

Andi Gutmans

22 years ago
At 08:59 AM 8/6/2004 -0700, Sara Golemon wrote:
> > I meant to mention this a while ago, but it slipped my mind. Most web > > servers do that time() call for us at the beginning of the request because > > they need it for logging purposes. I think the right approach here is to > > add a SAPI call to expose this. For Apache-1.3 it is right in the > > request_rec in the request_time field. Obviously the SAPI call would > > simply do the time() call itself if it can't fetch it from somewhere > > internally. > > >It sounds good enough, but I'd worry about people using this in >incompletely-thought-out benchmarking scripts. > >A SAPI which supports the request time record would appear to take slightly >longer than a SAPI which does not (the time between processing the request >in the web server and passing off to PHP {plus compile, plus execute down to >that instructuion}) > ><note> > Do <emphasis>NOT</emphasis> use this method for benchmarking between >different webservers, inconsistent implementations could give unexpected >results. ></note>
If you call it get_request_start_time() it should make it clearer what it means. I guess we could put it into SAPI I don't see any disadvantage although in any case, doing it one more time per-request wouldn't be that bad either. I hope to have more time towards the end of next week and then I can hopefully make this improvement. Andi

Rasmus Lerdorf

22 years ago
On Fri, 6 Aug 2004, Andi Gutmans wrote:
> If you call it get_request_start_time() it should make it clearer what it > means. > I guess we could put it into SAPI I don't see any disadvantage although in > any case, doing it one more time per-request wouldn't be that bad either. > I hope to have more time towards the end of next week and then I can > hopefully make this improvement.
Sure, it's a single syscall, but this entire effort is about removing syscalls, so adding one that isn't necessary because it has already been done during the request makes sense. I am just cleaning up some other code where a sapi call for this would make things a lot cleaner. Without the sapi call you end up needing to do this: #if HAVE_APACHE #include "httpd.h" #endif ... #if HAVE_APACHE t = ((request_rec *)SG(server_context))->request_time; #else t = time(0); #endif -Rasmus

George Schlossnagle

22 years ago
On Aug 6, 2004, at 5:12 PM, Rasmus Lerdorf wrote:
> On Fri, 6 Aug 2004, Andi Gutmans wrote: >> If you call it get_request_start_time() it should make it clearer >> what it >> means. >> I guess we could put it into SAPI I don't see any disadvantage >> although in >> any case, doing it one more time per-request wouldn't be that bad >> either. >> I hope to have more time towards the end of next week and then I can >> hopefully make this improvement. > > Sure, it's a single syscall, but this entire effort is about removing > syscalls, so adding one that isn't necessary because it has already > been > done during the request makes sense.
I didn't imagine this would be so controversial. I'm all for this added callout. George

Andi Gutmans

22 years ago
At 05:21 PM 8/6/2004 -0400, George Schlossnagle wrote:
>On Aug 6, 2004, at 5:12 PM, Rasmus Lerdorf wrote: > >>On Fri, 6 Aug 2004, Andi Gutmans wrote: >>>If you call it get_request_start_time() it should make it clearer what it >>>means. >>>I guess we could put it into SAPI I don't see any disadvantage although in >>>any case, doing it one more time per-request wouldn't be that bad either. >>>I hope to have more time towards the end of next week and then I can >>>hopefully make this improvement. >> >>Sure, it's a single syscall, but this entire effort is about removing >>syscalls, so adding one that isn't necessary because it has already been >>done during the request makes sense. > >I didn't imagine this would be so controversial. I'm all for this added >callout.
It's not. Andi

Andi Gutmans

22 years ago
Go ahead and add it. I'll use the new SAPI call in our patch. At 02:12 PM 8/6/2004 -0700, Rasmus Lerdorf wrote:

Rasmus Lerdorf

22 years ago
On Fri, 6 Aug 2004, Andi Gutmans wrote:
> Go ahead and add it. I'll use the new SAPI call in our patch.
I'm out the door for a weekend in Tahoe, but I will add it when I get back in a couple of days. -Rasmus