[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Crash when updating

From: Paul Burba <ptburba_at_gmail.com>
Date: Tue, 10 Jan 2012 11:36:07 -0500

On Mon, Jan 9, 2012 at 10:16 AM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Thu, Jan 5, 2012 at 2:37 PM, Stefan Küng <tortoisesvn_at_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
Received on 2012-01-10 17:36:41 CET

This is an archived mail posted to the Subversion Dev mailing list.