RE: Invalid memory read in FSFS

I would guess that the actual keys are one byte longer than the length passed to the hash function: a final '\0' to make them a c string.

Does using a strdup instead of that memdup fix the problem? (Or memdup with +1)

Bert

-----Original Message-----
From: "Philip Martin" <philip.martin_at_wandisco.com>
Sent: ‎26-‎2-‎2014 10:57
To: "Philip Martin" <philip_at_codematters.co.uk>
Cc: "dev_at_subversion.apache.org" <dev_at_subversion.apache.org>; "Stefan Fuhrmann" <stefan.fuhrmann_at_wandisco.com>
Subject: Re: Invalid memory read in FSFS

Philip Martin <philip_at_codematters.co.uk> writes:

> Philip Martin <philip.martin_at_wandisco.com> writes:
>
>> I'm seeing test failures when building Subversion and APR with pool
>> debugging:
>>
>> FAIL: lt-fs-fs-pack-test 13: upgrade txns to log addressing in shared FSFS
>> FAIL: lt-fs-fs-pack-test 14: upgrade txns started before svnadmin upgrade
>> FAIL: lt-fs-test 33: test dir prop preservation in unordered txns
>> FAIL: lt-fs-test 43: test merge directory properties
>>
>> Valgrind reports are typically:
>>
>> $ valgrind -q .libs/lt-fs-test --cleanup 33
>> ==32655== Invalid read of size 1
>> ==32655== at 0x402EA79: bcmp (mc_replace_strmem.c:935)
>> ==32655== by 0x41F6E72: find_entry (apr_hash.c:280)
>> ==32655== by 0x41F719D: apr_hash_set (apr_hash.c:357)
>> ==32655== by 0x50430CB: read_dir_entries (cached_data.c:2053)
>> ==32655== by 0x50432A4: get_dir_contents (cached_data.c:2098)
>> ==32655== by 0x5043640: svn_fs_fs__rep_contents_dir (cached_data.c:2195)
>> ==32655== by 0x50714B3: write_final_rev (transaction.c:2897)
>> ==32655== by 0x5074113: commit_body (transaction.c:3839)
>> ==32655== by 0x504A898: with_some_lock_file (fs_fs.c:185)
>> ==32655== by 0x504A970: svn_fs_fs__with_write_lock (fs_fs.c:203)
>> ==32655== by 0x5074BB1: svn_fs_fs__commit (transaction.c:4009)
>> ==32655== by 0x507983F: svn_fs_fs__commit_txn (tree.c:2228)
>> ==32655== Address 0x870d120 is 0 bytes inside a block of size 2 free'd
>> ==32655== at 0x402AF4C: free (vg_replace_malloc.c:468)
>> ==32655== by 0x42021F9: pool_clear_debug (apr_pools.c:1576)
>> ==32655== by 0x42022D9: apr_pool_clear_debug (apr_pools.c:1613)
>> ==32655== by 0x5042DCC: read_dir_entries (cached_data.c:1993)
>> ==32655== by 0x50432A4: get_dir_contents (cached_data.c:2098)
>> ==32655== by 0x5043640: svn_fs_fs__rep_contents_dir (cached_data.c:2195)
>> ==32655== by 0x50714B3: write_final_rev (transaction.c:2897)
>> ==32655== by 0x5074113: commit_body (transaction.c:3839)
>> ==32655== by 0x504A898: with_some_lock_file (fs_fs.c:185)
>> ==32655== by 0x504A970: svn_fs_fs__with_write_lock (fs_fs.c:203)
>> ==32655== by 0x5074BB1: svn_fs_fs__commit (transaction.c:4009)
>> ==32655== by 0x507983F: svn_fs_fs__commit_txn (tree.c:2228)
>>
>> I think this was caused by r1554711, switch directory rep to array
>> order, but I haven't identified the exact problem yet.
>
> I think it might be:
>
> Index: sw/subversion/src/subversion/libsvn_fs_fs/cached_data.c
> ===================================================================
> --- sw/subversion/src/subversion/libsvn_fs_fs/cached_data.c (revision 1571876)
> +++ sw/subversion/src/subversion/libsvn_fs_fs/cached_data.c (working copy)
> @@ -2050,7 +2050,10 @@
> /* In incremental mode, update the hash; otherwise, write to the
> * final array. */
> if (incremental)
> - apr_hash_set(hash, entry.key, entry.keylen, dirent);
> + apr_hash_set(hash,
> + apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
> + entry.keylen,
> + dirent);
> else
> APR_ARRAY_PUSH(entries, svn_fs_dirent_t *) = dirent;
> }
>
> that removes the above valgrind warning but now I see:
>
> $ valgrind -q .libs/lt-fs-test --cleanup 33
> ==26848== Invalid read of size 8
> ==26848== at 0x50513AE: svn_fs_fs__id_txn_used (id.c:151)
> ==26848== by 0x505CB34: svn_fs_fs__unparse_representation (low_level.c:890)
> ==26848== by 0x505CE8A: svn_fs_fs__write_noderev (low_level.c:939)
> ==26848== by 0x506B76A: svn_fs_fs__put_node_revision (transaction.c:499)
> ==26848== by 0x50497AC: svn_fs_fs__dag_update_ancestry (dag.c:1346)
> ==26848== by 0x5079474: merge (tree.c:2070)
> ==26848== by 0x50791B4: merge (tree.c:2021)
> ==26848== by 0x50791B4: merge (tree.c:2021)
> ==26848== by 0x507966A: merge_changes (tree.c:2121)
> ==26848== by 0x5079800: svn_fs_fs__commit_txn (tree.c:2218)
> ==26848== by 0x4048248: svn_fs_commit_txn (fs-loader.c:830)
> ==26848== by 0x402CAB: test_commit_txn (fs-test.c:81)
> ==26848== Address 0x7775668 is 72 bytes inside a block of size 112 free'd
> ==26848== at 0x402AF4C: free (vg_replace_malloc.c:468)
> ==26848== by 0x42021F9: pool_clear_debug (apr_pools.c:1576)
> ==26848== by 0x42022D9: apr_pool_clear_debug (apr_pools.c:1613)
> ==26848== by 0x5078B3A: merge (tree.c:1916)
> ==26848== by 0x50791B4: merge (tree.c:2021)
> ==26848== by 0x50791B4: merge (tree.c:2021)
> ==26848== by 0x507966A: merge_changes (tree.c:2121)
> ==26848== by 0x5079800: svn_fs_fs__commit_txn (tree.c:2218)
> ==26848== by 0x4048248: svn_fs_commit_txn (fs-loader.c:830)
> ==26848== by 0x402CAB: test_commit_txn (fs-test.c:81)
> ==26848== by 0x418544: unordered_txn_dirprops (fs-test.c:4655)
> ==26848== by 0x403BD93: do_test_num (svn_test_main.c:400)

I can fix this second warning with this:

Index: subversion/libsvn_fs_fs/tree.c
===================================================================
--- subversion/libsvn_fs_fs/tree.c (revision 1571978)
+++ subversion/libsvn_fs_fs/tree.c (working copy)
@@ -1957,7 +1957,7 @@ merge(svn_stringbuf_t *conflict_p,
                                                s_entry->id,
                                                s_entry->kind,
                                                txn_id,
- iterpool));
+ pool));
             }
           else
             {

Not sure whether either of these fixes is correct.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*