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 2012/01/05 20:37:22 UTC
Crash when updating
Hi,
From a crash report dump sent for TSVN for an update the attached stack
trace happened.
libsvn_wc\props.c, svn_wc__merge_props()
to_val = incoming_change->value
? svn_string_dup(incoming_change->value, result_pool) : NULL;
....
if (! from_val) /* adding a new property */
SVN_ERR(apply_single_prop_add(state, &conflict_remains,
db, local_abspath,
left_version, right_version,
is_dir, actual_props,
propname, base_val, to_val,
conflict_func, conflict_baton,
dry_run, result_pool, iterpool));
and then in apply_single_prop_add():
...
if (svn_string_compare(working_val, new_val))
crashes, because new_val is NULL.
the property is "svn:mergeinfo".
Could this happen if someone manually removes the svn:mergeinfo property
and then the next update (from another working copy) gets the to_val as
NULL?
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: Crash when updating
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Jan 10, 2012 at 2:07 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>> -----Original Message-----
>> From: Paul Burba [mailto:ptburba@gmail.com]
>> Sent: dinsdag 10 januari 2012 17:36
>> To: Bert Huijben
>> Cc: Subversion Development; Stefan Küng
>> Subject: Re: Crash when updating
>
> <snip>
>> if (! from_val) /* adding a new property */
>> SVN_ERR(apply_single_prop_add(state, &conflict_remains,
>> db, local_abspath,
>> left_version, right_version,
>> is_dir, actual_props,
>> propname, base_val, to_val,
>> ### ^^^^
>> ### This is obviously going to blow up since to_val is null!
>>
>> conflict_func, conflict_baton,
>> dry_run, result_pool, iterpool));
>> ]]]
>>
>> So it seems we need to put
>> subversion/libsvn_wc/wc_db.c:svn_wc__db_external_read_pristine_props(
>> )
>> back. Does that seem right to you, or is there a better way?
>
> I think we need to call svn_wc__db_read_pristine_props() instead.
>
> The db_external_ variants of the db functions are from when file externals were stored in a different SqLite table
That works, thanks. Fixed r1229833. Nominated for backport to 1.7.3.
Paul
RE: Crash when updating
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Paul Burba [mailto:ptburba@gmail.com]
> Sent: dinsdag 10 januari 2012 17:36
> To: Bert Huijben
> Cc: Subversion Development; Stefan Küng
> Subject: Re: Crash when updating
<snip>
> if (! from_val) /* adding a new property */
> SVN_ERR(apply_single_prop_add(state, &conflict_remains,
> db, local_abspath,
> left_version, right_version,
> is_dir, actual_props,
> propname, base_val, to_val,
> ### ^^^^
> ### This is obviously going to blow up since to_val is null!
>
> conflict_func, conflict_baton,
> dry_run, result_pool, iterpool));
> ]]]
>
> So it seems we need to put
> subversion/libsvn_wc/wc_db.c:svn_wc__db_external_read_pristine_props(
> )
> back. Does that seem right to you, or is there a better way?
I think we need to call svn_wc__db_read_pristine_props() instead.
The db_external_ variants of the db functions are from when file externals were stored in a different SqLite table
Bert
>
> Paul
Re: Crash when updating
Posted by Paul Burba <pt...@gmail.com>.
On Mon, Jan 9, 2012 at 10:16 AM, Paul Burba <pt...@gmail.com> wrote:
> On Thu, Jan 5, 2012 at 2:37 PM, Stefan Küng <to...@gmail.com> wrote:
>> Hi,
>>
>> From a crash report dump sent for TSVN for an update the attached stack
>> trace happened.
>>
>> libsvn_wc\props.c, svn_wc__merge_props()
>> to_val = incoming_change->value
>> ? svn_string_dup(incoming_change->value, result_pool) : NULL;
>> ....
>> if (! from_val) /* adding a new property */
>> SVN_ERR(apply_single_prop_add(state, &conflict_remains,
>> db, local_abspath,
>> left_version, right_version,
>> is_dir, actual_props,
>> propname, base_val, to_val,
>> conflict_func, conflict_baton,
>> dry_run, result_pool, iterpool));
>> and then in apply_single_prop_add():
>> ...
>> if (svn_string_compare(working_val, new_val))
>> crashes, because new_val is NULL.
>>
>> the property is "svn:mergeinfo".
>
>
> Hi Stefan,
>
> I saw 'mergeinfo' so took a quick look (happily it has nothing to do
> with mergeinfo per se, any property will do ;-)
>
>> Could this happen if someone manually removes the svn:mergeinfo property and
>> then the next update (from another working copy) gets the to_val as NULL?
>
> I'm not sure if this is what you mean, but I see one way to replicate
> this problem: Create a file external mapping to a file with some
> arbitrary property. Remap the same external to a file without that
> property. Boom.
>
> I filed an issue (issue #4093) which gives the full replication
> recipe. I also added a regression test in r1228340.
Hi Bert,
I traced this regression back to your change in r1124782:
> Modified: subversion/trunk/subversion/libsvn_wc/externals.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/externals.c?rev=1124782&r1=1124781&r2=1124782&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/externals.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/externals.c Thu May 19 13:53:20 2011
.
<snip>
.
> @@ -607,41 +607,17 @@ close_file(void *file_baton,
> svn_skel_t *all_work_items = NULL;
> svn_skel_t *work_item;
> apr_hash_t *base_props = NULL;
base_props set to null...
> apr_hash_t *actual_props = NULL;
> apr_hash_t *new_pristine_props = NULL;
> apr_hash_t *new_actual_props = NULL;
> apr_hash_t *new_dav_props = NULL;
> const svn_checksum_t *new_checksum = NULL;
> const svn_checksum_t *original_checksum = NULL;
> - svn_revnum_t changed_rev = SVN_INVALID_REVNUM;
> - apr_time_t changed_date = 0;
> - const char *changed_author = NULL;
> +
> svn_boolean_t added = !SVN_IS_VALID_REVNUM(eb->original_revision);
> const char *repos_relpath = svn_uri_is_child(eb->repos_root_url,
> eb->url, pool);
>
> if (! added)
> {
> - svn_boolean_t had_props;
> - svn_boolean_t props_mod;
> + new_checksum = eb->original_checksum;
>
> - SVN_ERR(svn_wc__db_external_read(NULL, NULL, NULL, NULL, NULL, NULL,
> - &changed_rev, &changed_date,
> - &changed_author, &original_checksum,
> - NULL, NULL, NULL, NULL, NULL, NULL,
> - NULL, NULL, NULL, &had_props,
> - &props_mod,
> - eb->db, eb->local_abspath,
> - eb->wri_abspath,
> - pool, pool));
> -
> - new_checksum = original_checksum;
> -
> - if (had_props)
> - SVN_ERR(svn_wc__db_external_read_pristine_props(&base_props, eb->db,
> - eb->local_abspath,
> - eb->wri_abspath,
> - pool, pool));
but we no longer populate base_props...
> - if (props_mod)
> - SVN_ERR(svn_wc__db_external_read_props(&actual_props, eb->db,
> - eb->local_abspath,
> - eb->wri_abspath,
> - pool, pool));
> + SVN_ERR(svn_wc__db_base_get_props(&actual_props, eb->db,
> + eb->local_abspath, pool, pool));
> }
>
> if (!base_props)
> base_props = apr_hash_make(pool);
So base_props is unconditionally an empty hash. Which causes the
segfault when close_file calls svn_wc__merge_props with an empty hash
as the PRISTINE_PROPS argument. svn_wc__merge_props has a prop
deletion, but the empty hash makes it think it has a prop addition:
[[[
svn_error_t *
svn_wc__merge_props(svn_skel_t **work_items,
svn_wc_notify_state_t *state,
apr_hash_t **new_pristine_props,
apr_hash_t **new_actual_props,
svn_wc__db_t *db,
const char *local_abspath,
svn_kind_t kind,
const svn_wc_conflict_version_t *left_version,
const svn_wc_conflict_version_t *right_version,
apr_hash_t *server_baseprops,
apr_hash_t *pristine_props,
apr_hash_t *actual_props,
const apr_array_header_t *propchanges,
svn_boolean_t base_merge,
svn_boolean_t dry_run,
svn_wc_conflict_resolver_func2_t conflict_func,
void *conflict_baton,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
<snip>
if (!server_baseprops)
server_baseprops = pristine_props;
<snip>
/* Looping over the array of incoming propchanges we want to apply: */
iterpool = svn_pool_create(scratch_pool);
for (i = 0; i < propchanges->nelts; i++)
{
<snip>
to_val = incoming_change->value
? svn_string_dup(incoming_change->value, result_pool) : NULL;
from_val = apr_hash_get(server_baseprops, propname, APR_HASH_KEY_STRING);
base_val = apr_hash_get(pristine_props, propname, APR_HASH_KEY_STRING);
<snip>
if (! from_val) /* adding a new property */
SVN_ERR(apply_single_prop_add(state, &conflict_remains,
db, local_abspath,
left_version, right_version,
is_dir, actual_props,
propname, base_val, to_val,
### ^^^^
### This is obviously going to blow up since to_val is null!
conflict_func, conflict_baton,
dry_run, result_pool, iterpool));
]]]
So it seems we need to put
subversion/libsvn_wc/wc_db.c:svn_wc__db_external_read_pristine_props()
back. Does that seem right to you, or is there a better way?
Paul
Re: Crash when updating
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Jan 5, 2012 at 2:37 PM, Stefan Küng <to...@gmail.com> wrote:
> Hi,
>
> From a crash report dump sent for TSVN for an update the attached stack
> trace happened.
>
> libsvn_wc\props.c, svn_wc__merge_props()
> to_val = incoming_change->value
> ? svn_string_dup(incoming_change->value, result_pool) : NULL;
> ....
> if (! from_val) /* adding a new property */
> SVN_ERR(apply_single_prop_add(state, &conflict_remains,
> db, local_abspath,
> left_version, right_version,
> is_dir, actual_props,
> propname, base_val, to_val,
> conflict_func, conflict_baton,
> dry_run, result_pool, iterpool));
> and then in apply_single_prop_add():
> ...
> if (svn_string_compare(working_val, new_val))
> crashes, because new_val is NULL.
>
> the property is "svn:mergeinfo".
Hi Stefan,
I saw 'mergeinfo' so took a quick look (happily it has nothing to do
with mergeinfo per se, any property will do ;-)
> Could this happen if someone manually removes the svn:mergeinfo property and
> then the next update (from another working copy) gets the to_val as NULL?
I'm not sure if this is what you mean, but I see one way to replicate
this problem: Create a file external mapping to a file with some
arbitrary property. Remap the same external to a file without that
property. Boom.
I filed an issue (issue #4093) which gives the full replication
recipe. I also added a regression test in r1228340.
Paul