Mailing List Archive: Apache: Dev

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c

Index | Next | Previous | View Threaded

wrowe at rowe-clan

Apr 10, 2006, 1:21 PM

Post #1 of 19 (2048 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c

-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 [at] apache 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.
>
>
>
>

nick at webthing

Apr 10, 2006, 1:48 PM

Post #2 of 19 (2001 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

On Monday 10 April 2006 20:59, rpluem [at] apache 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

wrowe at rowe-clan

Apr 10, 2006, 2:01 PM

Post #3 of 19 (1991 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

Nick Kew wrote:
> On Monday 10 April 2006 20:59, rpluem [at] apache 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

davi at haxent

Apr 10, 2006, 2:19 PM

Post #4 of 19 (2003 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

On Mon, 10 Apr 2006 16:01:29 -0500
"William A. Rowe, Jr." <wrowe [at] rowe-clan> wrote:

> Nick Kew wrote:
> > On Monday 10 April 2006 20:59, rpluem [at] apache 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

rpluem at apache

Apr 10, 2006, 2:39 PM

Post #5 of 19 (1980 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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

rpluem at apache

Apr 10, 2006, 2:43 PM

Post #6 of 19 (2003 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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

rpluem at apache

Apr 10, 2006, 2:50 PM

Post #7 of 19 (1989 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

On 04/10/2006 11:19 PM, Davi Arnaut wrote:
> On Mon, 10 Apr 2006 16:01:29 -0500
> "William A. Rowe, Jr." <wrowe [at] rowe-clan> wrote:
>
>
>>Nick Kew wrote:
>>
>>>On Monday 10 April 2006 20:59, rpluem [at] apache 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

nick at webthing

Apr 10, 2006, 3:08 PM

Post #8 of 19 (1984 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

On Monday 10 April 2006 22:01, William A. Rowe, Jr. wrote:
> Nick Kew wrote:
> > On Monday 10 April 2006 20:59, rpluem [at] apache 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

rpluem at apache

Apr 10, 2006, 3:09 PM

Post #9 of 19 (1978 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

On 04/10/2006 10:21 PM, William A. Rowe, Jr. wrote:
> -1 Veto.

Rolled back by r393088.

Regards

Rüdiger

rpluem at apache

Apr 10, 2006, 3:11 PM

Post #10 of 19 (1990 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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

davi at haxent

Apr 10, 2006, 3:34 PM

Post #11 of 19 (1984 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

On Mon, 10 Apr 2006 23:50:05 +0200
Ruediger Pluem <rpluem [at] apache> wrote:

>
>
> On 04/10/2006 11:19 PM, Davi Arnaut wrote:
> > On Mon, 10 Apr 2006 16:01:29 -0500
> > "William A. Rowe, Jr." <wrowe [at] rowe-clan> wrote:
> >
> >
> >>Nick Kew wrote:
> >>
> >>>On Monday 10 April 2006 20:59, rpluem [at] apache 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

fielding at gbiv

Apr 10, 2006, 7:00 PM

Post #12 of 19 (1980 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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

wrowe at rowe-clan

Apr 11, 2006, 9:05 AM

Post #13 of 19 (1991 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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

wrowe at rowe-clan

Apr 11, 2006, 10:29 AM

Post #14 of 19 (1986 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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

wrowe at rowe-clan

Apr 11, 2006, 10:31 AM

Post #15 of 19 (1983 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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

rpluem at apache

Apr 11, 2006, 2:05 PM

Post #16 of 19 (1977 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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

wrowe at rowe-clan

Apr 11, 2006, 2:10 PM

Post #17 of 19 (1982 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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?

nick at webthing

Apr 11, 2006, 2:55 PM

Post #18 of 19 (1983 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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

fielding at gbiv

Apr 11, 2006, 3:08 PM

Post #19 of 19 (2002 views)
Permalink

Re: svn commit: r393037 - in /httpd/httpd/trunk: CHANGES server/protocol.c [In reply to]

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

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads