On Fri, Jun 30, 2017 at 10:39 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> diff --git a/patch-ids.c b/patch-ids.c
>> index 9c0ab9e67a..b9b2ebbad0 100644
>> --- a/patch-ids.c
>> +++ b/patch-ids.c
>> @@ -37,6 +37,7 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,
>> */
>> static int patch_id_cmp(struct patch_id *a,
>> struct patch_id *b,
>> + const void *unused_keydata,
>> struct diff_options *opt)
>> {
>> if (is_null_oid(&a->patch_id) &&
>> @@ -57,7 +58,8 @@ int init_patch_ids(struct patch_ids *ids)
>> ids->diffopts.detect_rename = 0;
>> DIFF_OPT_SET(&ids->diffopts, RECURSIVE);
>> diff_setup_done(&ids->diffopts);
>> - hashmap_init(&ids->patches, (hashmap_cmp_fn)patch_id_cmp, 256);
>> + hashmap_init(&ids->patches, (hashmap_cmp_fn)patch_id_cmp,
>> + &ids->diffopts, 256);
>> return 0;
>> }
>>
>> @@ -93,7 +95,7 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
>> if (init_patch_id_entry(&patch, commit, ids))
>> return NULL;
>>
>> - return hashmap_get(&ids->patches, &patch, &ids->diffopts);
>> + return hashmap_get(&ids->patches, &patch, NULL);
>> }
+cc Peff
>
> This actually makes me wonder if we can demonstrate an existing
> breakage in tests. The old code used to pass NULL to the diffopts,
> causing it to be passed down through commit_patch_id() function down
> to diff_tree_oid() or diff_root_tree_oid(). When the codepath
> triggers the issue that Peff warned about in his old article (was it
> about rehashing or something?) that makes two entries compared
> (i.e. not using keydata, because we are not comparing an existing
> entry with a key and data only to see if that already exists in the
> hashmap), wouldn't that cause ll_diff_tree_oid() that is called from
> diff_tree_oid() to dereference NULL?
>
> Thanks.
I am at a loss here after re-reading your answer over and over,
but I think you are asking if patch_id_cmp can break, as
we have a callchain like
patch_id_cmp
commit_patch_id
(diff_root_tree_oid)
diff_tree_oid
ll_diff_tree_oid
passing diff_options down there, and patch_id_cmp may have
gotten NULL.
The answer is no, it was safe. (by accident?)
That is because we never use hashmap_get_next
on the hashmap that uses patch_id_cmp as the compare
function.
hashmap_get_next is the only function that does not pass
on a keydata, any other has valid caller provided keydata.
Thanks,
Stefan
