You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Küng <to...@gmail.com> on 2011/12/28 08:01:19 UTC
crash fetching status
From crash dumps sent for TSVN, there's a crash happening when fetching
the status. The stacktrace:
libsvn_tsvn32.dll!svn_relpath_join(const char * base=0x00000000,
const char * component=0x01fe2154, apr_pool_t * pool=0x01fe2080) Line
1158 + 0xc bytes C
libsvn_tsvn32.dll!make_file_baton(dir_baton *
parent_dir_baton=0x01fe80e0, const char * path=0x01d81f70, apr_pool_t *
pool=0x01fe2080) Line 1583 + 0x2d bytes C
libsvn_tsvn32.dll!add_file(const char * path=0x01d81f70, void *
parent_baton=0x01fe80e0, const char * copyfrom_path=0x00000000, long
copyfrom_revision=-1, apr_pool_t * pool=0x01fe2080, void * *
file_baton=0x01fe20e4) Line 2040 C
libsvn_tsvn32.dll!add_file(const char * path=0x01d81f70, void *
parent_baton=0x01fe80d8, const char * copyfrom_path=0x00000000, long
copyfrom_revision=-1, apr_pool_t * pool=0x01fe2080, void * *
file_baton=0x01fe20c4) Line 172 + 0x1f bytes C
libsvn_tsvn32.dll!ra_svn_handle_add_file(svn_ra_svn_conn_st *
conn=0x01dc2390, apr_pool_t * pool=0x01d81d88, const apr_array_header_t
* params=0x01d81e58, ra_svn_driver_state_t * ds=0x0254f084) Line 653 +
0x23 bytes C
libsvn_tsvn32.dll!svn_ra_svn_drive_editor2(svn_ra_svn_conn_st *
conn=0x01dc2390, apr_pool_t * pool=0x01d67d20, const svn_delta_editor_t
* editor=0x01d85528, void * edit_baton=0x01d85568, int *
aborted=0x00000000, int for_replay=0) Line 916 + 0x1d bytes C
libsvn_tsvn32.dll!ra_svn_finish_report(void * baton=0x01d6d0c0,
apr_pool_t * pool=0x01d67d20) Line 300 + 0x19 bytes C
libsvn_tsvn32.dll!reporter_finish_report(void *
report_baton=0x0254f1e4, apr_pool_t * pool=0x01d67d20) Line 217 + 0xc
bytes C
libsvn_tsvn32.dll!svn_wc_crawl_revisions5(svn_wc_context_t *
wc_ctx=0x01d67dc0, const char * local_abspath=0x01d685c0, const
svn_ra_reporter3_t * reporter=0x6c984028, void *
report_baton=0x0254f1e4, int restore_files=0, svn_depth_t depth=-2, int
honor_depth_exclude=0, int depth_compatibility_trick=0, int
use_commit_times=0, svn_error_t * (void *)* cancel_func=0x00360ee1, void
* cancel_baton=0x00a8f81c, void (void *, const svn_wc_notify_t *,
apr_pool_t *)* notify_func=0x00000000, void * notify_baton=0x00000000,
apr_pool_t * scratch_pool=0x01d67d20) Line 861 + 0xb bytes C
libsvn_tsvn32.dll!svn_client_status5(long * result_rev=0x0254f534,
svn_client_ctx_t * ctx=0x01d67d60, const char * path=0x01d26dd8, const
svn_opt_revision_t * revision=0x0254f2dc, svn_depth_t depth=-2, int
get_all=1, int update=1, int no_ignore=0, int ignore_externals=0, int
depth_as_sticky=1, const apr_array_header_t * changelists=0x00000000,
svn_error_t * (void *, const char *, const svn_client_status_t *,
apr_pool_t *)* status_func=0x00360ac4, void * status_baton=0x0254f2bc,
apr_pool_t * pool=0x01d67d20) Line 456 + 0x3c bytes C
Problem is in libsvn_wc\status.c, make_file_baton():
the line
f->repos_relpath = svn_relpath_join(find_dir_repos_relpath(pb, pool),
f->name, pool);
calls svn_relpath_join() with the first parameter as NULL, which then
leads to the crash because svn_relpath_join calls strlen() on the first
parameter.
The comments in find_dir_repos_relpath() clearly indicate that its
return value can be NULL in certain situations, which is what happens here.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: crash fetching status
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote:
> Yes, because NULL is not a valid relpath. The 'relpath' concept is well
> defined (in svn_dirent_uri.h). The representation of an empty relpath is the
> empty string.
Actually 'svn_dirent_uri.h' didn't spell out explicitly that paths are non-null unless documented otherwise. The old 'svn_path.h' did, and I've copied that note into 'svn_dirent_uri.h' in r1226740.
- Julian
Re: crash fetching status
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Küng wrote:
> On Thu, Dec 29, 2011 at 16:57, Stefan Küng wrote:
>> On 29.12.2011 16:43, Hyrum K Wright wrote:
>>> On Wed, Dec 28, 2011 at 1:01 AM, Stefan Küng wrote:
[...]
>>> I looked at similar places elsewhere, and they seemed to follow a
>>> pattern. The following patch is in a similar vein:
>>>
>>> [[[
>>> Index: subversion/libsvn_wc/status.c
>>> ===================================================================
>>> - f->repos_relpath = svn_relpath_join(find_dir_repos_relpath(pb, pool),
>>> - f->name, pool);
>>> +
>>> + dir_repos_relpath = find_dir_repos_relpath(pb, pool);
>>> + if (dir_repos_relpath)
>>> + f->repos_relpath = svn_relpath_join(dir_repos_relpath, f->name,
>>> pool);
>>> + else
>>> + f->repos_relpath = apr_pstrdup(pool, f->name);
>>> +
>>>
>>> All the tests pass, though I'm not sure if there are other correctness
>>> issues here or not, so I haven't committed this.
>>
>> Just a thought:
>> svn_relpath_join() joins two path/url parts. If one is an empty string, it
>> still works. But it crashes when one of the parts is NULL instead of an
>> empty string.
Yes, because NULL is not a valid relpath. The 'relpath' concept is well defined (in svn_dirent_uri.h). The representation of an empty relpath is the empty string.
The real problem here is that find_dir_repos_relpath() can return NULL whereas its callers (including but not limited to this one) expect a non-null value.
So the real solution is either to change find_dir_repos_relpath() so it always returns a valid 'relpath', or to change its callers to code around the fact that it can return NULL.
Does it return NULL to mean the empty relpath or for some other reason?
>> Maybe just rewrite svn_relpath_join() so that NULL is treated as an empty
>> string?
No. Don't make an exception.
- Julian
Re: crash fetching status
Posted by Stefan Küng <to...@gmail.com>.
On Thu, Dec 29, 2011 at 16:57, Stefan Küng <to...@gmail.com> wrote:
> On 29.12.2011 16:43, Hyrum K Wright wrote:
>>
>> On Wed, Dec 28, 2011 at 1:01 AM, Stefan Küng<to...@gmail.com>
>> wrote:
>
>
>> I looked at similar places elsewhere, and they seemed to follow a
>> pattern. The following patch is in a similar vein:
>>
>> [[[
>> Index: subversion/libsvn_wc/status.c
>> ===================================================================
>> --- subversion/libsvn_wc/status.c (revision 1225565)
>> +++ subversion/libsvn_wc/status.c (working copy)
>> @@ -1795,6 +1795,7 @@
>> struct dir_baton *pb = parent_dir_baton;
>> struct edit_baton *eb = pb->edit_baton;
>> struct file_baton *f = apr_pcalloc(pool, sizeof(*f));
>> + const char *dir_repos_relpath;
>>
>> /* Finish populating the baton members. */
>> f->local_abspath = svn_dirent_join(eb->anchor_abspath, path, pool);
>> @@ -1804,8 +1805,13 @@
>> f->edit_baton = eb;
>> f->ood_changed_rev = SVN_INVALID_REVNUM;
>> f->ood_changed_date = 0;
>> - f->repos_relpath = svn_relpath_join(find_dir_repos_relpath(pb, pool),
>> - f->name, pool);
>> +
>> + dir_repos_relpath = find_dir_repos_relpath(pb, pool);
>> + if (dir_repos_relpath)
>> + f->repos_relpath = svn_relpath_join(dir_repos_relpath, f->name,
>> pool);
>> + else
>> + f->repos_relpath = apr_pstrdup(pool, f->name);
>> +
>> f->ood_kind = svn_node_file;
>> f->ood_changed_author = NULL;
>> return f;
>> ]]]
>>
>> All the tests pass, though I'm not sure if there are other correctness
>> issues here or not, so I haven't committed this.
>
>
> Just a thought:
> svn_relpath_join() joins two path/url parts. If one is an empty string, it
> still works. But it crashes when one of the parts is NULL instead of an
> empty string.
> Maybe just rewrite svn_relpath_join() so that NULL is treated as an empty
> string? My thought is that this would be what's intended in those situations
> anyway?
I think this would be the best solution: there's a comment in
svn_relpath_join() that says:
/* If either is empty return the other */
So it really is intended to work this way. Treating a NULL string as
an empty string should work best IMHO.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: crash fetching status
Posted by Stefan Küng <to...@gmail.com>.
On 29.12.2011 16:43, Hyrum K Wright wrote:
> On Wed, Dec 28, 2011 at 1:01 AM, Stefan Küng<to...@gmail.com> wrote:
> I looked at similar places elsewhere, and they seemed to follow a
> pattern. The following patch is in a similar vein:
>
> [[[
> Index: subversion/libsvn_wc/status.c
> ===================================================================
> --- subversion/libsvn_wc/status.c (revision 1225565)
> +++ subversion/libsvn_wc/status.c (working copy)
> @@ -1795,6 +1795,7 @@
> struct dir_baton *pb = parent_dir_baton;
> struct edit_baton *eb = pb->edit_baton;
> struct file_baton *f = apr_pcalloc(pool, sizeof(*f));
> + const char *dir_repos_relpath;
>
> /* Finish populating the baton members. */
> f->local_abspath = svn_dirent_join(eb->anchor_abspath, path, pool);
> @@ -1804,8 +1805,13 @@
> f->edit_baton = eb;
> f->ood_changed_rev = SVN_INVALID_REVNUM;
> f->ood_changed_date = 0;
> - f->repos_relpath = svn_relpath_join(find_dir_repos_relpath(pb, pool),
> - f->name, pool);
> +
> + dir_repos_relpath = find_dir_repos_relpath(pb, pool);
> + if (dir_repos_relpath)
> + f->repos_relpath = svn_relpath_join(dir_repos_relpath, f->name, pool);
> + else
> + f->repos_relpath = apr_pstrdup(pool, f->name);
> +
> f->ood_kind = svn_node_file;
> f->ood_changed_author = NULL;
> return f;
> ]]]
>
> All the tests pass, though I'm not sure if there are other correctness
> issues here or not, so I haven't committed this.
Just a thought:
svn_relpath_join() joins two path/url parts. If one is an empty string,
it still works. But it crashes when one of the parts is NULL instead of
an empty string.
Maybe just rewrite svn_relpath_join() so that NULL is treated as an
empty string? My thought is that this would be what's intended in those
situations anyway?
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: crash fetching status
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Wed, Dec 28, 2011 at 1:01 AM, Stefan Küng <to...@gmail.com> wrote:
> From crash dumps sent for TSVN, there's a crash happening when fetching the
> status. The stacktrace:
>
> libsvn_tsvn32.dll!svn_relpath_join(const char * base=0x00000000,
> const char * component=0x01fe2154, apr_pool_t * pool=0x01fe2080) Line 1158
> + 0xc bytes C
> libsvn_tsvn32.dll!make_file_baton(dir_baton *
> parent_dir_baton=0x01fe80e0, const char * path=0x01d81f70, apr_pool_t *
> pool=0x01fe2080) Line 1583 + 0x2d bytes C
> libsvn_tsvn32.dll!add_file(const char * path=0x01d81f70, void *
> parent_baton=0x01fe80e0, const char * copyfrom_path=0x00000000, long
> copyfrom_revision=-1, apr_pool_t * pool=0x01fe2080, void * *
> file_baton=0x01fe20e4) Line 2040 C
> libsvn_tsvn32.dll!add_file(const char * path=0x01d81f70, void *
> parent_baton=0x01fe80d8, const char * copyfrom_path=0x00000000, long
> copyfrom_revision=-1, apr_pool_t * pool=0x01fe2080, void * *
> file_baton=0x01fe20c4) Line 172 + 0x1f bytes C
> libsvn_tsvn32.dll!ra_svn_handle_add_file(svn_ra_svn_conn_st *
> conn=0x01dc2390, apr_pool_t * pool=0x01d81d88, const apr_array_header_t *
> params=0x01d81e58, ra_svn_driver_state_t * ds=0x0254f084) Line 653 + 0x23
> bytes C
> libsvn_tsvn32.dll!svn_ra_svn_drive_editor2(svn_ra_svn_conn_st *
> conn=0x01dc2390, apr_pool_t * pool=0x01d67d20, const svn_delta_editor_t *
> editor=0x01d85528, void * edit_baton=0x01d85568, int * aborted=0x00000000,
> int for_replay=0) Line 916 + 0x1d bytes C
> libsvn_tsvn32.dll!ra_svn_finish_report(void * baton=0x01d6d0c0,
> apr_pool_t * pool=0x01d67d20) Line 300 + 0x19 bytes C
> libsvn_tsvn32.dll!reporter_finish_report(void *
> report_baton=0x0254f1e4, apr_pool_t * pool=0x01d67d20) Line 217 + 0xc bytes
> C
> libsvn_tsvn32.dll!svn_wc_crawl_revisions5(svn_wc_context_t *
> wc_ctx=0x01d67dc0, const char * local_abspath=0x01d685c0, const
> svn_ra_reporter3_t * reporter=0x6c984028, void * report_baton=0x0254f1e4,
> int restore_files=0, svn_depth_t depth=-2, int honor_depth_exclude=0, int
> depth_compatibility_trick=0, int use_commit_times=0, svn_error_t * (void *)*
> cancel_func=0x00360ee1, void * cancel_baton=0x00a8f81c, void (void *, const
> svn_wc_notify_t *, apr_pool_t *)* notify_func=0x00000000, void *
> notify_baton=0x00000000, apr_pool_t * scratch_pool=0x01d67d20) Line 861 +
> 0xb bytes C
> libsvn_tsvn32.dll!svn_client_status5(long * result_rev=0x0254f534,
> svn_client_ctx_t * ctx=0x01d67d60, const char * path=0x01d26dd8, const
> svn_opt_revision_t * revision=0x0254f2dc, svn_depth_t depth=-2, int
> get_all=1, int update=1, int no_ignore=0, int ignore_externals=0, int
> depth_as_sticky=1, const apr_array_header_t * changelists=0x00000000,
> svn_error_t * (void *, const char *, const svn_client_status_t *, apr_pool_t
> *)* status_func=0x00360ac4, void * status_baton=0x0254f2bc, apr_pool_t *
> pool=0x01d67d20) Line 456 + 0x3c bytes C
>
>
> Problem is in libsvn_wc\status.c, make_file_baton():
> the line
> f->repos_relpath = svn_relpath_join(find_dir_repos_relpath(pb, pool),
> f->name, pool);
> calls svn_relpath_join() with the first parameter as NULL, which then leads
> to the crash because svn_relpath_join calls strlen() on the first parameter.
>
> The comments in find_dir_repos_relpath() clearly indicate that its return
> value can be NULL in certain situations, which is what happens here.
I looked at similar places elsewhere, and they seemed to follow a
pattern. The following patch is in a similar vein:
[[[
Index: subversion/libsvn_wc/status.c
===================================================================
--- subversion/libsvn_wc/status.c (revision 1225565)
+++ subversion/libsvn_wc/status.c (working copy)
@@ -1795,6 +1795,7 @@
struct dir_baton *pb = parent_dir_baton;
struct edit_baton *eb = pb->edit_baton;
struct file_baton *f = apr_pcalloc(pool, sizeof(*f));
+ const char *dir_repos_relpath;
/* Finish populating the baton members. */
f->local_abspath = svn_dirent_join(eb->anchor_abspath, path, pool);
@@ -1804,8 +1805,13 @@
f->edit_baton = eb;
f->ood_changed_rev = SVN_INVALID_REVNUM;
f->ood_changed_date = 0;
- f->repos_relpath = svn_relpath_join(find_dir_repos_relpath(pb, pool),
- f->name, pool);
+
+ dir_repos_relpath = find_dir_repos_relpath(pb, pool);
+ if (dir_repos_relpath)
+ f->repos_relpath = svn_relpath_join(dir_repos_relpath, f->name, pool);
+ else
+ f->repos_relpath = apr_pstrdup(pool, f->name);
+
f->ood_kind = svn_node_file;
f->ood_changed_author = NULL;
return f;
]]]
All the tests pass, though I'm not sure if there are other correctness
issues here or not, so I haven't committed this.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/