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

Reply via email to