httpd-cvs mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@attglobal.net>
Subject Re: cvs commit: httpd-2.0/server request.c
Date Fri, 01 Nov 2002 11:50:06 GMT
"William A. Rowe, Jr." <wrowe@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." <wrowe@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...

Mime
View raw message