apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: [PATCH] apr_filepath_merge (on Windows)
Date Wed, 17 Sep 2003 19:05:53 GMT
At 12:42 PM 8/8/2003, cmpilato@collab.net wrote:
>Subversion users have been noticing some annoying behavior in our use
>of apr_filepath_merge() that only happens on Windows platforms.  We
>call this function with the APR_FILEPATH_TRUENAME flag to get the case
>and segment delimiters all in shape.  We always pass "" for the
>basepath.  The addpath comes straight from whatever targets the user
>passes on the commandline, so it can be absolute, relative, etc.

Thanks for your patch - and your analysis!  After walking through this bug
for some time - I believe we converge on the patch, which I've committed.
Thanks for your patience - I'm only now finding a few cycles to devote
again to APR.

>The specific problem we are seeing on Windows is that if you call
>apr_filepath_merge("", "..\foo\bar") it returns "foo/bar", which is,
>of course, *not* the same path that was provided to the function.  If
>you call apr_filepath_merge(NULL, "..\foo\bar"), you get a correct
>response, but it's an absolute path -- not what we want.
>
>I spent a few hours debugging this function, and found three concerns:
>
>   1.  There was a flag test that used "&&" instead of "&" -- obvious
>       bug.
>
>   2.  There was a check for "is this path trying to go above the
>       root" which is not conditional upon whether or not a root has
>       even been calculated.

Both of these fixes were correct.  I expanded on the commentary in code.

>I fixed these two things, cross my fingers, and re-ran my tests.  This
>time, however, apr_filepath_merge("", "..\foo\bar") returned
>"baz/foo/bar", where "baz" is the name of the "foo"'s parent
>directory.  So:
>
>   3.  There was a loop which is calling apr_lstat() to find the true
>       names of each path segment, but doesn't ignore "." and "..".

It should never, only leading ../ elements may be preserved!  There should
never remain an embedded ../ or ./ element after the base parsing is done.

But what we failed to do was step past the ../../ leading segments 
and trusting them as known-correct path.  Now, to do that we bypass 
the 'keptlen' - that part of the path name that is pure from the LHS path 
expression (we *never* rescan the LHS path, on the appended RHS.)

So by simply setting keptlen every time we grow the leading ../ elements,
we protect ourselves from this problem.  This also means that only one of
the cases, cropping a segment, requires us check EABOVEROOT and
truncate the keptlen - so I've indented that code as well.

>I have a patch which I'd like someone with the k-nowledge to review.
>Also, I'm unfamiliar with the apr and httpd testing setups -- anyone
>who can assist me with self-verification of this change would be
>greatly appreciated.

I have added the appropriate tests as well.

>Log Message:
>
>* file_io/win32/filepath.c
>  (apr_filepath_merge): Fix a flag test to use '&' instead of '&&'.
>    Don't count it as "going above the root" when no root is known.
>    Skip the apr_lstat() calls on "." and ".." path segments.
>
>Index: filepath.c
>===================================================================
>RCS file: /home/cvspublic/apr/file_io/win32/filepath.c,v
>retrieving revision 1.41
>diff -u -r1.41 filepath.c
>--- filepath.c  16 Feb 2003 21:59:08 -0000      1.41
>+++ filepath.c  8 Aug 2003 17:23:04 -0000
>@@ -704,7 +704,7 @@
> #endif
> 
>             /* backpath (../) */
>-            if (pathlen <= rootlen) 
>+            if (rootlen && (pathlen <= rootlen))
>             {
>                 /* Attempt to move above root.  Always die if the 
>                  * APR_FILEPATH_SECUREROOTTEST flag is specified.
>@@ -733,7 +733,7 @@
>                  */
>                 if (pathlen + 3 >= sizeof(path))
>                     return APR_ENAMETOOLONG;
>-                memcpy(path + pathlen, ((flags && APR_FILEPATH_NATIVE) 
>+                memcpy(path + pathlen, ((flags & APR_FILEPATH_NATIVE) 
>                                           ? "..\\" : "../"), 3);
>                 pathlen += 3;
>             }
>@@ -869,6 +869,24 @@
>                     break;
>                 }
>             }
>+
>+            /* Skip over the '.' and '..' segments. */
>+            if ((seglen == 1) 
>+                    && (path[keptlen + seglen - 1] == '.')) {
>+                keptlen += seglen;
>+                if (saveslash)
>+                    keptlen++;
>+                continue;
>+            }
>+            if ((seglen == 2) 
>+                    && (path[keptlen + seglen - 1] == '.')
>+                    && (path[keptlen + seglen - 2] == '.')) {
>+                keptlen += seglen;
>+                if (saveslash)
>+                    keptlen++;
>+                continue;
>+            }
>+
>             /* Null term for stat! */
>             path[keptlen + seglen] = '\0';
>             if ((rv = apr_lstat(&finfo, path, 



Mime
View raw message