On Wed, Oct 23, 2013 at 1:29 PM, Philip Martin
<philip.mar...@wandisco.com>wrote:
> fs-test 14 gives a valgrind warning:
>
> ==6748== Invalid read of size 8
> ==6748== at 0x587B590: verify_moves (transaction.c:3235)
> ==6748== by 0x587BC03: commit_body (transaction.c:3397)
> ==6748== by 0x5854151: with_some_lock_file (fs_fs.c:184)
> ==6748== by 0x5854222: svn_fs_fs__with_write_lock (fs_fs.c:202)
> ==6748== by 0x587CAEC: svn_fs_fs__commit (transaction.c:3626)
> ==6748== by 0x5881217: svn_fs_fs__commit_txn (tree.c:2114)
> ==6748== by 0x403531A: svn_fs_commit_txn2 (fs-loader.c:822)
> ==6748== by 0x403543E: svn_fs_commit_txn (fs-loader.c:859)
> ==6748== by 0x40D415: delete (fs-test.c:2662)
> ==6748== by 0x402A0FE: do_test_num (svn_test_main.c:273)
> ==6748== by 0x402AC39: main (svn_test_main.c:577)
> ==6748== Address 0x6e672c8 is 8 bytes after a block of size 48 alloc'd
> ==6748== at 0x4C28BED: malloc (vg_replace_malloc.c:263)
> ==6748== by 0x5079DDB: pool_alloc (apr_pools.c:1463)
> ==6748== by 0x5079F57: apr_palloc_debug (apr_pools.c:1504)
> ==6748== by 0x506DA1E: apr_pmemdup (apr_strings.c:119)
> ==6748== by 0x5874D56: fold_change (transaction.c:752)
> ==6748== by 0x5874E55: process_changes (transaction.c:789)
> ==6748== by 0x58750AF: svn_fs_fs__txn_changes_fetch (transaction.c:870)
> ==6748== by 0x587BB9B: commit_body (transaction.c:3391)
> ==6748== by 0x5854151: with_some_lock_file (fs_fs.c:184)
> ==6748== by 0x5854222: svn_fs_fs__with_write_lock (fs_fs.c:202)
> ==6748== by 0x587CAEC: svn_fs_fs__commit (transaction.c:3626)
> ==6748== by 0x5881217: svn_fs_fs__commit_txn (tree.c:2114)
>
> In fold_changes a key-value pair is added to the hash where the value is
> of type svn_fs_path_change2_t. In verify_moves the value is retrieved
> as type change_t.
>
> We could convert from one to other as below, but it might be better to
> get fold_changes to store a change_t or have the rest of verify_moves
> accept a svn_fs_path_change2_t.
>
> Index: subversion/libsvn_fs_fs/transaction.c
> ===================================================================
> --- subversion/libsvn_fs_fs/transaction.c (revision 1534698)
> +++ subversion/libsvn_fs_fs/transaction.c (working copy)
> @@ -3227,24 +3227,34 @@ verify_moves(svn_fs_t *fs,
>
> for (hi = apr_hash_first(pool, changed_paths); hi; hi =
> apr_hash_next(hi))
> {
> + const void *key;
> + apr_ssize_t len;
> + void *val;
> const char *path;
> - apr_ssize_t len;
> - change_t *change;
> - apr_hash_this(hi, (const void**)&path, &len, (void**)&change);
> + svn_fs_path_change2_t *info;
>
> - if ( change->info.copyfrom_path
> - && ( change->info.change_kind == svn_fs_path_change_move
> - || change->info.change_kind ==
> svn_fs_path_change_movereplace))
> + apr_hash_this(hi, &key, &len, &val);
> + path = key;
> + info = val;
> +
> + if ( info->copyfrom_path
> + && ( info->change_kind == svn_fs_path_change_move
> + || info->change_kind == svn_fs_path_change_movereplace))
> {
> svn_sort__item_t *item = apr_array_push(moves);
> + change_t *change = apr_palloc(pool, sizeof(change_t));
> +
> + change->path.data = path;
> + change->path.len = len;
> + change->info = *info;
> item->key = path;
> item->klen = len;
> item->value = change;
> }
>
> - if ( change->info.change_kind == svn_fs_path_change_delete
> - || change->info.change_kind == svn_fs_path_change_replace
> - || change->info.change_kind == svn_fs_path_change_movereplace)
> + if ( info->change_kind == svn_fs_path_change_delete
> + || info->change_kind == svn_fs_path_change_replace
> + || info->change_kind == svn_fs_path_change_movereplace)
> APR_ARRAY_PUSH(deletions, const char *) = path;
> }
>
Excellent find! This code has been carried over from /trunk,
but it not being used there atm.
Fixed on /trunk in r1535029.
-- Stefan^2.
