Re: [PATCH v3] index-pack: add ability to disable SHA-1 collision check

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 14, 2018, 12:40 UTC
Message-ID: <87va4z25p5.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqftw4nnj6.fsf@gitster-ct.c.googlers.com>

On Wed, Nov 14 2018, Junio C Hamano wrote:

Show 39 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Add a new core.checkCollisions setting. On by default, it can be set
>> to 'false' to disable the check for existing objects in sha1_object().
>> ...
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
>> index 2004e25da2..4a3508aa9f 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -791,23 +791,24 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
>>  {
>>  	void *new_data = NULL;
>>  	int collision_test_needed = 0;
>> +	int do_coll_check = git_config_get_collision_check();
>>
>>  	assert(data || obj_entry);
>>
>> -	if (startup_info->have_repository) {
>> +	if (do_coll_check && startup_info->have_repository) {
>>  		read_lock();
>>  		collision_test_needed =
>>  			has_sha1_file_with_flags(oid->hash, OBJECT_INFO_QUICK);
>>  		read_unlock();
>>  	}
>>
>> -	if (collision_test_needed && !data) {
>> +	if (do_coll_check && collision_test_needed && !data) {
>>  		read_lock();
>>  		if (!check_collison(obj_entry))
>>  			collision_test_needed = 0;
>>  		read_unlock();
>>  	}
>> -	if (collision_test_needed) {
>> +	if (do_coll_check && collision_test_needed) {
>
> If I am reading the patch correctly, The latter two changes are
> totally unnecessary.  c-t-needed is true only when dO-coll_check
> allowed the initial "do we even have that object?" check to kick in
> and never set otherwise.

You're right. I was trying to do this in a few different ways and didn't simplify this part.

> I am not all that enthused to the idea of sending a wrong message to
> our users, i.e. it is sometimes OK to sacrifice the security of
> collision detection.

I think I've made the case in side-threads that given the performance numbers and the danger of an actual SHA-1 collision this is something other powerusers would be interested in having.

> A small change like this is easy to adjust to apply to the codebase,
> even after today's codebase undergoes extensive modifications; quite
> honestly, I'd prefer not having to worry about it so close to the
> pre-release feature freeze.

Yeah, let's definitely wait with this under 2.20. I sent this out more because I re-rolled it for an internal deployment, and wanted to get some thoughts on what the plan should be for queuing up these two related (no collision detection && loose cache) features.