At 12:42 PM 8/8/2003, [EMAIL PROTECTED] 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,
