svn-3605

Version:

1.6.6 (patched on 1.6.10)

Bug Link:

http://subversion.tigris.org/issues/show_bug.cgi?id=3605

http://sharpsvn.open.collab.net/ds/viewMessage.do?dsForumId=728&dsMessageId=357283

Patch Link:

source code patch:

see the difference between 1.6.6 and 1.6.10 in ‘libsvn_wc/status.c’

(patch on trunk: http://svn.apache.org/viewvc?diff_format=l&view=revision&revision=879344)

Symptom:

When getting a status of the repository, it could trigger ‘access violation’ error. This bug was revealed when you use the core svn module as the third party in other applications. The log message is generated in “.net” framework.

The error message:

System.AccessViolati​onException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

   at svn_client_status4(Int32* , SByte* , svn_opt_revision_t* , IntPtr , Void* , svn_depth_t , Int32 , Int32 , Int32 , Int32 , apr_array_header_t* , svn_client_ctx_t* , apr_pool_t* )

   at SharpSvn.SvnClient.S​tatus(String path, SvnStatusArgs args, EventHandler`1 statusHandler)

   at SharpSvn.SvnClient.G​etStatus(String path, Collection`1& statuses)

Error location:

Finally, ‘svn_client_status4’ call goes into ‘is_child’ function and it crashes.

- subversion/libsvn_subr/dirent_uri.c:545

static const char *

/* ‘path1’ is the filename for a StyleCop.Cache file, which is ‘unversioned’. ‘path2’ is a bad pointer. */

is_child(path_type_t type, const char *path1, const char *path2,

         apr_pool_t *pool)

{

  …

/* access violation ! */

  for (i = 0; path1[i] && path2[i]; i++)

    if (path1[i] != path2[i])

      return NULL;

  ...

}

How it is diagnosed:

discussion & stack trace

Root Cause:

Brief:

Simple pool life time issue. the variable ‘pool’ has a short life-time, so it leads to dangling pointer. Finally, it introduces access violation.(seg fault).

This problem is only seen if it has externals definitions on specific sub-directory and then continue walking the rest of the tree.

Detail:

Stack trace:

  SharpSvn.dll!is_chil​d(path2, ...) Line 544 + 0x8 bytes C

  SharpSvn.dll!svn_uri​_is_child(url2, ...) Line 982 + 0x14 bytes C

  SharpSvn.dll!svn_path_is_child(path2, …)

  SharpSvn.dll!is_external_path(...)

/* Compare PATH with items in the EXTERNALS hash to see if PATH is the

   drop location for, or an intermediate directory of the drop

   location for, an externals definition.  Use POOL for

   scratchwork. */

/* traverse the EXTERNALS hash to find the proper path, and pool is used for temporary space*/

static svn_boolean_t

is_external_path(apr_hash_t *externals,

                 const char *path,

                 apr_pool_t *pool)

{

  apr_hash_index_t *hi;

  /* First try: does the path exist as a key in the hash? */

  if (apr_hash_get(externals, path, APR_HASH_KEY_STRING))

    return TRUE;

  /* Failing that, we need to check if any external is a child of

     PATH. */

  for (hi = apr_hash_first(pool, externals); hi; hi = apr_hash_next(hi))

    {

      const void *key;

      apr_hash_this(hi, &key, NULL, NULL);

     /* ‘key’ variable is passed to ‘svn_path_is_child’ and it propagates to the bad pointer ‘path2’ */

     /* That is, ‘key’ has a invalid value. We can infer that ‘externals’ or ‘pool’ is corrupted. */

      if (svn_path_is_child(path, key, pool))

        return TRUE;

    }

  return FALSE;

}

  SharpSvn.dll!send_un​versioned_item(...) Line 705 + 0x40 bytes C

/* ‘send_unversioned_item’ send a status structure and it is called only when the file is unversioned. the argument ‘externals’ is a hash of known externals definitions for this status run.*/

static svn_error_t *

send_unversioned_item(apr_hash_t *externals, ...)

{

  … ...

/* get the path */

    const char *path = svn_path_join(svn_wc_adm_access_path(adm_access),

                                   name, pool);

/* see if the path is from external repository or not. */

  svn_boolean_t is_external = is_external_path(externals, path, pool);

  … …

/* when there exists ‘svn:externals’, it calls ‘svn_url_is_child’ function */

  if (no_ignore || (! ignore_me) || is_external || status->repos_lock)

    return (status_func)(status_baton, path, status, pool);

  return SVN_NO_ERROR;

}

the remaining stack strace is:

/* in this function, the patch is applied. ‘eb->externals’ for storing externals definitions is replaced for long-life. */

  SharpSvn.dll!get_dir​_status(...) Line 1036 + 0x28 bytes C

  SharpSvn.dll!handle_​dir_entry(...) Line 798 + 0x27 bytes C

  SharpSvn.dll!get_dir​_status(...) Line 1107 + 0x52 bytes C

  SharpSvn.dll!handle_​dir_entry(...) Line 798 + 0x27 bytes C

  SharpSvn.dll!get_dir​_status(...) Line 1107 + 0x52 bytes C

  SharpSvn.dll!close_edit(...) Line 2146 + 0x22 bytes C

  SharpSvn.dll!close_edit(...) Line 313 + 0xc bytes C

/* the exception handled location from “.net” framework */

  SharpSvn.dll!svn_cli​ent_status4(...) Line 369 + 0xa bytes C

The patch is:

static svn_error_t * get_dir_status(...)

{

   … ...

  /* If "this dir" has "svn:externals" property set on it, store the

     name and value in traversal_info, along with this directory's depth.

     (Also, we want to track the externals internally so we can report

     status more accurately.) */

    {

      … …

      if (prop_val)

        {

          apr_array_header_t *ext_items;

/* get the appropriate ‘pool’ for ‘eb->externals’. it prevents dangling pointers */

+           apr_pool_t *hash_pool = apr_hash_pool_get(eb->externals);

          int i;

          if (eb->traversal_info)

            {

              apr_pool_t *dup_pool = eb->traversal_info->pool;

              const char *dup_path = apr_pstrdup(dup_pool, path);

              const char *dup_val = apr_pstrmemdup(dup_pool, prop_val->data,

                                                   prop_val->len);

              /* First things first -- we put the externals information

                 into the "global" traversal info structure. */

              apr_hash_set(eb->traversal_info->externals_old,

                           dup_path, APR_HASH_KEY_STRING, dup_val);

              apr_hash_set(eb->traversal_info->externals_new,

                           dup_path, APR_HASH_KEY_STRING, dup_val);

              apr_hash_set(eb->traversal_info->depths,

                           dup_path, APR_HASH_KEY_STRING,

                           svn_depth_to_word(dir_entry->depth));

            }

          /* Now, parse the thing, and copy the parsed results into

             our "global" externals hash. */

          SVN_ERR(svn_wc_parse_externals_description3(&ext_items, path,

                                                      prop_val->data, FALSE,

                                                      pool));

          for (i = 0; ext_items && i < ext_items->nelts; i++)

            {

              svn_wc_external_item2_t *item;

              item = APR_ARRAY_IDX(ext_items, i, svn_wc_external_item2_t *);

              apr_hash_set(eb->externals, svn_path_join(path,

                                                        item->target_dir,

                                                        pool),

/* use the appropriate ‘hash_pool’ of ‘eb->externals’. it prevents dangling pointers */

+                                                        hash_pool),

                           APR_HASH_KEY_STRING, item);

            }

        }

    }

    … ...

  for (hi = apr_hash_first(subpool, dirents); hi; hi = apr_hash_next(hi))

    {

      … …

/* Before the patch, this call leaded to segmentation fault. Now, ‘eb->externals’ is safe variable because it is using long-life memory pool. */

      SVN_ERR(send_unversioned_item(key, dirent_p->kind, dirent_p->special,

                                    adm_access,

                                    patterns, eb->externals, no_ignore,

                                    eb->repos_locks, eb->repos_root,

                                    status_func, status_baton, iterpool));

     … ...

    }

  ...

}

Failure symptom category

crash

Is there any log message?

Yes

How can we automatically insert the log message?

signal handler