On 3 June 2015 at 12:39, Philip Martin <philip.mar...@wandisco.com> wrote: > Bert Huijben <b...@qqmail.nl> writes: > >> Strange… this should never be necessarily when *removing* something >> from a hash (value =NULL). >> >> >> I think you see some kind of other problem. > > I agree this bit is unnecessary, I have changed it back. > >> @@ -2494,7 +2494,9 @@ read_dir_entries(apr_array_header_t *ent >> { >> /* We must be in incremental mode */ >> assert(hash); >> - apr_hash_set(hash, entry.key, entry.keylen, NULL); >> + apr_hash_set(hash, >> + apr_pstrmemdup(scratch_pool, entry.key, >> entry.keylen), >> + entry.keylen, NULL); >> continue; >> } >> > > This bit is the bug fix. It fixes some FAILs seen for fs-tests and > valgrind identified memory reads after free. I wrote the fix in the > wrong place first of all and then assumed I needed to fix it in both > places. > >> @@ -2534,7 +2536,9 @@ read_dir_entries(apr_array_header_t *ent >> /* 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_x__dirent_t *) = dirent; >> } > FWIW libsvn_fsfs uses dirent->name as key since it's already copied to result_pool: [[[ /* In incremental mode, update the hash; otherwise, write to the * final array. Be sure to use hash keys that survive this iteration. */ if (incremental) apr_hash_set(hash, dirent->name, entry.keylen, dirent); else APR_ARRAY_PUSH(entries, svn_fs_dirent_t *) = dirent; ]]]
-- Ivan Zhakov CTO | VisualSVN | http://www.visualsvn.com