You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ken Parzygnat <kp...@raleigh.ibm.com> on 1998/09/25 15:59:41 UTC
[PATCH] PR 3001- Win32: DocRoot x:/
Hi gang,
This is a Win32 patch to fix PR 3001. The problem in 3001 is that you can't
use 'x:/' as the DocumentRoot. One of the main problems is in
ap_os_canonical_filename in that 'x:/' was not being treated correctly. The
'/' was being stripped, then we'd call GetFullPathName which would take 'x:'
and change it to the current path on x:, not the root of x:. Also, we'd
end up adding another slash to x:/ and leave the routine with a 'x://'.
I also hit http_core.c because core_translate takes the document root (x:/)
and concatenates it with the URI (/foo) and you get 'x://foo' which isn't
any good. I thought this could be a problem on Unix, but I don't know if
folks ever declare DocumentRoot as '/', therefore, I ifdef'd for WIN32
only.
Let me know what you think!
--- .\main\http_core.c.orig Tue Sep 22 01:12:36 1998
+++ .\main\http_core.c Fri Sep 25 13:04:06 1998
@@ -2670,8 +2670,20 @@
(r->uri + r->server->pathlen), NULL);
}
else {
- r->filename = ap_pstrcat(r->pool, conf->ap_document_root, r->uri,
- NULL);
+#ifdef WIN32
+ /*
+ * Make sure that we do not mess up the translation by adding two
+ * /'s in a row. This happens under windows when the document
+ * root is set to x:/
+ */
+ if ((conf->ap_document_root[strlen(conf->ap_document_root)-1] ==
'/')
+ && (*(r->uri) == '/'))
+ r->filename = ap_pstrcat(r->pool, conf->ap_document_root,
r->uri+1,
+ NULL);
+ else
+#endif
+ r->filename = ap_pstrcat(r->pool, conf->ap_document_root,
r->uri,
+ NULL);
}
return OK;
--- .\os\win32\util_win32.c.orig Sat Sep 05 01:12:26 1998
+++ .\os\win32\util_win32.c Fri Sep 25 12:52:04 1998
@@ -23,7 +23,18 @@
for (nSlashes = 0; s > szFile && s[-1] == '\\'; ++nSlashes, --s)
;
- n = GetFullPathName(szFile, sizeof buf, buf, &szFilePart);
+ if ((strlen(szFile) == 2) && (szFile[1] == ':')) {
+ /*
+ * If the file name is x:, do not call GetFullPathName
+ * because it will use the current path of the executable
+ */
+ strcpy(buf,szFile);
+ n = strlen(buf);
+ szFilePart = buf + n;
+ }
+ else {
+ n = GetFullPathName(szFile, sizeof buf, buf, &szFilePart);
+ }
ap_assert(n);
ap_assert(n < sizeof buf);
@@ -36,6 +47,8 @@
* is no '\' in szInFile, it must just be a file name, so it should be
* valid to use the name from GetFullPathName. Be sure to adjust the
* 's' variable so the rest of the code functions normally.
+ * Note it is possible to get here when szFile == 'x:', but that is OK
+ * because we will bail out of this routine early.
*/
if (!s) {
szFile = buf;
@@ -180,9 +193,24 @@
buf[0] = ap_tolower(buf[0]);
- ap_assert(strlen(buf)+nSlashes < sizeof buf);
- while (nSlashes--) {
- strcat(buf, "/");
+ if (nSlashes) {
+ /*
+ * If there were additional trailing slashes, add them back on.
+ * Be sure not to add more than were originally there though,
+ * this could happen in cases where the file name is 'd:/'
+ */
+ ap_assert(strlen(buf)+nSlashes < sizeof buf);
+
+ d = buf + strlen(buf);
+ d--;
+ while ((d >= buf) && (*d == '/') && nSlashes) {
+ d--;
+ nSlashes--;
+ }
+
+ while (nSlashes--) {
+ strcat(buf, "/");
+ }
}
return ap_pstrdup(pPool, buf);
@@ -233,8 +261,13 @@
return stat(buf, pStat);
}
+ /*
+ * Below removes the trailing /, however, do not remove
+ * it in the case of 'x:/' or stat will fail
+ */
n = strlen(szPath);
- if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
+ if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
+ !((n == 3) && (szPath[1] == ':')))
char buf[_MAX_PATH];
ap_assert(n < _MAX_PATH);
- - - - - - - - - - - - - - - - - -
Ken Parzygnat
email: kparz@raleigh.ibm.com
----------------------------------------------------------------------------
----
Users of the Apache webserver are hereby granted a non-exclusive,
irrevocable,
world-wide, royalty-free, non-transferable license to use, execute, prepare
derivative works of, and distribute (internally and externally, and
including
derivative works) the code accompanying this license as part of, and
integrated into the Apache webserver. This code is provided "AS IS" WITHOUT
WARRANTY OF ANY KIND EITHER EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO
THE IMPLIED WARRANTY OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
AND ANY WARRANTY OF NON-INFRINGEMENT. THE ENTIRE RISK ARISING OUT OF THE
USE
OR PERFORMANCE OF THIS CODE REMAINS WITH USERS OF THE APACHE WEBSERVER. The
owner of this code represents and warrants that it is legally entitled to
grant the above license.
RE: [PATCH] PR 3001- Win32: DocRoot x:/
Posted by Ken Parzygnat <kp...@raleigh.ibm.com>.
Here's a refresh of the patch with all of
the changes that we've been discussing:
--- .\main\http_core.c.orig Tue Sep 22 01:12:36 1998
+++ .\main\http_core.c Mon Sep 28 18:08:21 1998
@@ -2670,8 +2670,18 @@
(r->uri + r->server->pathlen), NULL);
}
else {
- r->filename = ap_pstrcat(r->pool, conf->ap_document_root, r->uri,
- NULL);
+ /*
+ * Make sure that we do not mess up the translation by adding two
+ * /'s in a row. This happens under windows when the document
+ * root ends with a /
+ */
+ if ((conf->ap_document_root[strlen(conf->ap_document_root)-1] == '/')
+ && (*(r->uri) == '/'))
+ r->filename = ap_pstrcat(r->pool, conf->ap_document_root,
r->uri+1,
+ NULL);
+ else
+ r->filename = ap_pstrcat(r->pool, conf->ap_document_root, r->uri,
+ NULL);
}
return OK;
--- .\os\win32\util_win32.c.orig Sat Sep 05 01:12:26 1998
+++ .\os\win32\util_win32.c Mon Sep 28 19:12:02 1998
@@ -23,7 +23,18 @@
for (nSlashes = 0; s > szFile && s[-1] == '\\'; ++nSlashes, --s)
;
- n = GetFullPathName(szFile, sizeof buf, buf, &szFilePart);
+ if (strlen(szFile)==2 && szFile[1]==':') {
+ /*
+ * If the file name is x:, do not call GetFullPathName
+ * because it will use the current path of the executable
+ */
+ strcpy(buf,szFile);
+ n = strlen(buf);
+ szFilePart = buf + n;
+ }
+ else {
+ n = GetFullPathName(szFile, sizeof buf, buf, &szFilePart);
+ }
ap_assert(n);
ap_assert(n < sizeof buf);
@@ -36,6 +47,8 @@
* is no '\' in szInFile, it must just be a file name, so it should be
* valid to use the name from GetFullPathName. Be sure to adjust the
* 's' variable so the rest of the code functions normally.
+ * Note it is possible to get here when szFile == 'x:', but that is OK
+ * because we will bail out of this routine early.
*/
if (!s) {
szFile = buf;
@@ -180,9 +193,21 @@
buf[0] = ap_tolower(buf[0]);
- ap_assert(strlen(buf)+nSlashes < sizeof buf);
- while (nSlashes--) {
- strcat(buf, "/");
+ if (nSlashes) {
+ /*
+ * If there were additional trailing slashes, add them back on.
+ * Be sure not to add more than were originally there though,
+ * by checking to see if sub_canonical_filename added one;
+ * this could happen in cases where the file name is 'd:/'
+ */
+ ap_assert(strlen(buf)+nSlashes < sizeof buf);
+
+ if (nSlashes && buf[strlen(buf)-1] == '/')
+ nSlashes--;
+
+ while (nSlashes--) {
+ strcat(buf, "/");
+ }
}
return ap_pstrdup(pPool, buf);
@@ -233,8 +258,13 @@
return stat(buf, pStat);
}
+ /*
+ * Below removes the trailing /, however, do not remove
+ * it in the case of 'x:/' or stat will fail
+ */
n = strlen(szPath);
- if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
+ if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
+ !(n == 3 && szPath[1] == ':')) {
char buf[_MAX_PATH];
ap_assert(n < _MAX_PATH);
- - - - - - - - - - - - - - - - - -
Ken Parzygnat
email: kparz@raleigh.ibm.com
RE: [PATCH] PR 3001- Win32: DocRoot x:/
Posted by Ken Parzygnat <kp...@raleigh.ibm.com>.
> > > Yes, but it seems to me that this is not particularly a WIN32
> issue - we
> > > shouldn't be building paths with double slashes (unless
> explicitly asked
> > > to) on _any_ platform.
> >
> > I agree. This double slash problem will occur when the value
> > for DocumentRoot ends in a slash. I wasn't totally convinced that
> > someone on a Unix system would code DocumentRoot to be '/' whereas
> > on Windows, it's not too far fetched to have a user code 'x:/' as the
> > DocumentRoot. You can't code x: as the DocumentRoot because that is
> > not a directory (as the server will tell you). Any other
> directory '/foo',
> > just omit the last slash in the .conf file, and everything is
> OK. Do you
> > think the ifdef should be removed to enable the code for all platforms?
>
> I do.
Excellent, easily enough done.
>
> > > > > > + while ((d >= buf) && (*d == '/') && nSlashes) {
> > > > > > + d--;
> > > > > > + nSlashes--;
> > > > > > + }
> > > > > > +
> > > > > > + while (nSlashes--) {
> > > > > > + strcat(buf, "/");
> > > > > > + }
> > > > >
> > > > > Not convinced about this bit. If you are trying to avoid one
> > > slash when
> > > > > the path is "d:/" why not be more explicit about it?
> > > > I'm trying to be very careful. Back a while ago, you and a bunch of
> > > > other folks were killing yourselves to fix this routine. What
> > > I don't want
> > > > to break is the cases where we come in with some path
> > > > from a CGI request that may have tons of /'s at the end of it.
> > > This seemed
> > > > like the most generic (and safest) way to handle it.
> > >
> > > Not convinced. What you are saying is that if canonicalisation adds /s
> > > to the end, we should ignore that, assuming it is a mistake.
> Which seems
> > > wrong to me.
> >
> > Not exactly. What I'm saying is if sub_canonical_filename adds a
> > trailing slash (and it will add at most one), that it may be ignored
> > based on what we did prior to the call to sub_canonical_filename
> > during the canonicalization process.
>
> If it will add at most one, why use a loop?
Good question. I'm being overly cautious in counting trailing
slashes, when there really is no reason to be. I'll change it
to a simple:
if (nSlashes && buf[strlen(buf)-1] == '/')
nSlashes--;
> >
> > Hmmm.. Looking at the code segment above, it looks like a brace got
> > dropped. It's in the original that I sent, but not in what you
> and I got
> > from new-httpd. Let me see if I can repaste the segment:
> >
> > - if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
> > + if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
> > + !((n == 3) && (szPath[1] == ':')))
> >
> > char buf[_MAX_PATH];
> >
> > ap_assert(n < _MAX_PATH);
> >
> > Hopefully the brace on the last inserted line is there.
>
> Not that I can see.
This is distrubing indeed. I posted the same segment in a test
post to another mailing list and nothing was changed. Seems
that something from the time I send the patch to
new-httpd to the time I recieve it from new-httpd
is manipulating that line by replacing the '{' with
a 'CRLF'. There also seems to be a removal of trailing
whitespace occurring on several lines.
I'll try removing any whitespace before sending:
n = strlen(szPath);
- if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
+ if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
+ !((n == 3) && (szPath[1] == ':'))) {
char buf[_MAX_PATH];
ap_assert(n < _MAX_PATH);
If that didn't work, here are a few tests, all should end with a '{'
+ !((n == 3) && (szPath[1] == ':'))) {
+ ((n == 3) && (szPath[1] == ':'))) {
+ ((n == 3) && (szPath[1] == ':')))
+ {
+
!((n == 3) && (szPath[1] == ':'))) {
!((n == 3) && (szPath[1] == ':')))
>
> > With that
> > in place, yes, I think I want "==". I don't want to strip the trailing
> > slash if we have "x:/" because I don't want to stat "x:".
>
> :-) I know you want == - the redundant brackets would, in general, cause
> me to wonder whether = was the intention (since that is the case where
> they are not redundant).
>
Ahhh! Gotcha.
- - - - - - - - - - - - - - - - - -
Ken Parzygnat
email: kparz@raleigh.ibm.com
Re: [PATCH] PR 3001- Win32: DocRoot x:/
Posted by Ben Laurie <be...@algroup.co.uk>.
Ken Parzygnat wrote:
> > Yes, but it seems to me that this is not particularly a WIN32 issue - we
> > shouldn't be building paths with double slashes (unless explicitly asked
> > to) on _any_ platform.
>
> I agree. This double slash problem will occur when the value
> for DocumentRoot ends in a slash. I wasn't totally convinced that
> someone on a Unix system would code DocumentRoot to be '/' whereas
> on Windows, it's not too far fetched to have a user code 'x:/' as the
> DocumentRoot. You can't code x: as the DocumentRoot because that is
> not a directory (as the server will tell you). Any other directory '/foo',
> just omit the last slash in the .conf file, and everything is OK. Do you
> think the ifdef should be removed to enable the code for all platforms?
I do.
> > > > > + while ((d >= buf) && (*d == '/') && nSlashes) {
> > > > > + d--;
> > > > > + nSlashes--;
> > > > > + }
> > > > > +
> > > > > + while (nSlashes--) {
> > > > > + strcat(buf, "/");
> > > > > + }
> > > >
> > > > Not convinced about this bit. If you are trying to avoid one
> > slash when
> > > > the path is "d:/" why not be more explicit about it?
> > > I'm trying to be very careful. Back a while ago, you and a bunch of
> > > other folks were killing yourselves to fix this routine. What
> > I don't want
> > > to break is the cases where we come in with some path
> > > from a CGI request that may have tons of /'s at the end of it.
> > This seemed
> > > like the most generic (and safest) way to handle it.
> >
> > Not convinced. What you are saying is that if canonicalisation adds /s
> > to the end, we should ignore that, assuming it is a mistake. Which seems
> > wrong to me.
>
> Not exactly. What I'm saying is if sub_canonical_filename adds a
> trailing slash (and it will add at most one), that it may be ignored
> based on what we did prior to the call to sub_canonical_filename
> during the canonicalization process.
If it will add at most one, why use a loop?
> > > > > - if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
> > > > > + if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
> > > > > + !((n == 3) && (szPath[1] == ':')))
> > > >
> > > > I hate redundant brackets.
> > > Sorry, I tend to add the parens to inner expressions to avoid any
> > > ambiguity in the order of operations. Helps me and the compiler :-).
> >
> > That you are helped, I can believe. I can't believe it helps the
> > compiler :-)
> >
> > What it makes me do is wonder whether you meant "=" instead of "==".
>
> Hmmm.. Looking at the code segment above, it looks like a brace got
> dropped. It's in the original that I sent, but not in what you and I got
> from new-httpd. Let me see if I can repaste the segment:
>
> - if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
> + if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
> + !((n == 3) && (szPath[1] == ':')))
>
> char buf[_MAX_PATH];
>
> ap_assert(n < _MAX_PATH);
>
> Hopefully the brace on the last inserted line is there.
Not that I can see.
> With that
> in place, yes, I think I want "==". I don't want to strip the trailing
> slash if we have "x:/" because I don't want to stat "x:".
:-) I know you want == - the redundant brackets would, in general, cause
me to wonder whether = was the intention (since that is the case where
they are not redundant).
Cheers,
Ben.
--
Ben Laurie |Phone: +44 (181) 735 0686| Apache Group member
Freelance Consultant |Fax: +44 (181) 735 0689|http://www.apache.org/
and Technical Director|Email: ben@algroup.co.uk |
A.L. Digital Ltd, |Apache-SSL author http://www.apache-ssl.org/
London, England. |"Apache: TDG" http://www.ora.com/catalog/apache/
WE'RE RECRUITING! http://www.aldigital.co.uk/
RE: [PATCH] PR 3001- Win32: DocRoot x:/
Posted by Ken Parzygnat <kp...@raleigh.ibm.com>.
> -----Original Message-----
> From: new-httpd-owner@apache.org [mailto:new-httpd-owner@apache.org]On
> Behalf Of Ben Laurie
> Sent: Friday, September 25, 1998 4:15 PM
> To: new-httpd@apache.org
> Subject: Re: [PATCH] PR 3001- Win32: DocRoot x:/
>
>
> Ken Parzygnat wrote:
> >
> > > -----Original Message-----
> > > From: new-httpd-owner@apache.org [mailto:new-httpd-owner@apache.org]On
> > > Behalf Of Ben Laurie
> > > Sent: Friday, September 25, 1998 1:44 PM
> > > To: new-httpd@apache.org
> > > Subject: Re: [PATCH] PR 3001- Win32: DocRoot x:/
> > >
> > >
> > > Ken Parzygnat wrote:
> > > >
> > > > Hi gang,
> > > >
> > > > This is a Win32 patch to fix PR 3001. The problem in 3001 is
> > > that you can't
> > > > use 'x:/' as the DocumentRoot. One of the main problems is in
> > > > ap_os_canonical_filename in that 'x:/' was not being treated
> > > correctly. The
> > > > '/' was being stripped, then we'd call GetFullPathName which
> > > would take 'x:'
> > > > and change it to the current path on x:, not the root of
> x:. Also, we'd
> > > > end up adding another slash to x:/ and leave the routine
> with a 'x://'.
> > > >
> > > > I also hit http_core.c because core_translate takes the
> > > document root (x:/)
> > > > and concatenates it with the URI (/foo) and you get 'x://foo'
> > > which isn't
> > > > any good. I thought this could be a problem on Unix, but I
> > > don't know if
> > > > folks ever declare DocumentRoot as '/', therefore, I
> ifdef'd for WIN32
> > > > only.
> > >
> > > Why is this any different to the situtation where the root is "/" or
> > > "x:/some/where/"?
> >
> > It's really not. If you step through the code path, you'll see that in
> > Win32
> > Apache is building names like x:/some/where//Welcome.html. We get lucky
> > in this case when we call ap_os_canonical_filename because it strips the
> > slashes.
> >
> > However, we get in trouble with something like "x:/". Without
> my patch, we
> > don't handle "x:/" right in that ap_os_canonical_filename will
> return the
> > "current path on x:" which is something like "x:/apache" which
> isn't what
> > we want. With the patch, but not the http_core hit,
> > ap_os_canonical_filename will return "x:/", core_translate
> creates "x://"
> > (assuming you do a GET / request) and then S_ISDIR fails because
> > of the code in get_path_info which strips trailing /'s before doing the
> > stat, and x: is not a dir. If you go in with a name like x:/some/where/
> > and the trailing slash gets stripped, no big deal.
> >
> > I thought about massaging get_path_info, but I thought the
> better thing to
> > do would be to change core_translate since it seemed that
> carrying around
> > names like "x:/some/where//Welcome.html" or "x:/some/where//" would
> > bite us again elsewhere.
> >
> > Make sense?
>
> Yes, but it seems to me that this is not particularly a WIN32 issue - we
> shouldn't be building paths with double slashes (unless explicitly asked
> to) on _any_ platform.
I agree. This double slash problem will occur when the value
for DocumentRoot ends in a slash. I wasn't totally convinced that
someone on a Unix system would code DocumentRoot to be '/' whereas
on Windows, it's not too far fetched to have a user code 'x:/' as the
DocumentRoot. You can't code x: as the DocumentRoot because that is
not a directory (as the server will tell you). Any other directory '/foo',
just omit the last slash in the .conf file, and everything is OK. Do you
think the ifdef should be removed to enable the code for all platforms?
>
> > > > + while ((d >= buf) && (*d == '/') && nSlashes) {
> > > > + d--;
> > > > + nSlashes--;
> > > > + }
> > > > +
> > > > + while (nSlashes--) {
> > > > + strcat(buf, "/");
> > > > + }
> > >
> > > Not convinced about this bit. If you are trying to avoid one
> slash when
> > > the path is "d:/" why not be more explicit about it?
> > I'm trying to be very careful. Back a while ago, you and a bunch of
> > other folks were killing yourselves to fix this routine. What
> I don't want
> > to break is the cases where we come in with some path
> > from a CGI request that may have tons of /'s at the end of it.
> This seemed
> > like the most generic (and safest) way to handle it.
>
> Not convinced. What you are saying is that if canonicalisation adds /s
> to the end, we should ignore that, assuming it is a mistake. Which seems
> wrong to me.
Not exactly. What I'm saying is if sub_canonical_filename adds a
trailing slash (and it will add at most one), that it may be ignored
based on what we did prior to the call to sub_canonical_filename
during the canonicalization process.
The problem is that we strip trailing slashes in ap_os_canonical_filename
before calling sub_canonical_filename. We do this to handle the unusual
filenames like those coming in from CGIs. This has the unfortunate side
affect of stripping trailing slashes from valid directories like "x:\" and
"\\machine\foo\". sub_canonical_filename will add these slashes back,
then ap_os_canonical_filename will try to tack another on, and we know
this is incorrect. Another course of action is to modify
sub_canonical_filename to not add the a trailing slash, but knowing
the history of that routine, and looking at the way it calls itself
(possibly
adding slashes on intermediate steps), I'm not
convinced this can be done without causing more harm than good.
I believe this code will produce the expected results because
ap_os_canonical_filename won't create an unusual name
given a correct name. That is, if the input has no trailing slashes,
one may or may not be added based on output from
sub_canonical_filename. If input has one slash, the output will
only have one which is what I think we want for a
windows pathname (i.e. not two trailing slashes).
If input has n (where n>1) trailing slashes,
the output will have n. This seems correct since n slashes already
indicates an unusual name, and the addition of another slash
is probably not expected.
>
> > > > - if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
> > > > + if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
> > > > + !((n == 3) && (szPath[1] == ':')))
> > >
> > > I hate redundant brackets.
> > Sorry, I tend to add the parens to inner expressions to avoid any
> > ambiguity in the order of operations. Helps me and the compiler :-).
>
> That you are helped, I can believe. I can't believe it helps the
> compiler :-)
>
> What it makes me do is wonder whether you meant "=" instead of "==".
Hmmm.. Looking at the code segment above, it looks like a brace got
dropped. It's in the original that I sent, but not in what you and I got
from new-httpd. Let me see if I can repaste the segment:
- if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
+ if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
+ !((n == 3) && (szPath[1] == ':')))
char buf[_MAX_PATH];
ap_assert(n < _MAX_PATH);
Hopefully the brace on the last inserted line is there. With that
in place, yes, I think I want "==". I don't want to strip the trailing
slash if we have "x:/" because I don't want to stat "x:".
Re: [PATCH] PR 3001- Win32: DocRoot x:/
Posted by Ben Laurie <be...@algroup.co.uk>.
Ken Parzygnat wrote:
>
> > -----Original Message-----
> > From: new-httpd-owner@apache.org [mailto:new-httpd-owner@apache.org]On
> > Behalf Of Ben Laurie
> > Sent: Friday, September 25, 1998 1:44 PM
> > To: new-httpd@apache.org
> > Subject: Re: [PATCH] PR 3001- Win32: DocRoot x:/
> >
> >
> > Ken Parzygnat wrote:
> > >
> > > Hi gang,
> > >
> > > This is a Win32 patch to fix PR 3001. The problem in 3001 is
> > that you can't
> > > use 'x:/' as the DocumentRoot. One of the main problems is in
> > > ap_os_canonical_filename in that 'x:/' was not being treated
> > correctly. The
> > > '/' was being stripped, then we'd call GetFullPathName which
> > would take 'x:'
> > > and change it to the current path on x:, not the root of x:. Also, we'd
> > > end up adding another slash to x:/ and leave the routine with a 'x://'.
> > >
> > > I also hit http_core.c because core_translate takes the
> > document root (x:/)
> > > and concatenates it with the URI (/foo) and you get 'x://foo'
> > which isn't
> > > any good. I thought this could be a problem on Unix, but I
> > don't know if
> > > folks ever declare DocumentRoot as '/', therefore, I ifdef'd for WIN32
> > > only.
> >
> > Why is this any different to the situtation where the root is "/" or
> > "x:/some/where/"?
>
> It's really not. If you step through the code path, you'll see that in
> Win32
> Apache is building names like x:/some/where//Welcome.html. We get lucky
> in this case when we call ap_os_canonical_filename because it strips the
> slashes.
>
> However, we get in trouble with something like "x:/". Without my patch, we
> don't handle "x:/" right in that ap_os_canonical_filename will return the
> "current path on x:" which is something like "x:/apache" which isn't what
> we want. With the patch, but not the http_core hit,
> ap_os_canonical_filename will return "x:/", core_translate creates "x://"
> (assuming you do a GET / request) and then S_ISDIR fails because
> of the code in get_path_info which strips trailing /'s before doing the
> stat, and x: is not a dir. If you go in with a name like x:/some/where/
> and the trailing slash gets stripped, no big deal.
>
> I thought about massaging get_path_info, but I thought the better thing to
> do would be to change core_translate since it seemed that carrying around
> names like "x:/some/where//Welcome.html" or "x:/some/where//" would
> bite us again elsewhere.
>
> Make sense?
Yes, but it seems to me that this is not particularly a WIN32 issue - we
shouldn't be building paths with double slashes (unless explicitly asked
to) on _any_ platform.
> > > + while ((d >= buf) && (*d == '/') && nSlashes) {
> > > + d--;
> > > + nSlashes--;
> > > + }
> > > +
> > > + while (nSlashes--) {
> > > + strcat(buf, "/");
> > > + }
> >
> > Not convinced about this bit. If you are trying to avoid one slash when
> > the path is "d:/" why not be more explicit about it?
> I'm trying to be very careful. Back a while ago, you and a bunch of
> other folks were killing yourselves to fix this routine. What I don't want
> to break is the cases where we come in with some path
> from a CGI request that may have tons of /'s at the end of it. This seemed
> like the most generic (and safest) way to handle it.
Not convinced. What you are saying is that if canonicalisation adds /s
to the end, we should ignore that, assuming it is a mistake. Which seems
wrong to me.
> > > - if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
> > > + if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
> > > + !((n == 3) && (szPath[1] == ':')))
> >
> > I hate redundant brackets.
> Sorry, I tend to add the parens to inner expressions to avoid any
> ambiguity in the order of operations. Helps me and the compiler :-).
That you are helped, I can believe. I can't believe it helps the
compiler :-)
What it makes me do is wonder whether you meant "=" instead of "==".
Cheers,
Ben.
--
Ben Laurie |Phone: +44 (181) 735 0686| Apache Group member
Freelance Consultant |Fax: +44 (181) 735 0689|http://www.apache.org/
and Technical Director|Email: ben@algroup.co.uk |
A.L. Digital Ltd, |Apache-SSL author http://www.apache-ssl.org/
London, England. |"Apache: TDG" http://www.ora.com/catalog/apache/
WE'RE RECRUITING! http://www.aldigital.co.uk/
RE: [PATCH] PR 3001- Win32: DocRoot x:/
Posted by Ken Parzygnat <kp...@raleigh.ibm.com>.
> -----Original Message-----
> From: new-httpd-owner@apache.org [mailto:new-httpd-owner@apache.org]On
> Behalf Of Ben Laurie
> Sent: Friday, September 25, 1998 1:44 PM
> To: new-httpd@apache.org
> Subject: Re: [PATCH] PR 3001- Win32: DocRoot x:/
>
>
> Ken Parzygnat wrote:
> >
> > Hi gang,
> >
> > This is a Win32 patch to fix PR 3001. The problem in 3001 is
> that you can't
> > use 'x:/' as the DocumentRoot. One of the main problems is in
> > ap_os_canonical_filename in that 'x:/' was not being treated
> correctly. The
> > '/' was being stripped, then we'd call GetFullPathName which
> would take 'x:'
> > and change it to the current path on x:, not the root of x:. Also, we'd
> > end up adding another slash to x:/ and leave the routine with a 'x://'.
> >
> > I also hit http_core.c because core_translate takes the
> document root (x:/)
> > and concatenates it with the URI (/foo) and you get 'x://foo'
> which isn't
> > any good. I thought this could be a problem on Unix, but I
> don't know if
> > folks ever declare DocumentRoot as '/', therefore, I ifdef'd for WIN32
> > only.
>
> Why is this any different to the situtation where the root is "/" or
> "x:/some/where/"?
It's really not. If you step through the code path, you'll see that in
Win32
Apache is building names like x:/some/where//Welcome.html. We get lucky
in this case when we call ap_os_canonical_filename because it strips the
slashes.
However, we get in trouble with something like "x:/". Without my patch, we
don't handle "x:/" right in that ap_os_canonical_filename will return the
"current path on x:" which is something like "x:/apache" which isn't what
we want. With the patch, but not the http_core hit,
ap_os_canonical_filename will return "x:/", core_translate creates "x://"
(assuming you do a GET / request) and then S_ISDIR fails because
of the code in get_path_info which strips trailing /'s before doing the
stat, and x: is not a dir. If you go in with a name like x:/some/where/
and the trailing slash gets stripped, no big deal.
I thought about massaging get_path_info, but I thought the better thing to
do would be to change core_translate since it seemed that carrying around
names like "x:/some/where//Welcome.html" or "x:/some/where//" would
bite us again elsewhere.
Make sense?
>
>
> > Let me know what you think!
>
> > + d = buf + strlen(buf);
> > + d--;
>
> d=buf+strlen(buf)-1;
>
> surely?
Certainly!!
>
> > + while ((d >= buf) && (*d == '/') && nSlashes) {
> > + d--;
> > + nSlashes--;
> > + }
> > +
> > + while (nSlashes--) {
> > + strcat(buf, "/");
> > + }
>
> Not convinced about this bit. If you are trying to avoid one slash when
> the path is "d:/" why not be more explicit about it?
I'm trying to be very careful. Back a while ago, you and a bunch of
other folks were killing yourselves to fix this routine. What I don't want
to break is the cases where we come in with some path
from a CGI request that may have tons of /'s at the end of it. This seemed
like the most generic (and safest) way to handle it.
>
> > - if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
> > + if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
> > + !((n == 3) && (szPath[1] == ':')))
>
> I hate redundant brackets.
Sorry, I tend to add the parens to inner expressions to avoid any
ambiguity in the order of operations. Helps me and the compiler :-).
>
> You asked :-)
I sure did! Thanks for taking time! I'll be happy to fix up the
style issues and repost the patch if it is a patch you want to use.
>
> Cheers,
>
> Ben.
>
> --
> Ben Laurie |Phone: +44 (181) 735 0686| Apache Group member
> Freelance Consultant |Fax: +44 (181) 735 0689|http://www.apache.org/
> and Technical Director|Email: ben@algroup.co.uk |
> A.L. Digital Ltd, |Apache-SSL author http://www.apache-ssl.org/
> London, England. |"Apache: TDG" http://www.ora.com/catalog/apache/
>
> WE'RE RECRUITING! http://www.aldigital.co.uk/
>
Re: [PATCH] PR 3001- Win32: DocRoot x:/
Posted by Ben Laurie <be...@algroup.co.uk>.
Ken Parzygnat wrote:
>
> Hi gang,
>
> This is a Win32 patch to fix PR 3001. The problem in 3001 is that you can't
> use 'x:/' as the DocumentRoot. One of the main problems is in
> ap_os_canonical_filename in that 'x:/' was not being treated correctly. The
> '/' was being stripped, then we'd call GetFullPathName which would take 'x:'
> and change it to the current path on x:, not the root of x:. Also, we'd
> end up adding another slash to x:/ and leave the routine with a 'x://'.
>
> I also hit http_core.c because core_translate takes the document root (x:/)
> and concatenates it with the URI (/foo) and you get 'x://foo' which isn't
> any good. I thought this could be a problem on Unix, but I don't know if
> folks ever declare DocumentRoot as '/', therefore, I ifdef'd for WIN32
> only.
Why is this any different to the situtation where the root is "/" or
"x:/some/where/"?
> Let me know what you think!
> + d = buf + strlen(buf);
> + d--;
d=buf+strlen(buf)-1;
surely?
> + while ((d >= buf) && (*d == '/') && nSlashes) {
> + d--;
> + nSlashes--;
> + }
> +
> + while (nSlashes--) {
> + strcat(buf, "/");
> + }
Not convinced about this bit. If you are trying to avoid one slash when
the path is "d:/" why not be more explicit about it?
> - if (szPath[n - 1] == '\\' || szPath[n - 1] == '/') {
> + if ((szPath[n - 1] == '\\' || szPath[n - 1] == '/') &&
> + !((n == 3) && (szPath[1] == ':')))
I hate redundant brackets.
You asked :-)
Cheers,
Ben.
--
Ben Laurie |Phone: +44 (181) 735 0686| Apache Group member
Freelance Consultant |Fax: +44 (181) 735 0689|http://www.apache.org/
and Technical Director|Email: ben@algroup.co.uk |
A.L. Digital Ltd, |Apache-SSL author http://www.apache-ssl.org/
London, England. |"Apache: TDG" http://www.ora.com/catalog/apache/
WE'RE RECRUITING! http://www.aldigital.co.uk/