You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by wr...@apache.org on 2002/11/01 04:27:20 UTC
cvs commit: httpd-2.0/server request.c
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 <de...@gooddan.com>]
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 Changes Path
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 -0000 1.117
+++ request.c 1 Nov 2002 03:27:20 -0000 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
Posted by Jeff Trawick <tr...@attglobal.net>.
Greg Stein <gs...@lyra.org> writes:
> On Fri, Nov 01, 2002 at 03:27:20AM -0000, wrowe@apache.org wrote:
> >...
> > +++ request.c 1 Nov 2002 03:27:20 -0000 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 | trawick@attglobal.net
Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server request.c
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Nov 01, 2002 at 03:27:20AM -0000, wrowe@apache.org wrote:
>...
> +++ request.c 1 Nov 2002 03:27:20 -0000 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
Posted by Jeff Trawick <tr...@attglobal.net>.
"William A. Rowe, Jr." <wr...@apache.org> 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; XXXXXXXXX
}
}
} while (thisinfo.filetype == APR_DIR);
The line marked XXXXXXXX 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." <wr...@apache.org> 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 -0000 1.117
> > +++ request.c 1 Nov 2002 03:27:20 -0000 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 | trawick@attglobal.net
Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server request.c
Posted by Jeff Trawick <tr...@attglobal.net>.
"William A. Rowe, Jr." <wr...@apache.org> 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; XXXXXXXXX
}
}
} while (thisinfo.filetype == APR_DIR);
The line marked XXXXXXXX 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." <wr...@apache.org> 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 -0000 1.117
> > +++ request.c 1 Nov 2002 03:27:20 -0000 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 | trawick@attglobal.net
Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server request.c
Posted by "William A. Rowe, Jr." <wr...@apache.org>.
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, wrowe@apache.org 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 <de...@gooddan.com>]
>
> 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 Changes Path
> 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 -0000 1.117
> +++ request.c 1 Nov 2002 03:27:20 -0000 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';
> }
>
>
>
>