{"thread":{"id":"47386","subject":"[RFC] 'unsigned long' to 'size_t' conversion","startedAt":"2017-12-06T15:08:32Z","lastAt":"2017-12-06T21:44:04Z","messageCount":4,"participants":["Derrick Stolee","Thomas Braun","Brandon Williams","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"334246","messageId":"a660460d-b294-5113-bfaf-d98bcf99bad5@gmail.com","threadId":"47386","inReplyTo":null,"subject":"[RFC] 'unsigned long' to 'size_t' conversion","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2017-12-06T15:08:23Z","receivedAt":"2017-12-06T15:08:32Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"There are several places in Git where we refer to the size of an object \nby an 'unsigned long' instead of a 'size_t'. In 64-bit Linux, 'unsigned \nlong' is 8 bytes, but in 64-bit Windows it is 4 bytes.\n\nThe main issue with this conversion is that large objects fail to load \n(they seem to hash and store just fine). For example, the following \n'blob8gb' is an 8 GB file where the ith byte is equal to i % 256:\n\n$ git hash-object -w --no-filters blob8gb\n5391939346b98600acc0283dda24649450cec51f\n\n$ git cat-file -s 5391939346b98600acc0283dda24649450cec51f\nerror: bad object header\nfatal: git cat-file: could not get object info\n\nAn existing discussion can be found here: \nhttps://github.com/git-for-windows/git/issues/1063\n\nThe error message results from unpack_object_header_buffer() which had \nits most-recent meaningful change in 'ea4f9685: \nunpack_object_header_buffer(): clear the size field upon error' (in 2011).\n\nIn my opinion, the correct thing to do would be to replace all 'unsigned \nlong's that refer to an object size and replace them with 'size_t'. \nHowever, a simple \"git grep 'unsigned long size'\" reveals 194 results, \nand there are other permutations of names and pointer types all over.\n\nThis conversion would be a significant patch, so I wanted to get the \ncommunity's thoughts on this conversion.\n\nIf there are small, isolated chunks that can be done safely, then this \nmay be a good target for a first patch.\n\nThanks,\n\n-Stolee\n\n"},{"id":"334258","messageId":"0e22c1cf-57df-fe6c-c63f-d8a219e80aa1@virtuell-zuhause.de","threadId":"47386","inReplyTo":"a660460d-b294-5113-bfaf-d98bcf99bad5@gmail.com","subject":"Re: [RFC] 'unsigned long' to 'size_t' conversion","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2017-12-06T16:48:43Z","receivedAt":"2017-12-06T16:48:58Z","isPatch":false,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"Am 06.12.2017 um 16:08 schrieb Derrick Stolee:\n\nHi Derrick,\n\n> If there are small, isolated chunks that can be done safely, then this\n> may be a good target for a first patch.\n\nHere are some pointers to past discussions:\n-\nhttps://public-inbox.org/git/trinity-9f703269-6f73-4f6d-b90b-45e09e1c094c-1489582854278@3capp-gmx-bs66/\n-\nhttps://public-inbox.org/git/1502527643-21944-1-git-send-email-martin@mail.zuhause/\n\nI'm posting this mainly so that we can avoid, if possible, duplicated work.\n\nHope that helps,\nThomas\n"},{"id":"334263","messageId":"20171206181002.GA59217@google.com","threadId":"47386","inReplyTo":"a660460d-b294-5113-bfaf-d98bcf99bad5@gmail.com","subject":"Re: [RFC] 'unsigned long' to 'size_t' conversion","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-06T18:10:02Z","receivedAt":"2017-12-06T18:10:27Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/06, Derrick Stolee wrote:\n> There are several places in Git where we refer to the size of an\n> object by an 'unsigned long' instead of a 'size_t'. In 64-bit Linux,\n> 'unsigned long' is 8 bytes, but in 64-bit Windows it is 4 bytes.\n> \n> The main issue with this conversion is that large objects fail to\n> load (they seem to hash and store just fine). For example, the\n> following 'blob8gb' is an 8 GB file where the ith byte is equal to i\n> % 256:\n> \n> $ git hash-object -w --no-filters blob8gb\n> 5391939346b98600acc0283dda24649450cec51f\n> \n> $ git cat-file -s 5391939346b98600acc0283dda24649450cec51f\n> error: bad object header\n> fatal: git cat-file: could not get object info\n> \n> An existing discussion can be found here:\n> https://github.com/git-for-windows/git/issues/1063\n> \n> The error message results from unpack_object_header_buffer() which\n> had its most-recent meaningful change in 'ea4f9685:\n> unpack_object_header_buffer(): clear the size field upon error' (in\n> 2011).\n> \n> In my opinion, the correct thing to do would be to replace all\n> 'unsigned long's that refer to an object size and replace them with\n> 'size_t'. However, a simple \"git grep 'unsigned long size'\" reveals\n> 194 results, and there are other permutations of names and pointer\n> types all over.\n> \n> This conversion would be a significant patch, so I wanted to get the\n> community's thoughts on this conversion.\n> \n> If there are small, isolated chunks that can be done safely, then\n> this may be a good target for a first patch.\n\nI think that an effort like this would definitely be worthwhile.  Much\nlike the unsigned char[20] -> struct object_id conversion I would think\nthat the best way to go about such a conversion would be to do it in\nsmall chunks as you've mentioned.  That way you are only causing churn\nin hopefully small parts of the code base at a time instead of one\nmonolithic change that is sure to cause conflicts.\n\n-- \nBrandon Williams\n"},{"id":"334274","messageId":"20171206214343.GA19469@sigill.intra.peff.net","threadId":"47386","inReplyTo":"a660460d-b294-5113-bfaf-d98bcf99bad5@gmail.com","subject":"Re: [RFC] 'unsigned long' to 'size_t' conversion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-06T21:43:44Z","receivedAt":"2017-12-06T21:44:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 06, 2017 at 10:08:23AM -0500, Derrick Stolee wrote:\n\n> There are several places in Git where we refer to the size of an object by\n> an 'unsigned long' instead of a 'size_t'. In 64-bit Linux, 'unsigned long'\n> is 8 bytes, but in 64-bit Windows it is 4 bytes.\n> \n> The main issue with this conversion is that large objects fail to load (they\n> seem to hash and store just fine). For example, the following 'blob8gb' is\n> an 8 GB file where the ith byte is equal to i % 256:\n\nYeah, I think there's widespread agreement that this needs fixing. It's\njust big enough that nobody has tackled it. If you're willing, that\nsounds great to me. ;)\n\n> In my opinion, the correct thing to do would be to replace all 'unsigned\n> long's that refer to an object size and replace them with 'size_t'. However,\n> a simple \"git grep 'unsigned long size'\" reveals 194 results, and there are\n> other permutations of names and pointer types all over.\n\nYep. I think it's actually even trickier than that, though. Something\nlike off_t is probably the right type for the abstract concept of an\nobject size, since objects can be larger than the address space (in\nwhich case we'd generally hit streaming code paths when possible).\n\nBut then of course we'd want to use size_t for objects that we've\nactually placed into a buffers. At first glance it might be OK to just\nuse a larger type everywhere, but I think that runs into some funny\nsituations. For instance, you would not want to pass an off_t to\nxmalloc(), as it is truncated (which may lead to a too-small buffer and\nthen a heap-smashing overflow).\n\nSo in an ideal world it is something like:\n\n  - abstract object types are always off_t\n\n  - lower-level code that takes an object in a buffer always uses size_t\n\n  - at the conversion point, we must always use xsize_t() to catch\n    conversion problems.\n\n  - Any time that xsize_t() might die is a place where the caller should\n    probably figure out if it can use a streaming code path for large\n    objects.\n\n> This conversion would be a significant patch, so I wanted to get the\n> community's thoughts on this conversion.\n> \n> If there are small, isolated chunks that can be done safely, then this may\n> be a good target for a first patch.\n\nYes, I agree this probably has to be done incrementally. The threads\nThomas linked might be good starting points. But do be careful with\nincremental changes that you don't introduce any spots where we might\nget a value mismatch that used to be consistently truncated. E.g., this\ncode is wrong but not a security problem:\n\n  size_t full_len = strlen(src); /* ok */\n  unsigned long len = full_len; /* possible truncation! */\n  char *dst = xmalloc(len); /* buffer possibly smaller than full_len */\n  memcpy(dst, src, len); /* copies truncated value */\n\nBut this has a vulnerability:\n\n  memcpy(dst, src, full_len); /* whoops, buffer is too small! */\n\nObviously this is a stupid toy example, but in the real world things\nlike that \"unsigned long\" assignment happen via implicit conversion in\nfunction parameters. Or we never have \"full_len\" in the first place, and\ninstead build it up over a series of operations that write into the\nbuffer. Etc.\n\nI made a pass over the whole code-base a while back to fix any existing\nproblems and introduce helpers to make it harder to get this wrong\n(e.g., by making sure a consistent value is used for allocating and\npopulating a buffer). So I _hope_ that it should be hard to accidentally\nregress any code by switching out types. But it's something to be aware\nof.\n\n-Peff\n"}]}