You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Lieven Govaerts <lg...@mobsol.be> on 2006/07/23 12:28:11 UTC
[PATCH]: apr_filepath_merge on Windows handes ..\..\.. incorrectly. (is Subversion issue 1869)
Hi,
attached is a patch for an issue reported in the Subversion issue tracker as
issue 1869:
http://subversion.tigris.org/issues/show_bug.cgi?id=1869
The issue is that merging two paths "" and "..\..\.." with the
apr_filepath_merge functions returns a result of "..\" where it obviously
should have returned "..\..\..".
Attached patch will solve that issue. I included a new unit test to test for
this issue.
When replying, please include my email address in CC as I'm not subscribed
to this list.
kind regards,
Lieven.
Re: [PATCH]: apr_filepath_merge on Windows handes ..\..\.. incorrectly.
(is Subversion issue 1869)
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Lieven Govaerts wrote:
> --- file_io/win32/filepath.c (revision 422522)
> +++ file_io/win32/filepath.c (working copy)
> @@ -673,7 +673,7 @@
> */
> }
> else if (pathlen == 0 ||
> - (pathlen >= 3 && (pathlen == 3
> + (pathlen >= 3 && (pathlen % 3 == 0
> || path[pathlen - 4] == ':')
> && path[pathlen - 3] == '.'
> && path[pathlen - 2] == '.'
-1 on this specific solution; you are entrusting that path[]'s triplets are
all ../ patterns which definitely isn't a robust test.
There's something deeper going on here, +1 on your test, I'll get that applied
in the next day if nobody beats me to it, so we better ensure this is properly
fixed before release.
This is a great catch - thank you for your report!
Bill
Re: [PATCH]: apr_filepath_merge on Windows handes ..\..\.. incorrectly.
(is Subversion issue 1869)
Posted by Lieven Govaerts <lg...@mobsol.be>.
William A. Rowe, Jr. wrote:
> Anyways, the correct fix appears to be; FIRST evaluate that prior segment
> begins with / or \, as well as begins with : or looking for characters.
> Patch committed, see;
>
> http://svn.apache.org/repos/asf/apr/apr/trunk/file_io/win32/filepath.c
>
> commit 424915.
Thanks Bill for the thorough review and solution!
> As a future hint for better svn integration, please feel free to add any
> incident to our http://issues.apache.org/bugzilla, and fill in the URI with
> the link to the SVN incident. Visa versa - tag the svn incident with the
> bugzilla incident URI. I think it would promote better problem ticket
> handling.
Yup, I agree. Next time I'll use your issue tracker as well.
regards,
Lieven.
Re: [PATCH]: apr_filepath_merge on Windows handes ..\..\.. incorrectly.
(is Subversion issue 1869)
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Lieven Govaerts wrote:
> Hi,
>
> attached is a patch for an issue reported in the Subversion issue tracker as
> issue 1869: http://subversion.tigris.org/issues/show_bug.cgi?id=1869
>
> The issue is that merging two paths "" and "..\..\.." with the
> apr_filepath_merge functions returns a result of "..\" where it obviously
> should have returned "..\..\..".
I have applied your test patch, and confirmed that both ../../.. and ../../../
will fail in the manner you documented.
> Attached patch will solve that issue. I included a new unit test to test for
> this issue.
Your patch would blow chunks on ../ab/../cd/efg - which was why the -1.
You were -probably- looking at a known-buggy version of apr; see CHANGES for
the fixes, see the current code for the current behavior (the fix was applied
for the most recent releases of 0.9.x and 1.2.x). The bug was introduced in
commit 239927 and causes apr bug 38801. Another recently fixed bug includes
31878, a quirky edge case is //mach/share normalization.
Another recent bug was introduced in 422157 and then promptly reverted in 422924
(you can ignore both commits). We are debating if the root_from_cwd_and_back
tests should ignore case, ignore the drive letter case, or if apr_filepath_root
should return a TRUENAME.
Anyways, the correct fix appears to be; FIRST evaluate that prior segment
begins with / or \, as well as begins with : or looking for characters.
Patch committed, see;
http://svn.apache.org/repos/asf/apr/apr/trunk/file_io/win32/filepath.c
commit 424915.
As a future hint for better svn integration, please feel free to add any
incident to our http://issues.apache.org/bugzilla, and fill in the URI with
the link to the SVN incident. Visa versa - tag the svn incident with the
bugzilla incident URI. I think it would promote better problem ticket handling.
Bill
> Index: file_io/win32/filepath.c
> ===================================================================
> --- file_io/win32/filepath.c (revision 422522)
> +++ file_io/win32/filepath.c (working copy)
> @@ -673,7 +673,7 @@
> */
> }
> else if (pathlen == 0 ||
> - (pathlen >= 3 && (pathlen == 3
> + (pathlen >= 3 && (pathlen % 3 == 0
> || path[pathlen - 4] == ':')
> && path[pathlen - 3] == '.'
> && path[pathlen - 2] == '.'