{"thread":{"id":"57856","subject":"Re: [PATCH] Prevent git from rehashing 4GBi files","startedAt":"2022-05-06T17:08:28Z","lastAt":"2022-05-06T18:32:21Z","messageCount":2,"participants":["Jason Hatton","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"454925","messageId":"CY4PR16MB1655EE6CC2218AEA35B451E3AFC59@CY4PR16MB1655.namprd16.prod.outlook.com","threadId":"57856","inReplyTo":null,"subject":"Re: [PATCH] Prevent git from rehashing 4GBi files","fromName":"Jason Hatton","fromEmail":"jhatton@globalfinishing.com","sentAt":"2022-05-06T17:08:16Z","receivedAt":"2022-05-06T17:08:28Z","isPatch":true,"sender":{"key":"jhatton@globalfinishing.com","avatar":"https://avatars.githubusercontent.com/u/36747462?v=4"},"body":">Philip Oakley <philipoakley@iee.email> writes:\n>\n>> This \"Munge\" above isn't telling the reader 'why'/'what' is going on.\n>> The comment should in some way highlight that a zero size result is\n>> special, and that we have the roll over issue when the stored in 32 bits\n>> - the double duty of racy vs changed in the stat data heuristic.\n>> Synonyms of 'munge' ?\n\nmangle?\nhash?\n\n>>\n>>\n>>> + */\n>>> +unsigned int munge_st_size(off_t st_size) {\n>>> +    unsigned int sd_size = st_size;\n>>> +\n>>> +    if(!sd_size && st_size)\n>\n>Style.\n\nSomething like 1<<31?\n\n>\n>>> +        return 0x80000000;\n>>> +    else\n>>> +        return sd_size;\n>>> +}\n>\n>This may treat non-zero multiple of 4GiB as \"not racy\", but has\n>anybody double checked the concern Réne brought up earlier that a\n>4GiB file that was added and then got rewritten to 2GiB within the\n>same second would suddenly start getting treated as not racy?\n>\n>The patch (the firnal version of it anyway) needs to be accompanied\n>by a handful of test additions to tickle corner cases like that.\n>\n>Thanks, all, for working on this.\n\nIf the file size is changed by exactly 2GiB is a concern. This is an issue for\nfiles exactly a multiple of 4GiB. However, all files that are changed by a\nmultiple of 4GiB are vulnerable. Say 4GiB + 42 and 8GiB + 42 would appear the\nsame with the current version of git. I'm sure the true fix involves updating\nthe index file format with 64 bit files sizes and an explicit racy flag. I'm\nhopeful the rehashing issue for 4GiB files can be mitigated until than.\n\nI have a question about the coding style. Torsten indicated that there should\nbe an explicit type cast. The original code did not use an explicit type cast,\nso I'm unsure what is going on. One of you experts may have to make the final\npatch. I hope my proof of concept gets the idea across.\n\nThanks\n--\nJason\n\n"},{"id":"454936","messageId":"xmqqbkwaldg4.fsf@gitster.g","threadId":"57856","inReplyTo":"CY4PR16MB1655EE6CC2218AEA35B451E3AFC59@CY4PR16MB1655.namprd16.prod.outlook.com","subject":"Re: [PATCH] Prevent git from rehashing 4GBi files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T18:32:11Z","receivedAt":"2022-05-06T18:32:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jason Hatton <jhatton@globalfinishing.com> writes:\n\n>>Philip Oakley <philipoakley@iee.email> writes:\n>>\n>>> This \"Munge\" above isn't telling the reader 'why'/'what' is going on.\n>>> The comment should in some way highlight that a zero size result is\n>>> special, and that we have the roll over issue when the stored in 32 bits\n>>> - the double duty of racy vs changed in the stat data heuristic.\n>>> Synonyms of 'munge' ?\n>\n> mangle?\n> hash?\n>\n>>>\n>>>\n>>>> + */\n>>>> +unsigned int munge_st_size(off_t st_size) {\n>>>> +    unsigned int sd_size = st_size;\n>>>> +\n>>>> +    if(!sd_size && st_size)\n>>\n>>Style.\n>\n> Something like 1<<31?\n\nSorry, missing SP between \"if\" and \"(\" was what stood out like a\nsore thumb.\n\n>>\n>>>> +        return 0x80000000;\n\nThe .sd_size member is merely defined as \"unsigned int\" and so is\nthe return value from this helper.  They have no idea how big an\ninteger they are dealing with.  It is limited to 32-bit explicitly\nonly because create_from_disk() uses get_be32() on ondisk->size to\nget the value to be assigned to the member.\n\nSo I agree with writing it as 31-bit shift for ease of reading, but\nperhaps a comment to indicate where that size comes from would help\nthe readers while we are at it, perhaps?\n\n\t\treturn 1U<<31; /* ondisk_cache_entry.size */\n\nI dunno.\n\n>>>> +    else\n>>>> +        return sd_size;\n>>>> +}\n>>\n>>This may treat non-zero multiple of 4GiB as \"not racy\", but has\n>>anybody double checked the concern Réne brought up earlier that a\n>>4GiB file that was added and then got rewritten to 2GiB within the\n>>same second would suddenly start getting treated as not racy?\n>>\n>>The patch (the firnal version of it anyway) needs to be accompanied\n>>by a handful of test additions to tickle corner cases like that.\n>>\n>>Thanks, all, for working on this.\n>\n> If the file size is changed by exactly 2GiB is a concern. This is an issue for\n> files exactly a multiple of 4GiB. However, all files that are changed by a\n> multiple of 4GiB are vulnerable.\n\nSo if you have a 4GiB file, \"git add\" it, then rewrite it with a\ndifferent contents to make it a 8GiB file within the same second,\nwould Git mistakenly think that there is no change, because the racy\ngit protection is gone with this change?  I think that was one of\nthe concerns (there may have been others I am forgetting).\n\n"}]}