Re: cvs commit: httpd-2.0/server request.c
On Thu, Dec 12, 2002 at 11:31:44AM -0500, Paul J. Reder wrote: [EMAIL PROTECTED] wrote: wrowe 2002/12/11 23:05:54 Modified:server request.c Log: Make the code simpler to follow, and perhaps clear up the follow-symlink bug reports we have seen on bugzilla. e.g. 14206 etc. Sorry to be the bearer of bad news but the problem reported in 14206 still occurs with this new code. [snip] Now bring up your browser and request: http://your.machine.name:port/index.html You'll get a 403:forbidden error. http://your.machine.name:port/ You'll get the page foo.html. I can spend more time tracking this if you want, but it won't be till this afternoon. Can I point out the mail archived at http://marc.theaimsgroup.com/?l=apache-httpd-devm=103938644204488w=2 sent on December 8th, which has some discussion of this issue. It might give some pointers as to where to start looking. The patch there probably won't apply cleanly any more, of course. All the best, f -- Francis Daly[EMAIL PROTECTED]
Re: cvs commit: httpd-2.0/server request.c
[EMAIL PROTECTED] wrote: wrowe 2002/12/11 23:05:54 Modified:server request.c Log: Make the code simpler to follow, and perhaps clear up the follow-symlink bug reports we have seen on bugzilla. e.g. 14206 etc. Revision ChangesPath 1.122 +23 -43httpd-2.0/server/request.c Sorry to be the bearer of bad news but the problem reported in 14206 still occurs with this new code. All you have to do is the following: In your htdocs directory: mv index.html foo.html ln -s foo.html index.html In your httpd.conf: # Note: Options should not allow FollowSymLinks Directory / Options None AllowOverride None /Directory Directory /home/Apache/htdocs Options None AllowOverride None Order deny,allow Allow from all /Directory Now bring up your browser and request: http://your.machine.name:port/index.html You'll get a 403:forbidden error. http://your.machine.name:port/ You'll get the page foo.html. I can spend more time tracking this if you want, but it won't be till this afternoon. -- Paul J. Reder --- The strength of the Constitution lies entirely in the determination of each citizen to defend it. Only if every single citizen feels duty bound to do his share in this defense are the constitutional rights secure. -- Albert Einstein
Re: cvs commit: httpd-2.0/server request.c
On Fri, Nov 01, 2002 at 03:27:20AM -, [EMAIL PROTECTED] wrote:
...
+++ request.c 1 Nov 2002 03:27:20 - 1.118
@@ -924,6 +924,8 @@
/* That temporary trailing slash was useful, now drop it.
*/
if (temp_slash) {
+temp_slash = 0;
+AP_ASSERT(r-filename[filename_len-1] == '/');
Don't you want
*temp_slash = '\0';
??
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server request.c
William A. Rowe, Jr. [EMAIL PROTECTED] writes:
Folks, this looks wrong after consideration. If someone is familiar
with the Linux gcc optimizer, please see my last comments in
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14147
I'm starting to feel like the optimizer bit us.
That was the first thing I thought. However, since that loop does not
fit on one screen it took a bit more concentration to see what was
happening. Those gotos make it a little more interesting :)
do {
int temp_slash=0;
if (condition met) {
temp_slash=1;
}
...
minimerge:
...
minimerge2:
if (temp_slash) {
r-filename[--filename_len] = '\0';
}
if (matches) {
if (last_walk-matched == sec_ent[sec_idx]) {
goto minimerge; X
}
}
} while (thisinfo.filetype == APR_DIR);
The line marked is one place where we go back to a point
before zapping the last char of r-filename without clearing
temp_slash. And since filename_len is decremented, we will zap a
different char the next time.
I didn't check if there are other places where can can go back earlier
in the loop without temp_slash being cleared. But since there are two
goto destinations that seems possible.
It does boggle my mind that we developers apparently never hit this.
I tried to duplicate the mod_dav problem multiple times with no luck.
William A. Rowe, Jr. [EMAIL PROTECTED] writes:
Folks, this looks wrong after consideration. If someone is familiar
with the Linux gcc optimizer, please see my last comments in
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14147
I'm starting to feel like the optimizer bit us.
Bill
Index: request.c
===
RCS file: /home/cvs/httpd-2.0/server/request.c,v
retrieving revision 1.117
retrieving revision 1.118
diff -u -r1.117 -r1.118
--- request.c 25 Oct 2002 16:38:11 - 1.117
+++ request.c 1 Nov 2002 03:27:20 - 1.118
@@ -924,6 +924,8 @@
/* That temporary trailing slash was useful, now drop it.
*/
if (temp_slash) {
+temp_slash = 0;
+AP_ASSERT(r-filename[filename_len-1] == '/');
By the way... I would like to trust this code enough to make it
AP_DEBUG_ASSERT(), but I guess if you're scared I should be scared too
:)
r-filename[--filename_len] = '\0';
--
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server request.c
Greg Stein [EMAIL PROTECTED] writes:
On Fri, Nov 01, 2002 at 03:27:20AM -, [EMAIL PROTECTED] wrote:
...
+++ request.c 1 Nov 2002 03:27:20 - 1.118
@@ -924,6 +924,8 @@
/* That temporary trailing slash was useful, now drop it.
*/
if (temp_slash) {
+temp_slash = 0;
+AP_ASSERT(r-filename[filename_len-1] == '/');
Don't you want
*temp_slash = '\0';
??
As it is, temp_slash is a boolean, not the address of the last char.
But yes it would be better to let temp_slash be NULL or the address of
the last char.
--
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server request.c
Folks, this looks wrong after consideration. If someone is familiar
with the Linux gcc optimizer, please see my last comments in
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14147
I'm starting to feel like the optimizer bit us.
Bill
At 09:27 PM 10/31/2002, [EMAIL PROTECTED] wrote:
wrowe 2002/10/31 19:27:20
Modified:server request.c
Log:
Fix a trailing slash/last filename truncation bug observed on Linux,
which affected DAV MOVE operations and even general file access.
PR 14147, 10687, 10236 [Dan Good [EMAIL PROTECTED]]
I'm accepting Jeff Trawick's suggestion of twisting the test into an
assert, since it seems very unlikely (after correctly resetting the flag)
that this will fault.
Reviewed by: Jeff Trawick, Will Rowe
Revision ChangesPath
1.118 +2 -0 httpd-2.0/server/request.c
Index: request.c
===
RCS file: /home/cvs/httpd-2.0/server/request.c,v
retrieving revision 1.117
retrieving revision 1.118
diff -u -r1.117 -r1.118
--- request.c 25 Oct 2002 16:38:11 - 1.117
+++ request.c 1 Nov 2002 03:27:20 - 1.118
@@ -924,6 +924,8 @@
/* That temporary trailing slash was useful, now drop it.
*/
if (temp_slash) {
+temp_slash = 0;
+AP_ASSERT(r-filename[filename_len-1] == '/');
r-filename[--filename_len] = '\0';
}
Re: cvs commit: httpd-2.0/server request.c
[EMAIL PROTECTED] writes:
Index: request.c
===
RCS file: /home/cvs/httpd-2.0/server/request.c,v
retrieving revision 1.115
retrieving revision 1.116
diff -u -u -r1.115 -r1.116
--- request.c 5 Sep 2002 06:59:14 - 1.115
+++ request.c 25 Oct 2002 16:27:38 - 1.116
@@ -149,6 +149,12 @@
if (!r-proxyreq r-parsed_uri.path) {
access_status = ap_unescape_url(r-parsed_uri.path);
if (access_status) {
+if (access_status == HTTP_NOT_FOUND) {
+ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+ found %2f (encoded '/') in URI
+ (decoded='%s'), returning 404
+ r-parsed_uri.path);
+}
I'm very surprised that the first % in your format string doesn't
need to be escaped.
--
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server request.c
My gut instinct? You break something here.
ap_location_walk, IIRC, sets up the default_conf. Which means the
default_conf may not be brought in - given the case you've optimized
for here. I might be mistaken, and don't have the energy to research
this early morning, but I thought I better point it out sooner.
At 10:45 PM 5/11/2002, [EMAIL PROTECTED] wrote:
brianp 02/05/11 20:45:47
Modified:server request.c
Log:
Optimization: skip cache setup in location_walk() if the vhost
config contains no Location blocks
Revision ChangesPath
1.113 +2 -2 httpd-2.0/server/request.c
Index: request.c
===
RCS file: /home/cvs/httpd-2.0/server/request.c,v
retrieving revision 1.112
retrieving revision 1.113
diff -u -r1.112 -r1.113
--- request.c 24 Apr 2002 04:20:10 - 1.112
+++ request.c 12 May 2002 03:45:47 - 1.113
@@ -1192,8 +1192,6 @@
walk_cache_t *cache;
const char *entry_uri;
-cache = prep_walk_cache(AP_NOTE_LOCATION_WALK, r);
-
/* No tricks here, there are no Locations to parse in this vhost.
* We won't destroy the cache, just in case _this_ redirect is later
* redirected again to a vhost with Location blocks to optimize.
@@ -1201,6 +1199,8 @@
if (!num_sec) {
return OK;
}
+
+cache = prep_walk_cache(AP_NOTE_LOCATION_WALK, r);
/* Location and LocationMatch differ on their behaviour w.r.t.
multiple
* slashes. Location matches multiple slashes with a single slash,
Re: cvs commit: httpd-2.0/server request.c
William A. Rowe, Jr. wrote: My gut instinct? You break something here. ap_location_walk, IIRC, sets up the default_conf. Which means the default_conf may not be brought in - given the case you've optimized for here. I might be mistaken, and don't have the energy to research this early morning, but I thought I better point it out sooner. We should be okay...the default values set by prep_walk_cache() are stored with key AP_NOTE_LOCATION_WALK, which is only ever accessed in location_walk itself. So if one call to location_walk skips the prep_walk_cache() call but the next call can't skip it, the second call will invoke prep_walk_cache() and initialize the defaults. --Brian
Re: cvs commit: httpd-2.0/server request.c
FWIW, I see two stats rather than three to fetch c:/website/file500.html (an improvement). The first is: core_translate() apr_filepath_merge() apr_stat() The second is: core_map_to_storage() apr_lstat() apr_stat() Haven't dug into the code but is there a safe way to eliminate the filepath_merge or is this something required on Windows (in which case perhaps the results can be cached). Bill - Original Message - From: William A. Rowe, Jr. [EMAIL PROTECTED] To: [EMAIL PROTECTED]; [EMAIL PROTECTED] Sent: Friday, November 09, 2001 11:30 AM Subject: Re: cvs commit: httpd-2.0/server request.c From: Greg Ames [EMAIL PROTECTED] Sent: Friday, November 09, 2001 10:15 AM [EMAIL PROTECTED] wrote: wrowe 01/11/09 00:08:43 Modified:server request.c Log: Reintroduce the 'one stat dir_walk' Bill, How confident are you that this is rock solid? i.e., should I consider bumping it into 2.0.28? It would be great if we could have the performance benefit, but IMO stability is the top priority for a beta candidate tarball. Agreed that stability is key - and this is optimization, not essential. However, I don't mind incorporating it into .28. Bill S. said he'd test it on Win32. I can test it on Linux and FreeBSD. Please, do. Then let's draw it up for a few hours on apache.org to really pound on it. It is entirely possible this introduces a hole, but I doubt it. I see about five cases that it could cause trouble... then I rewalk the code and realize that they are already protected against. Thinks like skipping the final stat, well, we already glom the final stat outside of that loop. Come to think of it, the only scenario I can picture is; user requests a symlink (the final filename component) we stat - optimize the loop gets down to that last element/symlink we optimize away the actual stat to the target file That's badness, but it's the only scenario I can picture. What if we take it a step further, and test 'is this the end?' before we go and skip that last lstat. In this case, we can skip the lstat, but we musn't skip the actual stat. Stat'ing the final target, rather than lstat'ing it, is no good, we would end up with a stat - lstat - stat sequence that is a waste. Bill
Re: cvs commit: httpd-2.0/server request.c
On Sat, Sep 29, 2001 at 09:26:47AM -0700, Justin Erenkrantz wrote: On Sat, Sep 29, 2001 at 12:01:33PM -0400, Cliff Woolley wrote: On 29 Sep 2001 [EMAIL PROTECTED] wrote: if (strncmp(rnew-filename, fdir, fdirlen) == 0 rnew-filename[fdirlen] -ap_strchr_c(rnew-filename + fdirlen, '/') == NULL) +ap_strchr_c(rnew-filename + fdirlen, '/') == NULL +strlen(r-uri) != 0) Might I suggest r-uri[0] != '\0' or some such? You don't really need the string's length, just whether it's zero or not. Constant time instead of linear is always nice. =-) Ah, yes, that'd be better. But, I have a feeling OtherBill will be reverting it, so I won't muck anymore with it. If he says it is the right thing to do, then we can change it to that... -- justin I think ap_sub_req_lookup_filename is still broken. (Or at least acting differently than it used to) If a full path is given as the first argument, a 403 is returned. This is not caught by the perl test framework since all the tests that use ap_sub_req_lookup_file use a relative path. I'll try to look into this tomorrow. -Ryan
Re: cvs commit: httpd-2.0/server request.c
On 31 Aug 2001 [EMAIL PROTECTED] wrote: Assuming that's a correct translation, which I believe to be the case (and which also seems to jive with the previous version of the test), then that first part darned well better check == 0, as opposed to != 0. strncmp returns 0 when they match. =-) And voila, All tests successful, 1 test skipped. is the result from httpd-test And of course there's a downside: the tests are only as good as what they check for. It's still possible to get into the else case (and therefore segfault later) if you just make a relative subrequest for a file in some other directory. None of the mod_include tests currently test that (I'll ask the httpd-test guys to add some tests that check this). So you still segfault if you do something like this: !--#include file=subdir/foo.shtml-- Though this now works: !--#include file=foo.shtml-- Arggh. I think I like Bill's solution of ditching this INTERNALLY GENERATED crap and just make it NULL. But my brain is too fried right now to get that working. I'm going to sleep. We'll fix this tomorrow. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
