{"thread":{"id":"55692","subject":"[RFC PATCH] parallel-checkout: send the new object_id algo field to the workers","startedAt":"2021-05-14T14:36:09Z","lastAt":"2021-05-17T19:49:13Z","messageCount":5,"participants":["Matheus Tavares","brian m. carlson","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"424585","messageId":"a4225bc79d963c5a411105e2b75f9ca4b80de835.1621000916.git.matheus.bernardino@usp.br","threadId":"55692","inReplyTo":null,"subject":"[RFC PATCH] parallel-checkout: send the new object_id algo field to the workers","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-14T14:36:00Z","receivedAt":"2021-05-14T14:36:09Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"An object_id storing a SHA-1 name has some unused bytes at the end of\nthe hash array. Since these bytes are not used, they are usually not\ninitialized to any value either. However, at\nparallel_checkout.c:send_one_item() the object_id of a cache entry is\ncopied into a buffer which is later sent to a checkout worker through a\npipe write(). This makes Valgrind complain about passing uninitialized\nbytes to a syscall. The worker won't use these uninitialized bytes\neither, but the warning could confuse someone trying to debug this code;\nSo instead of using oidcpy(), send_one_item() uses hashcpy() to only\ncopy the used/initialized bytes of the object_id, and leave the\nremaining part with zeros.\n\nHowever, since cf0983213c (\"hash: add an algo member to struct\nobject_id\", 2021-04-26), using hashcpy() is no longer sufficient here as\nit won't copy the new algo field from the object_id. Let's add and use a\nnew function which meets both our requirements of copying all the\nimportant object_id data while still avoiding the uninitialized bytes,\nby padding the end of the hash array in the destination object_id. With\nthis change, we also no longer need the destination buffer from\nsend_one_item() to be initialized with zeros, so let's switch from\nxcalloc() to xmalloc() to make this clear.\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n\nHi, brian\n\nI've read the hash transition plan, but I'm not confident to say that I\nfully understand it yet, so maybe this patch is not exactly what we need\nhere. Mainly, I'm not sure I understand in which cases we will have an\nobject_id.algo that is not the_hash_algo. Is it for the early transition\nphases, where we have a SHA-256 repo that accepts user input as SHA-1? \n\nAlso, the object_id's copied here at send_one_item() always come from a\n`struct cache_entry`. In this case, can they still have different\n`algo`s or do we expect them to be the_hash_algo?\n\nThanks!\n\n hash.h              | 16 ++++++++++++++++\n parallel-checkout.c | 13 ++++++-------\n 2 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/hash.h b/hash.h\nindex 2986f991c6..9c6df4d952 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -263,6 +263,22 @@ static inline void oidcpy(struct object_id *dst, const struct object_id *src)\n \tdst->algo = src->algo;\n }\n \n+/* Like oidcpy() but zero-pads the unused bytes in dst's hash array. */\n+static inline void oidcpy_with_padding(struct object_id *dst,\n+\t\t\t\t       struct object_id *src)\n+{\n+\tsize_t hashsz;\n+\n+\tif (!src->algo)\n+\t\thashsz = the_hash_algo->rawsz;\n+\telse\n+\t\thashsz = hash_algos[src->algo].rawsz;\n+\n+\tmemcpy(dst->hash, src->hash, hashsz);\n+\tmemset(dst->hash + hashsz, 0, GIT_MAX_RAWSZ - hashsz);\n+\tdst->algo = src->algo;\n+}\n+\n static inline struct object_id *oiddup(const struct object_id *src)\n {\n \tstruct object_id *dst = xmalloc(sizeof(struct object_id));\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 09e8b10a35..684cbb9ab4 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -393,7 +393,7 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)\n \tlen_data = sizeof(struct pc_item_fixed_portion) + name_len +\n \t\t   working_tree_encoding_len;\n \n-\tdata = xcalloc(1, len_data);\n+\tdata = xmalloc(len_data);\n \n \tfixed_portion = (struct pc_item_fixed_portion *)data;\n \tfixed_portion->id = pc_item->id;\n@@ -403,13 +403,12 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)\n \tfixed_portion->name_len = name_len;\n \tfixed_portion->working_tree_encoding_len = working_tree_encoding_len;\n \t/*\n-\t * We use hashcpy() instead of oidcpy() because the hash[] positions\n-\t * after `the_hash_algo->rawsz` might not be initialized. And Valgrind\n-\t * would complain about passing uninitialized bytes to a syscall\n-\t * (write(2)). There is no real harm in this case, but the warning could\n-\t * hinder the detection of actual errors.\n+\t * We pad the unused bytes in the hash array because, otherwise,\n+\t * Valgrind would complain about passing uninitialized bytes to a\n+\t * write() syscall. The warning doesn't represent any real risk here,\n+\t * but it could hinder the detection of actual errors.\n \t */\n-\thashcpy(fixed_portion->oid.hash, pc_item->ce->oid.hash);\n+\toidcpy_with_padding(&fixed_portion->oid, &pc_item->ce->oid);\n \n \tvariant = data + sizeof(*fixed_portion);\n \tif (working_tree_encoding_len) {\n-- \n2.30.1\n\n"},{"id":"424613","messageId":"YJ7VI5kHAAk126YJ@camp.crustytoothpaste.net","threadId":"55692","inReplyTo":"a4225bc79d963c5a411105e2b75f9ca4b80de835.1621000916.git.matheus.bernardino@usp.br","subject":"Re: [RFC PATCH] parallel-checkout: send the new object_id algo field to the workers","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-05-14T19:53:07Z","receivedAt":"2021-05-14T19:53:13Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-05-14 at 14:36:00, Matheus Tavares wrote:\n> An object_id storing a SHA-1 name has some unused bytes at the end of\n> the hash array. Since these bytes are not used, they are usually not\n> initialized to any value either. However, at\n> parallel_checkout.c:send_one_item() the object_id of a cache entry is\n> copied into a buffer which is later sent to a checkout worker through a\n> pipe write(). This makes Valgrind complain about passing uninitialized\n> bytes to a syscall. The worker won't use these uninitialized bytes\n> either, but the warning could confuse someone trying to debug this code;\n> So instead of using oidcpy(), send_one_item() uses hashcpy() to only\n> copy the used/initialized bytes of the object_id, and leave the\n> remaining part with zeros.\n> \n> However, since cf0983213c (\"hash: add an algo member to struct\n> object_id\", 2021-04-26), using hashcpy() is no longer sufficient here as\n> it won't copy the new algo field from the object_id. Let's add and use a\n> new function which meets both our requirements of copying all the\n> important object_id data while still avoiding the uninitialized bytes,\n> by padding the end of the hash array in the destination object_id. With\n> this change, we also no longer need the destination buffer from\n> send_one_item() to be initialized with zeros, so let's switch from\n> xcalloc() to xmalloc() to make this clear.\n> \n> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n> ---\n> \n> Hi, brian\n> \n> I've read the hash transition plan, but I'm not confident to say that I\n> fully understand it yet, so maybe this patch is not exactly what we need\n> here. Mainly, I'm not sure I understand in which cases we will have an\n> object_id.algo that is not the_hash_algo. Is it for the early transition\n> phases, where we have a SHA-256 repo that accepts user input as SHA-1? \n\nYes, that's correct, as well as for interoperability with remotes using\na different hash algorithm.\n\n> Also, the object_id's copied here at send_one_item() always come from a\n> `struct cache_entry`. In this case, can they still have different\n> `algo`s or do we expect them to be the_hash_algo?\n\nNo, things in the index should always use the same algorithm..\n\nThe patch looks fine to me.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"424766","messageId":"7182880b-0f1e-7b3e-dc6e-b72c8ddd14b3@gmail.com","threadId":"55692","inReplyTo":"YJ7VI5kHAAk126YJ@camp.crustytoothpaste.net","subject":"Re: [RFC PATCH] parallel-checkout: send the new object_id algo field to the workers","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-17T16:54:21Z","receivedAt":"2021-05-17T16:54:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/14/2021 3:53 PM, brian m. carlson wrote:\n> On 2021-05-14 at 14:36:00, Matheus Tavares wrote:>>\n>> I've read the hash transition plan, but I'm not confident to say that I\n>> fully understand it yet, so maybe this patch is not exactly what we need\n>> here. Mainly, I'm not sure I understand in which cases we will have an\n>> object_id.algo that is not the_hash_algo. Is it for the early transition\n>> phases, where we have a SHA-256 repo that accepts user input as SHA-1? \n> \n> Yes, that's correct, as well as for interoperability with remotes using\n> a different hash algorithm.\n> \n>> Also, the object_id's copied here at send_one_item() always come from a\n>> `struct cache_entry`. In this case, can they still have different\n>> `algo`s or do we expect them to be the_hash_algo?\n> \n> No, things in the index should always use the same algorithm..\n> \n> The patch looks fine to me.\n\nChiming in to say I agree that this is a good patch.\n\nThanks,\n-Stolee\n"},{"id":"424776","messageId":"xmqqh7j16trl.fsf@gitster.g","threadId":"55692","inReplyTo":"7182880b-0f1e-7b3e-dc6e-b72c8ddd14b3@gmail.com","subject":"Re: [RFC PATCH] parallel-checkout: send the new object_id algo field to the workers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-17T19:32:30Z","receivedAt":"2021-05-17T19:32:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>> The patch looks fine to me.\n>\n> Chiming in to say I agree that this is a good patch.\n\nMe three?\n\nLet's have the non-RFC final patch.\n"},{"id":"424779","messageId":"4e113550d595925e4f458cdcb63c7d90205b389b.1621280649.git.matheus.bernardino@usp.br","threadId":"55692","inReplyTo":"a4225bc79d963c5a411105e2b75f9ca4b80de835.1621000916.git.matheus.bernardino@usp.br","subject":"[PATCH v2] parallel-checkout: send the new object_id algo field to the workers","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-17T19:49:03Z","receivedAt":"2021-05-17T19:49:13Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"An object_id storing a SHA-1 name has some unused bytes at the end of\nthe hash array. Since these bytes are not used, they are usually not\ninitialized to any value either. However, at\nparallel_checkout.c:send_one_item() the object_id of a cache entry is\ncopied into a buffer which is later sent to a checkout worker through a\npipe write(). This makes Valgrind complain about passing uninitialized\nbytes to a syscall. The worker won't use these uninitialized bytes\neither, but the warning could confuse someone trying to debug this code;\nSo instead of using oidcpy(), send_one_item() uses hashcpy() to only\ncopy the used/initialized bytes of the object_id, and leave the\nremaining part with zeros.\n\nHowever, since cf0983213c (\"hash: add an algo member to struct\nobject_id\", 2021-04-26), using hashcpy() is no longer sufficient here as\nit won't copy the new algo field from the object_id. Let's add and use a\nnew function which meets both our requirements of copying all the\nimportant object_id data while still avoiding the uninitialized bytes,\nby padding the end of the hash array in the destination object_id. With\nthis change, we also no longer need the destination buffer from\nsend_one_item() to be initialized with zeros, so let's switch from\nxcalloc() to xmalloc() to make this clear.\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n\nThere is no change since v1, I'm just dropping the RFC status :)\n\n\n hash.h              | 16 ++++++++++++++++\n parallel-checkout.c | 13 ++++++-------\n 2 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/hash.h b/hash.h\nindex 2986f991c6..9c6df4d952 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -263,6 +263,22 @@ static inline void oidcpy(struct object_id *dst, const struct object_id *src)\n \tdst->algo = src->algo;\n }\n \n+/* Like oidcpy() but zero-pads the unused bytes in dst's hash array. */\n+static inline void oidcpy_with_padding(struct object_id *dst,\n+\t\t\t\t       struct object_id *src)\n+{\n+\tsize_t hashsz;\n+\n+\tif (!src->algo)\n+\t\thashsz = the_hash_algo->rawsz;\n+\telse\n+\t\thashsz = hash_algos[src->algo].rawsz;\n+\n+\tmemcpy(dst->hash, src->hash, hashsz);\n+\tmemset(dst->hash + hashsz, 0, GIT_MAX_RAWSZ - hashsz);\n+\tdst->algo = src->algo;\n+}\n+\n static inline struct object_id *oiddup(const struct object_id *src)\n {\n \tstruct object_id *dst = xmalloc(sizeof(struct object_id));\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 6b1af32bb3..ddc0ff3c06 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -411,7 +411,7 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)\n \tlen_data = sizeof(struct pc_item_fixed_portion) + name_len +\n \t\t   working_tree_encoding_len;\n \n-\tdata = xcalloc(1, len_data);\n+\tdata = xmalloc(len_data);\n \n \tfixed_portion = (struct pc_item_fixed_portion *)data;\n \tfixed_portion->id = pc_item->id;\n@@ -421,13 +421,12 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)\n \tfixed_portion->name_len = name_len;\n \tfixed_portion->working_tree_encoding_len = working_tree_encoding_len;\n \t/*\n-\t * We use hashcpy() instead of oidcpy() because the hash[] positions\n-\t * after `the_hash_algo->rawsz` might not be initialized. And Valgrind\n-\t * would complain about passing uninitialized bytes to a syscall\n-\t * (write(2)). There is no real harm in this case, but the warning could\n-\t * hinder the detection of actual errors.\n+\t * We pad the unused bytes in the hash array because, otherwise,\n+\t * Valgrind would complain about passing uninitialized bytes to a\n+\t * write() syscall. The warning doesn't represent any real risk here,\n+\t * but it could hinder the detection of actual errors.\n \t */\n-\thashcpy(fixed_portion->oid.hash, pc_item->ce->oid.hash);\n+\toidcpy_with_padding(&fixed_portion->oid, &pc_item->ce->oid);\n \n \tvariant = data + sizeof(*fixed_portion);\n \tif (working_tree_encoding_len) {\n-- \n2.30.1\n\n"}]}