You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ka...@apache.org on 2010/02/25 14:40:22 UTC
svn commit: r916286 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h
mirror.c mod_dav_svn.c
Author: kameshj
Date: Thu Feb 25 13:40:22 2010
New Revision: 916286
URL: http://svn.apache.org/viewvc?rev=916286&view=rev
Log:
With the below apache configuration(See the trailing slash at the end of '/svn/').
<Location /svn/>
DAV svn
SVNParentPath /repositories
#See the trailing slash on the master URI also can cause the confusion.
SVNMasterURI http://master/svn/
SVNAdvertiseV2Protocol Off
</Location>
We get the following error on the client side.
svn: Commit failed (details follow):
svn: MKACTIVITY of '/svn/demujin/!svn/act/4b6d547c-018d-4e02-9d3f-2b283076cc06': Could not read status line: connection was closed by server (http://localhost)
On the server(proxy) it is an assertion error on the following code block from subversion/mod_dav_svn/mirror.c:proxy_request_fixup
assert((uri_segment[0] == '\0')
|| (uri_segment[0] == '/'));
For the above configuration we get the uri_segment with the value 'reponame/some/path/inside/the/repo'.
We fix this by canonicalizing the 'root_dir'(The one in Location) and
'uri.path'(Path portion of Master URI).
* subversion/mod_dav_svn/dav_svn.h
(dav_svn__get_root_dir): Document that root_dir is in canonicalized form.
* subversion/mod_dav_svn/mod_dav_svn.c
(create_dir_config): Canonicalize the root_dir.
* subversion/mod_dav_svn/mirror.c
(dav_svn__location_in_filter, dav_svn__location_body_filter):
As root_dir is in canonical form canonicalize the uri.path too to avoid
spurious errors.
(dav_svn__location_header_filter): As root_dir is canonical we need to
explicitly introduce the path seperator.
Suggested by: julianfoad
Modified:
subversion/trunk/subversion/mod_dav_svn/dav_svn.h
subversion/trunk/subversion/mod_dav_svn/mirror.c
subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Modified: subversion/trunk/subversion/mod_dav_svn/dav_svn.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/dav_svn.h?rev=916286&r1=916285&r2=916286&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/dav_svn.h (original)
+++ subversion/trunk/subversion/mod_dav_svn/dav_svn.h Thu Feb 25 13:40:22 2010
@@ -352,7 +352,7 @@
/* Return the activities db */
const char * dav_svn__get_activities_db(request_rec *r);
-/* Return the root directory */
+/* Return the root directory in canonicalized form */
const char * dav_svn__get_root_dir(request_rec *r);
Modified: subversion/trunk/subversion/mod_dav_svn/mirror.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mirror.c?rev=916286&r1=916285&r2=916286&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/mirror.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/mirror.c Thu Feb 25 13:40:22 2010
@@ -128,7 +128,7 @@
locate_ctx_t *ctx = f->ctx;
apr_status_t rv;
apr_bucket *bkt;
- const char *master_uri, *root_dir;
+ const char *master_uri, *root_dir, *canonicalized_uri;
apr_uri_t uri;
/* Don't filter if we're in a subrequest or we aren't setup to
@@ -143,7 +143,11 @@
(that is, if our root path matches that of the master server). */
apr_uri_parse(r->pool, master_uri, &uri);
root_dir = dav_svn__get_root_dir(r);
- if (strcmp(uri.path, root_dir) == 0) {
+ if (uri.path)
+ canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
+ else
+ canonicalized_uri = uri.path;
+ if (strcmp(canonicalized_uri, root_dir) == 0) {
ap_remove_input_filter(f);
return ap_get_brigade(f->next, bb, mode, block, readbytes);
}
@@ -156,7 +160,7 @@
if (!f->ctx) {
ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
- ctx->remotepath = uri.path;
+ ctx->remotepath = canonicalized_uri;
ctx->remotepath_len = strlen(ctx->remotepath);
ctx->localpath = root_dir;
ctx->localpath_len = strlen(ctx->localpath);
@@ -226,7 +230,7 @@
start_foo += strlen(master_uri);
new_uri = ap_construct_url(r->pool,
apr_pstrcat(r->pool,
- dav_svn__get_root_dir(r),
+ dav_svn__get_root_dir(r), "/",
start_foo, NULL),
r);
apr_table_set(r->headers_out, "Location", new_uri);
@@ -240,7 +244,7 @@
request_rec *r = f->r;
locate_ctx_t *ctx = f->ctx;
apr_bucket *bkt;
- const char *master_uri, *root_dir;
+ const char *master_uri, *root_dir, *canonicalized_uri;
apr_uri_t uri;
/* Don't filter if we're in a subrequest or we aren't setup to
@@ -255,7 +259,11 @@
(that is, if our root path matches that of the master server). */
apr_uri_parse(r->pool, master_uri, &uri);
root_dir = dav_svn__get_root_dir(r);
- if (strcmp(uri.path, root_dir) == 0) {
+ if (uri.path)
+ canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
+ else
+ canonicalized_uri = uri.path;
+ if (strcmp(canonicalized_uri, root_dir) == 0) {
ap_remove_output_filter(f);
return ap_pass_brigade(f->next, bb);
}
@@ -268,7 +276,7 @@
if (!f->ctx) {
ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
- ctx->remotepath = uri.path;
+ ctx->remotepath = canonicalized_uri;
ctx->remotepath_len = strlen(ctx->remotepath);
ctx->localpath = root_dir;
ctx->localpath_len = strlen(ctx->localpath);
Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=916286&r1=916285&r2=916286&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Thu Feb 25 13:40:22 2010
@@ -169,7 +169,8 @@
/* NOTE: dir==NULL creates the default per-dir config */
dir_conf_t *conf = apr_pcalloc(p, sizeof(*conf));
- conf->root_dir = dir;
+ if (dir)
+ conf->root_dir = svn_dirent_canonicalize(dir, p);
conf->bulk_updates = CONF_FLAG_ON;
conf->v2_protocol = CONF_FLAG_ON;
Re: svn commit: r916286 - in /subversion/trunk/subversion/mod_dav_svn:
dav_svn.h mirror.c mod_dav_svn.c
Posted by Kamesh Jayachandran <ka...@collab.net>.
On 02/26/2010 08:59 PM, Kamesh Jayachandran wrote:
> On 02/25/2010 11:36 PM, Julian Foad wrote:
>> On Thu, 2010-02-25 at 20:45 +0530, Kamesh Jayachandran wrote:
>>> On 02/25/2010 08:29 PM, Julian Foad wrote:
>>>> kameshj@apache.org wrote:
>> [...]
>>>>> - if (strcmp(uri.path, root_dir) == 0) {
>>>>> + if (uri.path)
>>>>> + canonicalized_uri = svn_dirent_canonicalize(uri.path,
>>>>> r->pool);
>>>>>
>>>> Oops, you called "dirent_canonicalize" on a URI.
>>> Is there any uri canonicalize function?.
>> svn_uri_canonicalize() if it's a URI (in which non-URI characters must
>> be escaped as '%XX').
>>
>
> Ok, I wanted to see the failure in my eyes before attempting to fix
> the same.
>
> Following are my observations,
>
> <Location "/svn 1/">
> DAV svn
> SVNParentPath /repositories
> </Location>
> <Location "/svn 2/">
> DAV svn
> SVNParentPath /repositories-slave
> SVNMasterURI "http://localhost/svn 1"
> </Location>
>
> I could not see the difference between "svn_uri_canonicalize()" and
> svn_dirent_canonicalize() for the above configuration, by the way both
> fails while proxying.
Fixed the svn_dirent_canonicalize->svn_uri_canonicalize in r917512.
Fixed the write through proxy *not* proxying the request to master if
the configured location has the encodable content in r917523
With regards
Kamesh Jayachandran
>
> I have an upcoming local patch in progress attempting to fix the same.
>
>
>>> FWIW here uri.path is the PATH portion of the URL, i.e something like
>>> /svn/blah/blah
>> svn_relpath_canonicalize() if it's a "relpath" (see<svn_dirent_uri.h>
>> for definitions).
>>
>> It's definitely wrong to use a "dirent" function on the path portion of
>> a URL.
>>
>> [...]
>>
>>>>> Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=916286&r1=916285&r2=916286&view=diff
>>>>>
>>>>> ==============================================================================
>>>>>
>>>>> --- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
>>>>> +++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Thu Feb
>>>>> 25 13:40:22 2010
>>>>> @@ -169,7 +169,8 @@
>>>>> /* NOTE: dir==NULL creates the default per-dir config */
>>>>> dir_conf_t *conf = apr_pcalloc(p, sizeof(*conf));
>>>>>
>>>>> - conf->root_dir = dir;
>>>>> + if (dir)
>>>>> + conf->root_dir = svn_dirent_canonicalize(dir, p);
>>>>>
>>>> And is this "root_dir" meant to be a disk path or a URI? I'm not
>>>> sure,
>>>> myself.
>>>>
>>> May be the disk path if the context is driven by<Directory
>>> /some/path/to/repo>, though I never tried that way.
>>>
>>> For the<Location /svn/> configuration root_dir is '/svn'(after
>>> canonicaliztion.
>> We need to know what type of path it is and use the correct
>> canonicalization function(s). Maybe it requires two different functions
>> depending on ... something.
>>
>
> IIUC only way to configure the SVN url is via <Location>, so no issues.
>
> With regards
> Kamesh Jayachandran
>> - Julian
>>
>>
>
RE: svn commit: r916286 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mirror.c mod_dav_svn.c
Posted by Kamesh Jayachandran <ka...@collab.net>.
>> I could not see the difference between "svn_uri_canonicalize()" and
>> svn_dirent_canonicalize() for the above configuration, by the way both
>> fails while proxying.
>>
>> I have an upcoming local patch in progress attempting to fix the same.
>Is it a local disk path?
No.
>If no, then never use svn_dirent_*() on it.
>(If yes, always use svn_dirent_*() on it)
Ok.
>svn_dirent_*() has platform dependent behavior.
>You might not see it on your OS, but the canonicalization rules for dirents are defined by your platform.
>Just a few simple examples:
>The canonical format of 'a:/':
>* On linux: 'a:' (never end a path with a '/', except for the root directory)
>* On Windows: 'A:/' (Drive letters always uppercase and in this case followed by a '/', as 'A:' refers to the current directory on drive A:)
>'C:hi' is a dirent to the file named 'C:hi' on Linux, but on Windows it is a dirent pointing to the file 'hi' in the current directory of drive C:.
>If you use svn_uri_*(), these paths are handled the same, which is most likely what you want. The <Location> tag defines the path space on your >webserver.
Thanks for the explanation.
With regards
Kamesh Jayachandran
RE: svn commit: r916286 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mirror.c mod_dav_svn.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Kamesh Jayachandran [mailto:kamesh@collab.net]
> Sent: vrijdag 26 februari 2010 16:29
> To: commits@subversion.apache.org
> Subject: Re: svn commit: r916286 - in
> /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mirror.c
> mod_dav_svn.c
>
> On 02/25/2010 11:36 PM, Julian Foad wrote:
> > On Thu, 2010-02-25 at 20:45 +0530, Kamesh Jayachandran wrote:
> >
> >> On 02/25/2010 08:29 PM, Julian Foad wrote:
> >>
> >>> kameshj@apache.org wrote:
> >>>
> > [...]
> >
> >>>> - if (strcmp(uri.path, root_dir) == 0) {
> >>>> + if (uri.path)
> >>>> + canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
> >>>>
> >>>>
> >>> Oops, you called "dirent_canonicalize" on a URI.
> >>>
> >> Is there any uri canonicalize function?.
> >>
> > svn_uri_canonicalize() if it's a URI (in which non-URI characters must
> > be escaped as '%XX').
> >
> >
>
> Ok, I wanted to see the failure in my eyes before attempting to fix the
> same.
>
> Following are my observations,
>
> <Location "/svn 1/">
> DAV svn
> SVNParentPath /repositories
> </Location>
> <Location "/svn 2/">
> DAV svn
> SVNParentPath /repositories-slave
> SVNMasterURI "http://localhost/svn 1"
> </Location>
>
> I could not see the difference between "svn_uri_canonicalize()" and
> svn_dirent_canonicalize() for the above configuration, by the way both
> fails while proxying.
>
> I have an upcoming local patch in progress attempting to fix the same.
Is it a local disk path?
If no, then never use svn_dirent_*() on it.
(If yes, always use svn_dirent_*() on it)
svn_dirent_*() has platform dependent behavior.
You might not see it on your OS, but the canonicalization rules for dirents are defined by your platform.
Just a few simple examples:
The canonical format of 'a:/':
* On linux: 'a:' (never end a path with a '/', except for the root directory)
* On Windows: 'A:/' (Drive letters always uppercase and in this case followed by a '/', as 'A:' refers to the current directory on drive A:)
'C:hi' is a dirent to the file named 'C:hi' on Linux, but on Windows it is a dirent pointing to the file 'hi' in the current directory of drive C:.
If you use svn_uri_*(), these paths are handled the same, which is most likely what you want. The <Location> tag defines the path space on your webserver.
Bert
Re: svn commit: r916286 - in /subversion/trunk/subversion/mod_dav_svn:
dav_svn.h mirror.c mod_dav_svn.c
Posted by Kamesh Jayachandran <ka...@collab.net>.
On 02/25/2010 11:36 PM, Julian Foad wrote:
> On Thu, 2010-02-25 at 20:45 +0530, Kamesh Jayachandran wrote:
>
>> On 02/25/2010 08:29 PM, Julian Foad wrote:
>>
>>> kameshj@apache.org wrote:
>>>
> [...]
>
>>>> - if (strcmp(uri.path, root_dir) == 0) {
>>>> + if (uri.path)
>>>> + canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
>>>>
>>>>
>>> Oops, you called "dirent_canonicalize" on a URI.
>>>
>> Is there any uri canonicalize function?.
>>
> svn_uri_canonicalize() if it's a URI (in which non-URI characters must
> be escaped as '%XX').
>
>
Ok, I wanted to see the failure in my eyes before attempting to fix the
same.
Following are my observations,
<Location "/svn 1/">
DAV svn
SVNParentPath /repositories
</Location>
<Location "/svn 2/">
DAV svn
SVNParentPath /repositories-slave
SVNMasterURI "http://localhost/svn 1"
</Location>
I could not see the difference between "svn_uri_canonicalize()" and
svn_dirent_canonicalize() for the above configuration, by the way both
fails while proxying.
I have an upcoming local patch in progress attempting to fix the same.
>> FWIW here uri.path is the PATH portion of the URL, i.e something like
>> /svn/blah/blah
>>
> svn_relpath_canonicalize() if it's a "relpath" (see<svn_dirent_uri.h>
> for definitions).
>
> It's definitely wrong to use a "dirent" function on the path portion of
> a URL.
>
> [...]
>
>
>>>> Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
>>>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=916286&r1=916285&r2=916286&view=diff
>>>> ==============================================================================
>>>> --- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
>>>> +++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Thu Feb 25 13:40:22 2010
>>>> @@ -169,7 +169,8 @@
>>>> /* NOTE: dir==NULL creates the default per-dir config */
>>>> dir_conf_t *conf = apr_pcalloc(p, sizeof(*conf));
>>>>
>>>> - conf->root_dir = dir;
>>>> + if (dir)
>>>> + conf->root_dir = svn_dirent_canonicalize(dir, p);
>>>>
>>>>
>>> And is this "root_dir" meant to be a disk path or a URI? I'm not sure,
>>> myself.
>>>
>>>
>> May be the disk path if the context is driven by<Directory
>> /some/path/to/repo>, though I never tried that way.
>>
>> For the<Location /svn/> configuration root_dir is '/svn'(after
>> canonicaliztion.
>>
> We need to know what type of path it is and use the correct
> canonicalization function(s). Maybe it requires two different functions
> depending on ... something.
>
>
IIUC only way to configure the SVN url is via <Location>, so no issues.
With regards
Kamesh Jayachandran
> - Julian
>
>
>
Re: svn commit: r916286 - in
/subversion/trunk/subversion/mod_dav_svn: dav_svn.h mirror.c mod_dav_svn.c
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2010-02-25 at 20:45 +0530, Kamesh Jayachandran wrote:
> On 02/25/2010 08:29 PM, Julian Foad wrote:
> > kameshj@apache.org wrote:
[...]
> >> - if (strcmp(uri.path, root_dir) == 0) {
> >> + if (uri.path)
> >> + canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
> >>
> > Oops, you called "dirent_canonicalize" on a URI.
>
> Is there any uri canonicalize function?.
svn_uri_canonicalize() if it's a URI (in which non-URI characters must
be escaped as '%XX').
> FWIW here uri.path is the PATH portion of the URL, i.e something like
> /svn/blah/blah
svn_relpath_canonicalize() if it's a "relpath" (see <svn_dirent_uri.h>
for definitions).
It's definitely wrong to use a "dirent" function on the path portion of
a URL.
[...]
> >> Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
> >> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=916286&r1=916285&r2=916286&view=diff
> >> ==============================================================================
> >> --- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
> >> +++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Thu Feb 25 13:40:22 2010
> >> @@ -169,7 +169,8 @@
> >> /* NOTE: dir==NULL creates the default per-dir config */
> >> dir_conf_t *conf = apr_pcalloc(p, sizeof(*conf));
> >>
> >> - conf->root_dir = dir;
> >> + if (dir)
> >> + conf->root_dir = svn_dirent_canonicalize(dir, p);
> >>
> > And is this "root_dir" meant to be a disk path or a URI? I'm not sure,
> > myself.
> >
>
> May be the disk path if the context is driven by <Directory
> /some/path/to/repo>, though I never tried that way.
>
> For the <Location /svn/> configuration root_dir is '/svn'(after
> canonicaliztion.
We need to know what type of path it is and use the correct
canonicalization function(s). Maybe it requires two different functions
depending on ... something.
- Julian
Re: svn commit: r916286 - in
/subversion/trunk/subversion/mod_dav_svn: dav_svn.h mirror.c mod_dav_svn.c
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2010-02-25 at 20:45 +0530, Kamesh Jayachandran wrote:
> On 02/25/2010 08:29 PM, Julian Foad wrote:
> > kameshj@apache.org wrote:
[...]
> >> - if (strcmp(uri.path, root_dir) == 0) {
> >> + if (uri.path)
> >> + canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
> >>
> > Oops, you called "dirent_canonicalize" on a URI.
>
> Is there any uri canonicalize function?.
svn_uri_canonicalize() if it's a URI (in which non-URI characters must
be escaped as '%XX').
> FWIW here uri.path is the PATH portion of the URL, i.e something like
> /svn/blah/blah
svn_relpath_canonicalize() if it's a "relpath" (see <svn_dirent_uri.h>
for definitions).
It's definitely wrong to use a "dirent" function on the path portion of
a URL.
[...]
> >> Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
> >> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=916286&r1=916285&r2=916286&view=diff
> >> ==============================================================================
> >> --- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
> >> +++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Thu Feb 25 13:40:22 2010
> >> @@ -169,7 +169,8 @@
> >> /* NOTE: dir==NULL creates the default per-dir config */
> >> dir_conf_t *conf = apr_pcalloc(p, sizeof(*conf));
> >>
> >> - conf->root_dir = dir;
> >> + if (dir)
> >> + conf->root_dir = svn_dirent_canonicalize(dir, p);
> >>
> > And is this "root_dir" meant to be a disk path or a URI? I'm not sure,
> > myself.
> >
>
> May be the disk path if the context is driven by <Directory
> /some/path/to/repo>, though I never tried that way.
>
> For the <Location /svn/> configuration root_dir is '/svn'(after
> canonicaliztion.
We need to know what type of path it is and use the correct
canonicalization function(s). Maybe it requires two different functions
depending on ... something.
- Julian
Re: svn commit: r916286 - in /subversion/trunk/subversion/mod_dav_svn:
dav_svn.h mirror.c mod_dav_svn.c
Posted by Kamesh Jayachandran <ka...@collab.net>.
On 02/25/2010 08:29 PM, Julian Foad wrote:
> kameshj@apache.org wrote:
>
>> Author: kameshj
>> Date: Thu Feb 25 13:40:22 2010
>> New Revision: 916286
>>
> Hi Kamesh. Some review comments below ...
>
>
>> URL: http://svn.apache.org/viewvc?rev=916286&view=rev
>> Log:
>> With the below apache configuration(See the trailing slash at the end of '/svn/').
>>
>> <Location /svn/>
>> DAV svn
>> SVNParentPath /repositories
>> #See the trailing slash on the master URI also can cause the confusion.
>> SVNMasterURI http://master/svn/
>> SVNAdvertiseV2Protocol Off
>> </Location>
>>
>>
>> We get the following error on the client side.
>>
>> svn: Commit failed (details follow):
>> svn: MKACTIVITY of '/svn/demujin/!svn/act/4b6d547c-018d-4e02-9d3f-2b283076cc06': Could not read status line: connection was closed by server (http://localhost)
>>
>>
>> On the server(proxy) it is an assertion error on the following code block from subversion/mod_dav_svn/mirror.c:proxy_request_fixup
>>
>> assert((uri_segment[0] == '\0')
>> || (uri_segment[0] == '/'));
>>
>> For the above configuration we get the uri_segment with the value 'reponame/some/path/inside/the/repo'.
>>
>> We fix this by canonicalizing the 'root_dir'(The one in Location) and
>> 'uri.path'(Path portion of Master URI).
>> * subversion/mod_dav_svn/dav_svn.h
>> (dav_svn__get_root_dir): Document that root_dir is in canonicalized form.
>> * subversion/mod_dav_svn/mod_dav_svn.c
>> (create_dir_config): Canonicalize the root_dir.
>> * subversion/mod_dav_svn/mirror.c
>> (dav_svn__location_in_filter, dav_svn__location_body_filter):
>> As root_dir is in canonical form canonicalize the uri.path too to avoid
>> spurious errors.
>> (dav_svn__location_header_filter): As root_dir is canonical we need to
>> explicitly introduce the path seperator.
>>
>> Suggested by: julianfoad
>>
>> Modified:
>> subversion/trunk/subversion/mod_dav_svn/dav_svn.h
>> subversion/trunk/subversion/mod_dav_svn/mirror.c
>> subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
>>
>> Modified: subversion/trunk/subversion/mod_dav_svn/dav_svn.h
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/dav_svn.h?rev=916286&r1=916285&r2=916286&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/mod_dav_svn/dav_svn.h (original)
>> +++ subversion/trunk/subversion/mod_dav_svn/dav_svn.h Thu Feb 25 13:40:22 2010
>> @@ -352,7 +352,7 @@
>> /* Return the activities db */
>> const char * dav_svn__get_activities_db(request_rec *r);
>>
>> -/* Return the root directory */
>> +/* Return the root directory in canonicalized form */
>> const char * dav_svn__get_root_dir(request_rec *r);
>>
>>
>>
>> Modified: subversion/trunk/subversion/mod_dav_svn/mirror.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mirror.c?rev=916286&r1=916285&r2=916286&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/mod_dav_svn/mirror.c (original)
>> +++ subversion/trunk/subversion/mod_dav_svn/mirror.c Thu Feb 25 13:40:22 2010
>> @@ -128,7 +128,7 @@
>> locate_ctx_t *ctx = f->ctx;
>> apr_status_t rv;
>> apr_bucket *bkt;
>> - const char *master_uri, *root_dir;
>> + const char *master_uri, *root_dir, *canonicalized_uri;
>> apr_uri_t uri;
>>
>> /* Don't filter if we're in a subrequest or we aren't setup to
>> @@ -143,7 +143,11 @@
>> (that is, if our root path matches that of the master server). */
>> apr_uri_parse(r->pool, master_uri,&uri);
>> root_dir = dav_svn__get_root_dir(r);
>> - if (strcmp(uri.path, root_dir) == 0) {
>> + if (uri.path)
>> + canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
>>
> Oops, you called "dirent_canonicalize" on a URI.
>
Is there any uri canonicalize function?.
FWIW here uri.path is the PATH portion of the URL, i.e something like
/svn/blah/blah
>
>> + else
>> + canonicalized_uri = uri.path;
>> + if (strcmp(canonicalized_uri, root_dir) == 0) {
>> ap_remove_input_filter(f);
>> return ap_get_brigade(f->next, bb, mode, block, readbytes);
>> }
>> @@ -156,7 +160,7 @@
>>
>> if (!f->ctx) {
>> ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
>> - ctx->remotepath = uri.path;
>> + ctx->remotepath = canonicalized_uri;
>> ctx->remotepath_len = strlen(ctx->remotepath);
>> ctx->localpath = root_dir;
>> ctx->localpath_len = strlen(ctx->localpath);
>> @@ -226,7 +230,7 @@
>> start_foo += strlen(master_uri);
>> new_uri = ap_construct_url(r->pool,
>> apr_pstrcat(r->pool,
>> - dav_svn__get_root_dir(r),
>> + dav_svn__get_root_dir(r), "/",
>> start_foo, NULL),
>> r);
>> apr_table_set(r->headers_out, "Location", new_uri);
>> @@ -240,7 +244,7 @@
>> request_rec *r = f->r;
>> locate_ctx_t *ctx = f->ctx;
>> apr_bucket *bkt;
>> - const char *master_uri, *root_dir;
>> + const char *master_uri, *root_dir, *canonicalized_uri;
>> apr_uri_t uri;
>>
>> /* Don't filter if we're in a subrequest or we aren't setup to
>> @@ -255,7 +259,11 @@
>> (that is, if our root path matches that of the master server). */
>> apr_uri_parse(r->pool, master_uri,&uri);
>> root_dir = dav_svn__get_root_dir(r);
>> - if (strcmp(uri.path, root_dir) == 0) {
>> + if (uri.path)
>> + canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
>>
> And here.
>
>
Same as above comment.
>> + else
>> + canonicalized_uri = uri.path;
>> + if (strcmp(canonicalized_uri, root_dir) == 0) {
>> ap_remove_output_filter(f);
>> return ap_pass_brigade(f->next, bb);
>> }
>> @@ -268,7 +276,7 @@
>>
>> if (!f->ctx) {
>> ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
>> - ctx->remotepath = uri.path;
>> + ctx->remotepath = canonicalized_uri;
>> ctx->remotepath_len = strlen(ctx->remotepath);
>> ctx->localpath = root_dir;
>> ctx->localpath_len = strlen(ctx->localpath);
>>
>> Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=916286&r1=916285&r2=916286&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
>> +++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Thu Feb 25 13:40:22 2010
>> @@ -169,7 +169,8 @@
>> /* NOTE: dir==NULL creates the default per-dir config */
>> dir_conf_t *conf = apr_pcalloc(p, sizeof(*conf));
>>
>> - conf->root_dir = dir;
>> + if (dir)
>> + conf->root_dir = svn_dirent_canonicalize(dir, p);
>>
> And is this "root_dir" meant to be a disk path or a URI? I'm not sure,
> myself.
>
May be the disk path if the context is driven by <Directory
/some/path/to/repo>, though I never tried that way.
For the <Location /svn/> configuration root_dir is '/svn'(after
canonicaliztion.
Thanks for the review.
With regards
Kamesh Jayachandran
Re: svn commit: r916286 - in
/subversion/trunk/subversion/mod_dav_svn: dav_svn.h mirror.c mod_dav_svn.c
Posted by Julian Foad <ju...@btopenworld.com>.
kameshj@apache.org wrote:
> Author: kameshj
> Date: Thu Feb 25 13:40:22 2010
> New Revision: 916286
Hi Kamesh. Some review comments below ...
> URL: http://svn.apache.org/viewvc?rev=916286&view=rev
> Log:
> With the below apache configuration(See the trailing slash at the end of '/svn/').
>
> <Location /svn/>
> DAV svn
> SVNParentPath /repositories
> #See the trailing slash on the master URI also can cause the confusion.
> SVNMasterURI http://master/svn/
> SVNAdvertiseV2Protocol Off
> </Location>
>
>
> We get the following error on the client side.
>
> svn: Commit failed (details follow):
> svn: MKACTIVITY of '/svn/demujin/!svn/act/4b6d547c-018d-4e02-9d3f-2b283076cc06': Could not read status line: connection was closed by server (http://localhost)
>
>
> On the server(proxy) it is an assertion error on the following code block from subversion/mod_dav_svn/mirror.c:proxy_request_fixup
>
> assert((uri_segment[0] == '\0')
> || (uri_segment[0] == '/'));
>
> For the above configuration we get the uri_segment with the value 'reponame/some/path/inside/the/repo'.
>
> We fix this by canonicalizing the 'root_dir'(The one in Location) and
> 'uri.path'(Path portion of Master URI).
> * subversion/mod_dav_svn/dav_svn.h
> (dav_svn__get_root_dir): Document that root_dir is in canonicalized form.
> * subversion/mod_dav_svn/mod_dav_svn.c
> (create_dir_config): Canonicalize the root_dir.
> * subversion/mod_dav_svn/mirror.c
> (dav_svn__location_in_filter, dav_svn__location_body_filter):
> As root_dir is in canonical form canonicalize the uri.path too to avoid
> spurious errors.
> (dav_svn__location_header_filter): As root_dir is canonical we need to
> explicitly introduce the path seperator.
>
> Suggested by: julianfoad
>
> Modified:
> subversion/trunk/subversion/mod_dav_svn/dav_svn.h
> subversion/trunk/subversion/mod_dav_svn/mirror.c
> subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
>
> Modified: subversion/trunk/subversion/mod_dav_svn/dav_svn.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/dav_svn.h?rev=916286&r1=916285&r2=916286&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/dav_svn.h (original)
> +++ subversion/trunk/subversion/mod_dav_svn/dav_svn.h Thu Feb 25 13:40:22 2010
> @@ -352,7 +352,7 @@
> /* Return the activities db */
> const char * dav_svn__get_activities_db(request_rec *r);
>
> -/* Return the root directory */
> +/* Return the root directory in canonicalized form */
> const char * dav_svn__get_root_dir(request_rec *r);
>
>
>
> Modified: subversion/trunk/subversion/mod_dav_svn/mirror.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mirror.c?rev=916286&r1=916285&r2=916286&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/mirror.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/mirror.c Thu Feb 25 13:40:22 2010
> @@ -128,7 +128,7 @@
> locate_ctx_t *ctx = f->ctx;
> apr_status_t rv;
> apr_bucket *bkt;
> - const char *master_uri, *root_dir;
> + const char *master_uri, *root_dir, *canonicalized_uri;
> apr_uri_t uri;
>
> /* Don't filter if we're in a subrequest or we aren't setup to
> @@ -143,7 +143,11 @@
> (that is, if our root path matches that of the master server). */
> apr_uri_parse(r->pool, master_uri, &uri);
> root_dir = dav_svn__get_root_dir(r);
> - if (strcmp(uri.path, root_dir) == 0) {
> + if (uri.path)
> + canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
Oops, you called "dirent_canonicalize" on a URI.
> + else
> + canonicalized_uri = uri.path;
> + if (strcmp(canonicalized_uri, root_dir) == 0) {
> ap_remove_input_filter(f);
> return ap_get_brigade(f->next, bb, mode, block, readbytes);
> }
> @@ -156,7 +160,7 @@
>
> if (!f->ctx) {
> ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
> - ctx->remotepath = uri.path;
> + ctx->remotepath = canonicalized_uri;
> ctx->remotepath_len = strlen(ctx->remotepath);
> ctx->localpath = root_dir;
> ctx->localpath_len = strlen(ctx->localpath);
> @@ -226,7 +230,7 @@
> start_foo += strlen(master_uri);
> new_uri = ap_construct_url(r->pool,
> apr_pstrcat(r->pool,
> - dav_svn__get_root_dir(r),
> + dav_svn__get_root_dir(r), "/",
> start_foo, NULL),
> r);
> apr_table_set(r->headers_out, "Location", new_uri);
> @@ -240,7 +244,7 @@
> request_rec *r = f->r;
> locate_ctx_t *ctx = f->ctx;
> apr_bucket *bkt;
> - const char *master_uri, *root_dir;
> + const char *master_uri, *root_dir, *canonicalized_uri;
> apr_uri_t uri;
>
> /* Don't filter if we're in a subrequest or we aren't setup to
> @@ -255,7 +259,11 @@
> (that is, if our root path matches that of the master server). */
> apr_uri_parse(r->pool, master_uri, &uri);
> root_dir = dav_svn__get_root_dir(r);
> - if (strcmp(uri.path, root_dir) == 0) {
> + if (uri.path)
> + canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
And here.
> + else
> + canonicalized_uri = uri.path;
> + if (strcmp(canonicalized_uri, root_dir) == 0) {
> ap_remove_output_filter(f);
> return ap_pass_brigade(f->next, bb);
> }
> @@ -268,7 +276,7 @@
>
> if (!f->ctx) {
> ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
> - ctx->remotepath = uri.path;
> + ctx->remotepath = canonicalized_uri;
> ctx->remotepath_len = strlen(ctx->remotepath);
> ctx->localpath = root_dir;
> ctx->localpath_len = strlen(ctx->localpath);
>
> Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=916286&r1=916285&r2=916286&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Thu Feb 25 13:40:22 2010
> @@ -169,7 +169,8 @@
> /* NOTE: dir==NULL creates the default per-dir config */
> dir_conf_t *conf = apr_pcalloc(p, sizeof(*conf));
>
> - conf->root_dir = dir;
> + if (dir)
> + conf->root_dir = svn_dirent_canonicalize(dir, p);
And is this "root_dir" meant to be a disk path or a URI? I'm not sure,
myself.
> conf->bulk_updates = CONF_FLAG_ON;
> conf->v2_protocol = CONF_FLAG_ON;
- Julian
Re: svn commit: r916286 - in
/subversion/trunk/subversion/mod_dav_svn: dav_svn.h mirror.c mod_dav_svn.c
Posted by Julian Foad <ju...@btopenworld.com>.
kameshj@apache.org wrote:
> Author: kameshj
> Date: Thu Feb 25 13:40:22 2010
> New Revision: 916286
Hi Kamesh. Some review comments below ...
> URL: http://svn.apache.org/viewvc?rev=916286&view=rev
> Log:
> With the below apache configuration(See the trailing slash at the end of '/svn/').
>
> <Location /svn/>
> DAV svn
> SVNParentPath /repositories
> #See the trailing slash on the master URI also can cause the confusion.
> SVNMasterURI http://master/svn/
> SVNAdvertiseV2Protocol Off
> </Location>
>
>
> We get the following error on the client side.
>
> svn: Commit failed (details follow):
> svn: MKACTIVITY of '/svn/demujin/!svn/act/4b6d547c-018d-4e02-9d3f-2b283076cc06': Could not read status line: connection was closed by server (http://localhost)
>
>
> On the server(proxy) it is an assertion error on the following code block from subversion/mod_dav_svn/mirror.c:proxy_request_fixup
>
> assert((uri_segment[0] == '\0')
> || (uri_segment[0] == '/'));
>
> For the above configuration we get the uri_segment with the value 'reponame/some/path/inside/the/repo'.
>
> We fix this by canonicalizing the 'root_dir'(The one in Location) and
> 'uri.path'(Path portion of Master URI).
> * subversion/mod_dav_svn/dav_svn.h
> (dav_svn__get_root_dir): Document that root_dir is in canonicalized form.
> * subversion/mod_dav_svn/mod_dav_svn.c
> (create_dir_config): Canonicalize the root_dir.
> * subversion/mod_dav_svn/mirror.c
> (dav_svn__location_in_filter, dav_svn__location_body_filter):
> As root_dir is in canonical form canonicalize the uri.path too to avoid
> spurious errors.
> (dav_svn__location_header_filter): As root_dir is canonical we need to
> explicitly introduce the path seperator.
>
> Suggested by: julianfoad
>
> Modified:
> subversion/trunk/subversion/mod_dav_svn/dav_svn.h
> subversion/trunk/subversion/mod_dav_svn/mirror.c
> subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
>
> Modified: subversion/trunk/subversion/mod_dav_svn/dav_svn.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/dav_svn.h?rev=916286&r1=916285&r2=916286&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/dav_svn.h (original)
> +++ subversion/trunk/subversion/mod_dav_svn/dav_svn.h Thu Feb 25 13:40:22 2010
> @@ -352,7 +352,7 @@
> /* Return the activities db */
> const char * dav_svn__get_activities_db(request_rec *r);
>
> -/* Return the root directory */
> +/* Return the root directory in canonicalized form */
> const char * dav_svn__get_root_dir(request_rec *r);
>
>
>
> Modified: subversion/trunk/subversion/mod_dav_svn/mirror.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mirror.c?rev=916286&r1=916285&r2=916286&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/mirror.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/mirror.c Thu Feb 25 13:40:22 2010
> @@ -128,7 +128,7 @@
> locate_ctx_t *ctx = f->ctx;
> apr_status_t rv;
> apr_bucket *bkt;
> - const char *master_uri, *root_dir;
> + const char *master_uri, *root_dir, *canonicalized_uri;
> apr_uri_t uri;
>
> /* Don't filter if we're in a subrequest or we aren't setup to
> @@ -143,7 +143,11 @@
> (that is, if our root path matches that of the master server). */
> apr_uri_parse(r->pool, master_uri, &uri);
> root_dir = dav_svn__get_root_dir(r);
> - if (strcmp(uri.path, root_dir) == 0) {
> + if (uri.path)
> + canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
Oops, you called "dirent_canonicalize" on a URI.
> + else
> + canonicalized_uri = uri.path;
> + if (strcmp(canonicalized_uri, root_dir) == 0) {
> ap_remove_input_filter(f);
> return ap_get_brigade(f->next, bb, mode, block, readbytes);
> }
> @@ -156,7 +160,7 @@
>
> if (!f->ctx) {
> ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
> - ctx->remotepath = uri.path;
> + ctx->remotepath = canonicalized_uri;
> ctx->remotepath_len = strlen(ctx->remotepath);
> ctx->localpath = root_dir;
> ctx->localpath_len = strlen(ctx->localpath);
> @@ -226,7 +230,7 @@
> start_foo += strlen(master_uri);
> new_uri = ap_construct_url(r->pool,
> apr_pstrcat(r->pool,
> - dav_svn__get_root_dir(r),
> + dav_svn__get_root_dir(r), "/",
> start_foo, NULL),
> r);
> apr_table_set(r->headers_out, "Location", new_uri);
> @@ -240,7 +244,7 @@
> request_rec *r = f->r;
> locate_ctx_t *ctx = f->ctx;
> apr_bucket *bkt;
> - const char *master_uri, *root_dir;
> + const char *master_uri, *root_dir, *canonicalized_uri;
> apr_uri_t uri;
>
> /* Don't filter if we're in a subrequest or we aren't setup to
> @@ -255,7 +259,11 @@
> (that is, if our root path matches that of the master server). */
> apr_uri_parse(r->pool, master_uri, &uri);
> root_dir = dav_svn__get_root_dir(r);
> - if (strcmp(uri.path, root_dir) == 0) {
> + if (uri.path)
> + canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
And here.
> + else
> + canonicalized_uri = uri.path;
> + if (strcmp(canonicalized_uri, root_dir) == 0) {
> ap_remove_output_filter(f);
> return ap_pass_brigade(f->next, bb);
> }
> @@ -268,7 +276,7 @@
>
> if (!f->ctx) {
> ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
> - ctx->remotepath = uri.path;
> + ctx->remotepath = canonicalized_uri;
> ctx->remotepath_len = strlen(ctx->remotepath);
> ctx->localpath = root_dir;
> ctx->localpath_len = strlen(ctx->localpath);
>
> Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=916286&r1=916285&r2=916286&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Thu Feb 25 13:40:22 2010
> @@ -169,7 +169,8 @@
> /* NOTE: dir==NULL creates the default per-dir config */
> dir_conf_t *conf = apr_pcalloc(p, sizeof(*conf));
>
> - conf->root_dir = dir;
> + if (dir)
> + conf->root_dir = svn_dirent_canonicalize(dir, p);
And is this "root_dir" meant to be a disk path or a URI? I'm not sure,
myself.
> conf->bulk_updates = CONF_FLAG_ON;
> conf->v2_protocol = CONF_FLAG_ON;
- Julian