You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by rp...@apache.org on 2006/04/10 21:59:35 UTC
svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Author: rpluem
Date: Mon Apr 10 12:59:33 2006
New Revision: 393037
URL: http://svn.apache.org/viewcvs?rev=393037&view=rev
Log:
* Prevent r->parsed_uri.path from being NULL as this can cause segmentation
faults e.g. in mod_cache. Set it to "/" in this case.
PR: 39259
Submitted by: Davi Arnaut <davi haxent.com.br>
Reviewed by: rpluem
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/server/protocol.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/CHANGES?rev=393037&r1=393036&r2=393037&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Mon Apr 10 12:59:33 2006
@@ -2,6 +2,9 @@
Changes with Apache 2.3.0
[Remove entries to the current 2.0 and 2.2 section below, when backported]
+ *) core: Set the path of a parsed uri in a request to "/" if the path is
+ NULL. PR 39259. [Davi Arnaut <davi haxent.com.br>]
+
*) htdbm: Warn the user when adding a plaintext password on a platform
where it wouldn't work with the server (i.e., anywhere that has
crypt()). [Jeff Trawick]
Modified: httpd/httpd/trunk/server/protocol.c
URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/server/protocol.c?rev=393037&r1=393036&r2=393037&view=diff
==============================================================================
--- httpd/httpd/trunk/server/protocol.c (original)
+++ httpd/httpd/trunk/server/protocol.c Mon Apr 10 12:59:33 2006
@@ -518,9 +518,11 @@
r->hostname = r->parsed_uri.hostname;
}
+ if (r->parsed_uri.path == NULL)
+ r->parsed_uri.path = apr_pstrdup(r->pool, "/");
+
r->args = r->parsed_uri.query;
- r->uri = r->parsed_uri.path ? r->parsed_uri.path
- : apr_pstrdup(r->pool, "/");
+ r->uri = r->parsed_uri.path;
#if defined(OS2) || defined(WIN32)
/* Handle path translations for OS/2 and plug security hole.
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/11/2006 12:09 AM, Ruediger Pluem wrote:
>
> On 04/10/2006 10:21 PM, William A. Rowe, Jr. wrote:
>
>>-1 Veto.
>
>
> Rolled back by r393088.
Of course it must be r393087 :-).
Regards
Rüdiger
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/10/2006 10:21 PM, William A. Rowe, Jr. wrote:
> -1 Veto.
Rolled back by r393088.
Regards
Rüdiger
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/10/2006 11:39 PM, Ruediger Pluem wrote:
>
>
>
> No arguing about your veto. I just try to understand it better to address it.
>
> So some questions:
>
> 1. As I just learned from RFC3986 an empty path is allowed (I was not aware of this
> before). But why do we set r->uri to "/" in this case? Shouldn't it be NULL too
> in this case?
Ok. Forget about this. This was just answered.
Regards
Rüdiger
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/10/2006 10:21 PM, William A. Rowe, Jr. wrote:
> -1 Veto.
>
> You aren't checking the scheme (other than that we've dumped CONNECT
> earlier
> in the code) so you don't know that this transform is appropriate. If you
> would like to test that scheme applies, you still have the question of
> unusual
> methods (eg non-GET/non-POST)
No arguing about your veto. I just try to understand it better to address it.
So some questions:
1. As I just learned from RFC3986 an empty path is allowed (I was not aware of this
before). But why do we set r->uri to "/" in this case? Shouldn't it be NULL too
in this case?
2. Do you know any http method where this transformation is wrong or even better
for which ones this is correct?
3. Provided the list from 2. is present. Would it address your veto to do this
transformation only if r->method_number is one of these methods?
Regards
Rüdiger
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
-1 Veto.
You aren't checking the scheme (other than that we've dumped CONNECT earlier
in the code) so you don't know that this transform is appropriate. If you
would like to test that scheme applies, you still have the question of unusual
methods (eg non-GET/non-POST)
rpluem@apache.org wrote:
> Author: rpluem
> Date: Mon Apr 10 12:59:33 2006
> New Revision: 393037
>
> URL: http://svn.apache.org/viewcvs?rev=393037&view=rev
> Log:
> * Prevent r->parsed_uri.path from being NULL as this can cause segmentation
> faults e.g. in mod_cache. Set it to "/" in this case.
>
> PR: 39259
> Submitted by: Davi Arnaut <davi haxent.com.br>
> Reviewed by: rpluem
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/server/protocol.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/CHANGES?rev=393037&r1=393036&r2=393037&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Mon Apr 10 12:59:33 2006
> @@ -2,6 +2,9 @@
> Changes with Apache 2.3.0
> [Remove entries to the current 2.0 and 2.2 section below, when backported]
>
> + *) core: Set the path of a parsed uri in a request to "/" if the path is
> + NULL. PR 39259. [Davi Arnaut <davi haxent.com.br>]
> +
> *) htdbm: Warn the user when adding a plaintext password on a platform
> where it wouldn't work with the server (i.e., anywhere that has
> crypt()). [Jeff Trawick]
>
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/server/protocol.c?rev=393037&r1=393036&r2=393037&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Mon Apr 10 12:59:33 2006
> @@ -518,9 +518,11 @@
> r->hostname = r->parsed_uri.hostname;
> }
>
> + if (r->parsed_uri.path == NULL)
> + r->parsed_uri.path = apr_pstrdup(r->pool, "/");
> +
> r->args = r->parsed_uri.query;
> - r->uri = r->parsed_uri.path ? r->parsed_uri.path
> - : apr_pstrdup(r->pool, "/");
> + r->uri = r->parsed_uri.path;
>
> #if defined(OS2) || defined(WIN32)
> /* Handle path translations for OS/2 and plug security hole.
>
>
>
>
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Davi Arnaut wrote:
>
> It is legal. And Nick is right, that should be fixed in apr_uri. I will
> send a patch tomorrow.
Once again, you describe the http: scheme. Let's be careful about our
assumptions, and ensure we don't further constrain apr_uri to the http
behavior :) I'm all for apr_uri 'doing the right thing'.
We may *also* want to fix (on 2.2) the ap_fn - because we do *not* insist
on any particular apr (other than apr 1.2.0 and later). But fixing in apr
would ensure we need not leave such a patch on trunk/, because we can then
insist on apr 1.3 and later for httpd 2.3/2.4.
Bill
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES
server/protocol.c
Posted by Davi Arnaut <da...@haxent.com.br>.
On Mon, 10 Apr 2006 23:50:05 +0200
Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 04/10/2006 11:19 PM, Davi Arnaut wrote:
> > On Mon, 10 Apr 2006 16:01:29 -0500
> > "William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
> >
> >
> >>Nick Kew wrote:
> >>
> >>>On Monday 10 April 2006 20:59, rpluem@apache.org wrote:
> >>>
> >>>
> >>>
> >>>>* Prevent r->parsed_uri.path from being NULL as this can cause segmentation
> >>>> faults e.g. in mod_cache. Set it to "/" in this case.
> >>>
> >>>
> >>>A better fix to that would surely be for apr_uri to guarantee
> >>>setting path to non-null on parsing a URI. That way it gets set
> >>>exactly when a URI is parsed.
> >>
> >>+1. However, the exact scenario is
>
> I also thought initially to fix this in apr-util, but right know I am not
> sure about it, because IMHO apr_uri_parse should do generic uri parsing.
> Setting an empty uri to "/" seems to be HTTP specific, so I am not sure
> if we should do this in apr_uri_parse. At least we would need to check
> whether the scheme is http or https.
>
> Regards
>
> Rüdiger
>
>
After reading RFC 2396 (Uniform Resource Identifiers), well I think it's
scheme dependent (<scheme-specific-part>), but I'm not 100% sure:
3. URI Syntactic Components
The URI syntax is dependent upon the scheme. In general, absolute
URI are written as follows:
<scheme>:<scheme-specific-part>
An absolute URI contains the name of the scheme being used (<scheme>)
followed by a colon (":") and then a string (the <scheme-specific-
part>) whose interpretation depends on the scheme.
...
<scheme>://<authority><path>?<query>
each of which, except <scheme>, may be absent from a particular URI.
For example, some URI schemes do not allow an <authority> component,
and others do not use a <query> component.
...
--
Davi Arnaut
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On Apr 11, 2006, at 2:55 PM, Nick Kew wrote:
> On Tuesday 11 April 2006 22:10, William A. Rowe, Jr. wrote:
>> Ruediger Pluem wrote:
>>> On 04/11/2006 04:00 AM, Roy T. Fielding wrote:
>>>> It probably needs to be updated for RFC 3986 anyway. The path
>>>> should
>>>> be set to "", not NULL. The HTTP server should take care of the
>>>> redirect from "" to "/", which in this case means the http-proxy
>>>> needs to check for "" when it sends a request and respond with a
>>>> redirect that adds the "/".
>
> Um, it's not really a redirect; it's just a normalisation. Shouldn't
> really invoke any redirect logic, whether internal or external.
The server should redirect any time the characters in the request URI
are changed, since that impacts the digests used in various access
control mechanisms.
....Roy
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Nick Kew <ni...@webthing.com>.
On Tuesday 11 April 2006 22:10, William A. Rowe, Jr. wrote:
> Ruediger Pluem wrote:
> > On 04/11/2006 04:00 AM, Roy T. Fielding wrote:
> >>It probably needs to be updated for RFC 3986 anyway. The path should
> >>be set to "", not NULL. The HTTP server should take care of the
> >>redirect from "" to "/", which in this case means the http-proxy
> >>needs to check for "" when it sends a request and respond with a
> >>redirect that adds the "/".
Um, it's not really a redirect; it's just a normalisation. Shouldn't
really invoke any redirect logic, whether internal or external.
> > In general I agree, but I think the transformation from "" to "/"
> > must happen very early to avoid disturbing the cache code in the quick
> > handler. So I guess this could be only done in the post_read_request hook
> > of the proxy, but I don't think that we can trigger a redirect from this
> > hook. Provided my thoughts are not wrong, anybody an idea how to solve
> > this?
Just adjust the URL itself in that hook?
> If I read this correctly, http://foo.example.com is wrong, whereas
> http://foo.example.com/ is correct.
No. Both forms are correct, and equivalent (by definition).
> Does it merit an external redirect,
> much as http://foo.example.com/dirname would externally redirect the
> user to http://foo.example.com/dirname/
That's different, because the two are of course distinct in URL-space.
--
Nick Kew
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Ruediger Pluem wrote:
>
> On 04/11/2006 04:00 AM, Roy T. Fielding wrote:
>>
>>It probably needs to be updated for RFC 3986 anyway. The path should
>>be set to "", not NULL. The HTTP server should take care of the
>>redirect from "" to "/", which in this case means the http-proxy
>>needs to check for "" when it sends a request and respond with a
>>redirect that adds the "/".
>
> In general I agree, but I think the transformation from "" to "/"
> must happen very early to avoid disturbing the cache code in the quick
> handler. So I guess this could be only done in the post_read_request hook
> of the proxy, but I don't think that we can trigger a redirect from this hook.
> Provided my thoughts are not wrong, anybody an idea how to solve this?
If I read this correctly, http://foo.example.com is wrong, whereas
http://foo.example.com/ is correct. Does it merit an external redirect,
much as http://foo.example.com/dirname would externally redirect the
user to http://foo.example.com/dirname/
Do I understand this correctly? If so, isn't this entire conversation
focusing on the wrong section of code?
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/11/2006 04:00 AM, Roy T. Fielding wrote:
>
>
> It probably needs to be updated for RFC 3986 anyway. The path should
> be set to "", not NULL. The HTTP server should take care of the
> redirect from "" to "/", which in this case means the http-proxy
> needs to check for "" when it sends a request and respond with a
> redirect that adds the "/".
In general I agree, but I think the transformation from "" to "/"
must happen very early to avoid disturbing the cache code in the quick
handler. So I guess this could be only done in the post_read_request hook
of the proxy, but I don't think that we can trigger a redirect from this hook.
Provided my thoughts are not wrong, anybody an idea how to solve this?
Regards
Rüdiger
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Roy T. Fielding wrote:
> On Apr 10, 2006, at 2:50 PM, Ruediger Pluem wrote:
>
>> I also thought initially to fix this in apr-util, but right know I am
>> not
>> sure about it, because IMHO apr_uri_parse should do generic uri parsing.
>> Setting an empty uri to "/" seems to be HTTP specific, so I am not sure
>> if we should do this in apr_uri_parse. At least we would need to check
>> whether the scheme is http or https.
>
> It probably needs to be updated for RFC 3986 anyway. The path should
> be set to "", not NULL. The HTTP server should take care of the
> redirect from "" to "/", which in this case means the http-proxy
> needs to check for "" when it sends a request and respond with a
> redirect that adds the "/".
+1
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On Apr 10, 2006, at 2:50 PM, Ruediger Pluem wrote:
> I also thought initially to fix this in apr-util, but right know I
> am not
> sure about it, because IMHO apr_uri_parse should do generic uri
> parsing.
> Setting an empty uri to "/" seems to be HTTP specific, so I am not
> sure
> if we should do this in apr_uri_parse. At least we would need to check
> whether the scheme is http or https.
It probably needs to be updated for RFC 3986 anyway. The path should
be set to "", not NULL. The HTTP server should take care of the
redirect from "" to "/", which in this case means the http-proxy
needs to check for "" when it sends a request and respond with a
redirect that adds the "/".
....Roy
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/10/2006 11:19 PM, Davi Arnaut wrote:
> On Mon, 10 Apr 2006 16:01:29 -0500
> "William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
>
>
>>Nick Kew wrote:
>>
>>>On Monday 10 April 2006 20:59, rpluem@apache.org wrote:
>>>
>>>
>>>
>>>>* Prevent r->parsed_uri.path from being NULL as this can cause segmentation
>>>> faults e.g. in mod_cache. Set it to "/" in this case.
>>>
>>>
>>>A better fix to that would surely be for apr_uri to guarantee
>>>setting path to non-null on parsing a URI. That way it gets set
>>>exactly when a URI is parsed.
>>
>>+1. However, the exact scenario is
I also thought initially to fix this in apr-util, but right know I am not
sure about it, because IMHO apr_uri_parse should do generic uri parsing.
Setting an empty uri to "/" seems to be HTTP specific, so I am not sure
if we should do this in apr_uri_parse. At least we would need to check
whether the scheme is http or https.
Regards
Rüdiger
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES
server/protocol.c
Posted by Davi Arnaut <da...@haxent.com.br>.
On Mon, 10 Apr 2006 16:01:29 -0500
"William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
> Nick Kew wrote:
> > On Monday 10 April 2006 20:59, rpluem@apache.org wrote:
> >
> >
> >>* Prevent r->parsed_uri.path from being NULL as this can cause segmentation
> >> faults e.g. in mod_cache. Set it to "/" in this case.
> >
> >
> > A better fix to that would surely be for apr_uri to guarantee
> > setting path to non-null on parsing a URI. That way it gets set
> > exactly when a URI is parsed.
>
> +1. However, the exact scenario is
>
> GET http://somehost.example.com HTTP/1.1
>
> is this even legal? If so, what does 2616 have to say about it? If it says
> to promote this to a http://somehost.example.com (based on scheme http/https)
> then it doesn't hurt to do so. This gets around the issue of caching
> three copies, one of http://somehost.example.com, http://somehost.example.com/
> and http://somehost.example.com/index.html
>
> Bill
>
3.2.2 http URL
The "http" scheme is used to locate network resources via the HTTP
protocol. This section defines the scheme-specific syntax and
semantics for http URLs.
http_URL = "http:" "//" host [ ":" port ] [ abs_path [ "?" query ]]
...
If the abs_path is not present in the URL, it MUST be given as "/" when
used as a Request-URI for a resource (section 5.1.2).
...
3.2.3 URI Comparison
..
- An empty abs_path is equivalent to an abs_path of "/".
..
It is legal. And Nick is right, that should be fixed in apr_uri. I will
send a patch tomorrow.
Davi Arnaut
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Nick Kew wrote:
> Yes, it's legal. The leading slash is implied. Note: that's leading
> slash, which is firmly different to a trailing slash in a path.
for abspath you are correct. but in general;
URI = scheme ":" hier-part [ "?" query ] [ "#" fragment ]
hier-part = "//" authority path-abempty
/ path-absolute
/ path-rootless
/ path-empty
injecting a leading slash where an empty (rootless) path is implied will
change the meaning on the fly :(
Bill
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Nick Kew <ni...@webthing.com>.
On Monday 10 April 2006 22:01, William A. Rowe, Jr. wrote:
> Nick Kew wrote:
> > On Monday 10 April 2006 20:59, rpluem@apache.org wrote:
> >>* Prevent r->parsed_uri.path from being NULL as this can cause
> >> segmentation faults e.g. in mod_cache. Set it to "/" in this case.
> >
> > A better fix to that would surely be for apr_uri to guarantee
> > setting path to non-null on parsing a URI. That way it gets set
> > exactly when a URI is parsed.
>
> +1. However, the exact scenario is
>
> GET http://somehost.example.com HTTP/1.1
>
> is this even legal? If so, what does 2616 have to say about it?
Yes, it's legal. The leading slash is implied. Note: that's leading
slash, which is firmly different to a trailing slash in a path.
--
Nick Kew
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Nick Kew wrote:
> On Monday 10 April 2006 20:59, rpluem@apache.org wrote:
>
>
>>* Prevent r->parsed_uri.path from being NULL as this can cause segmentation
>> faults e.g. in mod_cache. Set it to "/" in this case.
>
>
> A better fix to that would surely be for apr_uri to guarantee
> setting path to non-null on parsing a URI. That way it gets set
> exactly when a URI is parsed.
+1. However, the exact scenario is
GET http://somehost.example.com HTTP/1.1
is this even legal? If so, what does 2616 have to say about it? If it says
to promote this to a http://somehost.example.com (based on scheme http/https)
then it doesn't hurt to do so. This gets around the issue of caching
three copies, one of http://somehost.example.com, http://somehost.example.com/
and http://somehost.example.com/index.html
Bill
Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Nick Kew <ni...@webthing.com>.
On Monday 10 April 2006 20:59, rpluem@apache.org wrote:
> * Prevent r->parsed_uri.path from being NULL as this can cause segmentation
> faults e.g. in mod_cache. Set it to "/" in this case.
A better fix to that would surely be for apr_uri to guarantee
setting path to non-null on parsing a URI. That way it gets set
exactly when a URI is parsed.
--
Nick Kew