You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2006/07/16 04:19:13 UTC
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Damnit, you really want more case-canonical flaws in httpd?
I don't give a shit if it's force-lower or force-upper. It so happens
that 90% of the time, the NT Kernel represents drive letters in upper
case. But it's neither here nor there, revert this.
Oh - btw - nice job Justin and Paul on the MinGW effort!
Bill
pquerna@apache.org wrote:
> Author: pquerna
> Date: Sat Jul 15 00:16:32 2006
> New Revision: 422157
>
> URL: http://svn.apache.org/viewvc?rev=422157&view=rev
> Log:
> apr_filepath_root: Remove the force upper-casing of the Drive letter on Win32. This was causing the test failure on testnames line 219. All evidence points to that the comment above the upper case switch is invalid, since everything passes after removing it.
>
> Modified:
> apr/apr/trunk/file_io/win32/filepath.c
>
> Modified: apr/apr/trunk/file_io/win32/filepath.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/file_io/win32/filepath.c?rev=422157&r1=422156&r2=422157&view=diff
> ==============================================================================
> --- apr/apr/trunk/file_io/win32/filepath.c (original)
> +++ apr/apr/trunk/file_io/win32/filepath.c Sat Jul 15 00:16:32 2006
> @@ -283,10 +283,6 @@
> {
> apr_status_t rv;
> /* Validate that D:\ drive exists, test must be rooted
> - * Note that posix/win32 insists a drive letter is upper case,
> - * so who are we to argue with a 'feature'.
> - * It is a safe fold, since only A-Z is legal, and has no
> - * side effects of legal mis-mapped non-us-ascii codes.
> */
> newpath = apr_palloc(p, 4);
> newpath[0] = testpath[0];
> @@ -294,7 +290,6 @@
> newpath[2] = seperator[0];
> newpath[3] = '\0';
> if (flags & APR_FILEPATH_TRUENAME) {
> - newpath[0] = apr_toupper(newpath[0]);
> rv = filepath_root_test(newpath, p);
> if (rv)
> return rv;
>
>
>
> .
>
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Paul Querna wrote:
> William A. Rowe, Jr. wrote:
>>
>>> Again, these are the testnames tests that were failing.
>>
>> Cite them.
>
> I did, in the commit message:
> "This was causing the test failure on testnames line 219."
Thanks Paul, let's fix that. And if we feel we should TRUENAME the
results of get_path, then we could attack that too.
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by Paul Querna <ch...@force-elite.com>.
William A. Rowe, Jr. wrote:
> Justin Erenkrantz wrote:
>>
>> The problem is that the APR code relies on the MSVC run-time being
>> consistent: as we have demonstrated, it's not. It can and does report
>> c:\ in several circumstances.
>
> Yes, and so what? This should be harmless... please indicate the bug
> that the VETOED code supposedly corrects?
>
> (And Mr. Committer, revert your vetoed code already.)
I will try to get to it tonight.
>> Note that all APR was doing was
>> toupper() which doesn't handle Unicode either.
>
> No need. Drive LETTERS aren't full unicode, the drive letter is ascii.
>
>> Again, these are the testnames tests that were failing.
>
> Cite them.
I did, in the commit message:
"This was causing the test failure on testnames line 219."
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Paul Querna wrote:
>
> Er. I'm sorry I didn't revert it in under 24 hours, but like, I wasn't
> just reading email and hanging out with my machine ready to revert a
> commit right after I made it. Like i said earlier, I will try to get to
> it tonight, we all have busy schedules, so don't lay down this bullshit
> that its bad etiquette to not revert a commit as quickly as you would like.
I was reading something quite different, as in, the longer it sits there
the more likely the veto is to be forgotten/overlooked. This has happened
here in the past and won't be tollerated again ;-)
A couple days - that's fine. Thank you for the ack, it's what I was looking
for. IMHO if one has cycles to debate, one has cycles to commit, so leaving
the code in-tree without expressing an interest in reverting it yourself
certainly looked deliberately belligerent.
Bill
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by Paul Querna <ch...@force-elite.com>.
William A. Rowe, Jr. wrote:
> Justin Erenkrantz wrote:
>> On 7/17/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
>>> Justin Erenkrantz wrote:
>>> >
>>> > The problem is that the APR code relies on the MSVC run-time being
>>> > consistent: as we have demonstrated, it's not. It can and does report
>>> > c:\ in several circumstances.
>>>
>>> Yes, and so what? This should be harmless... please indicate the bug
>>> that the VETOED code supposedly corrects?
>>>
>>> (And Mr. Committer, revert your vetoed code already.)
>>
>> Why the rush to revert? We're trying to understand the codebase here
>> to ensure we're making the right change rather than rushing to revert:
>> the testcases indicate a clearly false assumption in the APR runtime -
>> that drive letters are always upper-cased by the runtime.
>
> Piddle with your experiments in a sandbox. Vetoed code needs to go when
> it's vetoed. This veto is over the fact that you've CHANGED security
> related behavior, and that won't become acceptable.
>
> The fact that it hasn't been reverted shows really bad etiquite by the
> committer. Commit then review means just that; this is committed, the
> fact that it breaks canonical comparisons was observed, now that it's
> reviewed it needs to be reverted.
Er. I'm sorry I didn't revert it in under 24 hours, but like, I wasn't
just reading email and hanging out with my machine ready to revert a
commit right after I made it. Like i said earlier, I will try to get to
it tonight, we all have busy schedules, so don't lay down this bullshit
that its bad etiquette to not revert a commit as quickly as you would like.
-Paul
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Mon, Jul 17, 2006 at 11:10:01AM -0700, Justin Erenkrantz wrote:
> ++1. Something like below? -- justin
I'd test for path[1] == ':', but yep :-)
> ===================================================================
> --- filesys.c (revision 421960)
> +++ filesys.c (working copy)
> @@ -193,6 +193,9 @@
> }
> #endif
> if (!(flags & APR_FILEPATH_NATIVE)) {
> + if (path[0] != '\\') {
> + path[0] = apr_toupper(path[0]);
> + }
> for (*rootpath = path; **rootpath; ++*rootpath) {
> if (**rootpath == '\\')
> **rootpath = '/';
>
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Colm MacCarthaigh wrote:
> On Mon, Jul 17, 2006 at 01:11:24PM -0500, William A. Rowe, Jr. wrote:
>> cd c:\progra~1\Apache~1
>>
>> so apr_filepath_get returns "c:\progra~1\Apache~1"
>>
>> and the TRUENAME resolves to "c:\Program Files\Apache Software Foundation".
>>
>> Do we really want to spend the cycles in filepath_get to normalize this
>> case?
>
> The dangerous thing I'm thinking of is when people think they can use
> strncmp as a cheap form of "is this filename a child of parent-dir".
> Although thinking about it harder, it's probably safer for that to fail.
Yup. Although the canonical form says "C:\Program Files\Apache", there are
good reasons for the user to want the shorthand path "c:\progra~1\apache"
since it doesn't suffer from whitespace-in-pathname issues.
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Colm MacCarthaigh wrote:
> On Mon, Jul 17, 2006 at 01:11:24PM -0500, William A. Rowe, Jr. wrote:
>> cd c:\progra~1\Apache~1
>>
>> so apr_filepath_get returns "c:\progra~1\Apache~1"
>>
>> and the TRUENAME resolves to "c:\Program Files\Apache Software Foundation".
>>
>> Do we really want to spend the cycles in filepath_get to normalize this
>> case?
>
> The dangerous thing I'm thinking of is when people think they can use
> strncmp as a cheap form of "is this filename a child of parent-dir".
> Although thinking about it harder, it's probably safer for that to fail.
You know - a fun test might be to get the finfo of the 'canonical' name path,
and the raw current_dir path, and just compare the dev/inodes to determine
that our 'identity test' passes ;-)
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Mon, Jul 17, 2006 at 01:11:24PM -0500, William A. Rowe, Jr. wrote:
> cd c:\progra~1\Apache~1
>
> so apr_filepath_get returns "c:\progra~1\Apache~1"
>
> and the TRUENAME resolves to "c:\Program Files\Apache Software Foundation".
>
> Do we really want to spend the cycles in filepath_get to normalize this
> case?
The dangerous thing I'm thinking of is when people think they can use
strncmp as a cheap form of "is this filename a child of parent-dir".
Although thinking about it harder, it's probably safer for that to fail.
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 7/17/06, Colm MacCarthaigh <co...@stdlib.net> wrote:
> maybe there's a case for modifying apr_filepath_get on win32 here, to
> use the same canonicalised format? We're probably not the only people
> likely to perform these kind of niaive string comparisons, and we're
> supposed to know our own codebase ;-)
++1. Something like below? -- justin
Index: filesys.c
===================================================================
--- filesys.c (revision 421960)
+++ filesys.c (working copy)
@@ -193,6 +193,9 @@
}
#endif
if (!(flags & APR_FILEPATH_NATIVE)) {
+ if (path[0] != '\\') {
+ path[0] = apr_toupper(path[0]);
+ }
for (*rootpath = path; **rootpath; ++*rootpath) {
if (**rootpath == '\\')
**rootpath = '/';
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Colm MacCarthaigh wrote:
>
> maybe there's a case for modifying apr_filepath_get on win32 here, to
> use the same canonicalised format? We're probably not the only people
> likely to perform these kind of niaive string comparisons, and we're
> supposed to know our own codebase ;-)
Very true :) Actually I started pondering that. The drive letter of
course, that's trivial. The interesting bit gets into when the user
cd c:\progra~1\Apache~1
so apr_filepath_get returns "c:\progra~1\Apache~1"
and the TRUENAME resolves to "c:\Program Files\Apache Software Foundation".
Do we really want to spend the cycles in filepath_get to normalize this case?
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Mon, Jul 17, 2006 at 12:30:09PM -0500, William A. Rowe, Jr. wrote:
> Piddle with your experiments in a sandbox. Vetoed code needs to go when
> it's vetoed. This veto is over the fact that you've CHANGED security
> related behavior, and that won't become acceptable.
>
> The fact that it hasn't been reverted shows really bad etiquite by the
> committer. Commit then review means just that; this is committed, the
> fact that it breaks canonical comparisons was observed, now that it's
> reviewed it needs to be reverted.
>
> Then we can discuss what the correct fix is, in terms of solving the
> test cases.
Is there some kind of metaphysical bubble of blockage that prevents us
from discussing it before an arbitrary svn operation?
*reaches out to feel for the bubble*
*bursts it*
:-)
Anyway, lest I increase our productivity ...
maybe there's a case for modifying apr_filepath_get on win32 here, to
use the same canonicalised format? We're probably not the only people
likely to perform these kind of niaive string comparisons, and we're
supposed to know our own codebase ;-)
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Justin Erenkrantz wrote:
> On 7/17/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
>> Justin Erenkrantz wrote:
>> >
>> > The problem is that the APR code relies on the MSVC run-time being
>> > consistent: as we have demonstrated, it's not. It can and does report
>> > c:\ in several circumstances.
>>
>> Yes, and so what? This should be harmless... please indicate the bug
>> that the VETOED code supposedly corrects?
>>
>> (And Mr. Committer, revert your vetoed code already.)
>
> Why the rush to revert? We're trying to understand the codebase here
> to ensure we're making the right change rather than rushing to revert:
> the testcases indicate a clearly false assumption in the APR runtime -
> that drive letters are always upper-cased by the runtime.
Piddle with your experiments in a sandbox. Vetoed code needs to go when
it's vetoed. This veto is over the fact that you've CHANGED security
related behavior, and that won't become acceptable.
The fact that it hasn't been reverted shows really bad etiquite by the
committer. Commit then review means just that; this is committed, the
fact that it breaks canonical comparisons was observed, now that it's
reviewed it needs to be reverted.
Then we can discuss what the correct fix is, in terms of solving the
test cases.
>> > Note that all APR was doing was
>> > toupper() which doesn't handle Unicode either.
>>
>> No need. Drive LETTERS aren't full unicode, the drive letter is ascii.
>
> Then why bring up Unicode at all?
Because if you compare the entire filepath string you need to canonicalize
the full utf-8 set to a known state. The best known state is the true path.
>> > Again, these are the testnames tests that were failing.
>>
>> Cite them.
>
> As Paul said, the testnames cases fail. I believe the specific test
> case is root_from_cwd_and_back(). apr_filepath_get and
> apr_filepath_root are assumed to be identity, but they are not under
> APR and Win32. MSVC runtime reports c:\ via apr_filepath_get(), but
> APR upcases that to C:\ when TRUENAME is set.
Actually, before you run the test, why not try cd C:\Path-to-apr\ ???
You will realize you just flipped from failing under one case to failing
under another case.
> Hence, c != C. That is
> the root of the incorrect behavior and what's fixed in 422157.
Nope, not fixed. TRUENAME has a meaning and you just broke it, not
by canonicalizing to lower case (a silly half-assed solution seeing
as it breaks the other half of the cases) but by removing the promise
that TRUENAME performs canonicalization.
>> > Win32 reports c:\ and APR expects it to be C:\. So, the tests fail.
>>
>> Test(s) plural? Cite them.
>
> I think there are more assumptions in Win32 testnames that rely upon
> the identity functions working - so they have to be fixed up too. In
> short, testnames would need to be written to be drive-letter
> case-insensitive on Win32. -- justin
Sure thing. That sounds like a solution.
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 7/17/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Justin Erenkrantz wrote:
> >
> > The problem is that the APR code relies on the MSVC run-time being
> > consistent: as we have demonstrated, it's not. It can and does report
> > c:\ in several circumstances.
>
> Yes, and so what? This should be harmless... please indicate the bug
> that the VETOED code supposedly corrects?
>
> (And Mr. Committer, revert your vetoed code already.)
Why the rush to revert? We're trying to understand the codebase here
to ensure we're making the right change rather than rushing to revert:
the testcases indicate a clearly false assumption in the APR runtime -
that drive letters are always upper-cased by the runtime.
> > Note that all APR was doing was
> > toupper() which doesn't handle Unicode either.
>
> No need. Drive LETTERS aren't full unicode, the drive letter is ascii.
Then why bring up Unicode at all?
> > Again, these are the testnames tests that were failing.
>
> Cite them.
As Paul said, the testnames cases fail. I believe the specific test
case is root_from_cwd_and_back(). apr_filepath_get and
apr_filepath_root are assumed to be identity, but they are not under
APR and Win32. MSVC runtime reports c:\ via apr_filepath_get(), but
APR upcases that to C:\ when TRUENAME is set. Hence, c != C. That is
the root of the incorrect behavior and what's fixed in 422157.
> > Win32 reports c:\ and APR expects it to be C:\. So, the tests fail.
>
> Test(s) plural? Cite them.
I think there are more assumptions in Win32 testnames that rely upon
the identity functions working - so they have to be fixed up too. In
short, testnames would need to be written to be drive-letter
case-insensitive on Win32. -- justin
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Justin Erenkrantz wrote:
>
> The problem is that the APR code relies on the MSVC run-time being
> consistent: as we have demonstrated, it's not. It can and does report
> c:\ in several circumstances.
Yes, and so what? This should be harmless... please indicate the bug
that the VETOED code supposedly corrects?
(And Mr. Committer, revert your vetoed code already.)
> Note that all APR was doing was
> toupper() which doesn't handle Unicode either.
No need. Drive LETTERS aren't full unicode, the drive letter is ascii.
> Again, these are the testnames tests that were failing.
Cite them.
> Win32 reports c:\ and APR expects it to be C:\. So, the tests fail.
Test(s) plural? Cite them.
> As Paul said,
> either the tests need to be rewritten to support case-insensitivity or
> APR needs to be fixed to respect what the runtimes provide to us. At
> this point, I think APR needs to be fixed. -- justin
No, and the veto stands.
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 7/17/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Comparisons. Deny "C:/" pattern. Now, are we going to catch "c:/"?
>
> If we get TRUECASE of the pattern and the path, they must match if same
> or mismatch if different. Case insensitive test is not sufficient since
> that's a whopping 96 codepage points out of 64k in unicode.
The problem is that the APR code relies on the MSVC run-time being
consistent: as we have demonstrated, it's not. It can and does report
c:\ in several circumstances. Note that all APR was doing was
toupper() which doesn't handle Unicode either.
Again, these are the testnames tests that were failing. Win32 reports
c:\ and APR expects it to be C:\. So, the tests fail. As Paul said,
either the tests need to be rewritten to support case-insensitivity or
APR needs to be fixed to respect what the runtimes provide to us. At
this point, I think APR needs to be fixed. -- justin
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Justin Erenkrantz wrote:
> On 7/16/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
>> To clarify -again-, the case of the name is determined by the case of
>> the name in the filesystem. The case of the drive is arbitrarily chosen
>> but must be one or another.
>
> Not true, AFAICT. The MSVC run-time can and does report c:\ or C:\.
> FWIW, the MSDN documentation for GetFullPathName() never refers to
> this case-sensitivity. In fact, the MSDN docs refer to it as c:\ - so
> where does having the drive-letter be capitalized come from again? --
Comparisons. Deny "C:/" pattern. Now, are we going to catch "c:/"?
If we get TRUECASE of the pattern and the path, they must match if same
or mismatch if different. Case insensitive test is not sufficient since
that's a whopping 96 codepage points out of 64k in unicode.
So we can pick force-upper or pick force lower. Picking neither is not
an option.
Uppercase comes from ancient history in DOS roots, where the physical
device name is upper case.
Bill
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 7/16/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> To clarify -again-, the case of the name is determined by the case of
> the name in the filesystem. The case of the drive is arbitrarily chosen
> but must be one or another.
Not true, AFAICT. The MSVC run-time can and does report c:\ or C:\.
FWIW, the MSDN documentation for GetFullPathName() never refers to
this case-sensitivity. In fact, the MSDN docs refer to it as c:\ - so
where does having the drive-letter be capitalized come from again? --
justin
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
William A. Rowe, Jr. wrote:
> Paul Querna wrote:
>>
>> So, should the fix to instead change all of our other filepath
>> functions to force everything to be one way?
>
> Choose upper, choose lower. I suggest upper. The point is that when
> you canonicalize something, you take the 'true case', or you punt. Doing
> nothing isn't an option. THAT is a -1.
To clarify -again-, the case of the name is determined by the case of
the name in the filesystem. The case of the drive is arbitrarily chosen
but must be one or another.
Are you going to revert this vetoed commit, sir?
>> The other option I was playing with was changing the testnames tests,
>> to all be case insensitive, but that means changing nearly every
>> existing test case there, and on other platforms that are case
>> sensitive we would need different tests.
You might want to post what error you are seeing, since there wasn't an
apparent error, here.
Bill
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Paul Querna wrote:
> William A. Rowe, Jr. wrote:
>> Damnit, you really want more case-canonical flaws in httpd?
>
> So, is that a -1 or just a complaint?
That part was a question after my less-than-fun trawl through svn blame
on server/request.c
>> I don't give a shit if it's force-lower or force-upper. It so happens
>> that 90% of the time, the NT Kernel represents drive letters in upper
>> case. But it's neither here nor there, revert this.
>
> So, should the fix to instead change all of our other filepath functions
> to force everything to be one way?
Choose upper, choose lower. I suggest upper. The point is that when
you canonicalize something, you take the 'true case', or you punt. Doing
nothing isn't an option. THAT is a -1.
> The other option I was playing with was changing the testnames tests, to
> all be case insensitive, but that means changing nearly every existing
> test case there, and on other platforms that are case sensitive we would
> need different tests.
Not necessary. We know the canonicalization pattern. You can anticipate.
Bill
Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Posted by Paul Querna <ch...@force-elite.com>.
William A. Rowe, Jr. wrote:
> Damnit, you really want more case-canonical flaws in httpd?
So, is that a -1 or just a complaint?
> I don't give a shit if it's force-lower or force-upper. It so happens
> that 90% of the time, the NT Kernel represents drive letters in upper
> case. But it's neither here nor there, revert this.
So, should the fix to instead change all of our other filepath functions
to force everything to be one way?
The other option I was playing with was changing the testnames tests, to
all be case insensitive, but that means changing nearly every existing
test case there, and on other platforms that are case sensitive we would
need different tests.
-Paul