git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [RFC PATCH] parallel-checkout: send the new object_id algo field to the workers
@ 2021-05-14 14:36 Matheus Tavares
  2021-05-14 19:53 ` brian m. carlson
  2021-05-17 19:49 ` [PATCH v2] " Matheus Tavares
  0 siblings, 2 replies; 5+ messages in thread
From: Matheus Tavares @ 2021-05-14 14:36 UTC (permalink / raw)
  To: sandals; +Cc: git

An object_id storing a SHA-1 name has some unused bytes at the end of
the hash array. Since these bytes are not used, they are usually not
initialized to any value either. However, at
parallel_checkout.c:send_one_item() the object_id of a cache entry is
copied into a buffer which is later sent to a checkout worker through a
pipe write(). This makes Valgrind complain about passing uninitialized
bytes to a syscall. The worker won't use these uninitialized bytes
either, but the warning could confuse someone trying to debug this code;
So instead of using oidcpy(), send_one_item() uses hashcpy() to only
copy the used/initialized bytes of the object_id, and leave the
remaining part with zeros.

However, since cf0983213c ("hash: add an algo member to struct
object_id", 2021-04-26), using hashcpy() is no longer sufficient here as
it won't copy the new algo field from the object_id. Let's add and use a
new function which meets both our requirements of copying all the
important object_id data while still avoiding the uninitialized bytes,
by padding the end of the hash array in the destination object_id. With
this change, we also no longer need the destination buffer from
send_one_item() to be initialized with zeros, so let's switch from
xcalloc() to xmalloc() to make this clear.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---

Hi, brian

I've read the hash transition plan, but I'm not confident to say that I
fully understand it yet, so maybe this patch is not exactly what we need
here. Mainly, I'm not sure I understand in which cases we will have an
object_id.algo that is not the_hash_algo. Is it for the early transition
phases, where we have a SHA-256 repo that accepts user input as SHA-1? 

Also, the object_id's copied here at send_one_item() always come from a
`struct cache_entry`. In this case, can they still have different
`algo`s or do we expect them to be the_hash_algo?

Thanks!

 hash.h              | 16 ++++++++++++++++
 parallel-checkout.c | 13 ++++++-------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/hash.h b/hash.h
index 2986f991c6..9c6df4d952 100644
--- a/hash.h
+++ b/hash.h
@@ -263,6 +263,22 @@ static inline void oidcpy(struct object_id *dst, const struct object_id *src)
 	dst->algo = src->algo;
 }
 
+/* Like oidcpy() but zero-pads the unused bytes in dst's hash array. */
+static inline void oidcpy_with_padding(struct object_id *dst,
+				       struct object_id *src)
+{
+	size_t hashsz;
+
+	if (!src->algo)
+		hashsz = the_hash_algo->rawsz;
+	else
+		hashsz = hash_algos[src->algo].rawsz;
+
+	memcpy(dst->hash, src->hash, hashsz);
+	memset(dst->hash + hashsz, 0, GIT_MAX_RAWSZ - hashsz);
+	dst->algo = src->algo;
+}
+
 static inline struct object_id *oiddup(const struct object_id *src)
 {
 	struct object_id *dst = xmalloc(sizeof(struct object_id));
diff --git a/parallel-checkout.c b/parallel-checkout.c
index 09e8b10a35..684cbb9ab4 100644
--- a/parallel-checkout.c
+++ b/parallel-checkout.c
@@ -393,7 +393,7 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)
 	len_data = sizeof(struct pc_item_fixed_portion) + name_len +
 		   working_tree_encoding_len;
 
-	data = xcalloc(1, len_data);
+	data = xmalloc(len_data);
 
 	fixed_portion = (struct pc_item_fixed_portion *)data;
 	fixed_portion->id = pc_item->id;
@@ -403,13 +403,12 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)
 	fixed_portion->name_len = name_len;
 	fixed_portion->working_tree_encoding_len = working_tree_encoding_len;
 	/*
-	 * We use hashcpy() instead of oidcpy() because the hash[] positions
-	 * after `the_hash_algo->rawsz` might not be initialized. And Valgrind
-	 * would complain about passing uninitialized bytes to a syscall
-	 * (write(2)). There is no real harm in this case, but the warning could
-	 * hinder the detection of actual errors.
+	 * We pad the unused bytes in the hash array because, otherwise,
+	 * Valgrind would complain about passing uninitialized bytes to a
+	 * write() syscall. The warning doesn't represent any real risk here,
+	 * but it could hinder the detection of actual errors.
 	 */
-	hashcpy(fixed_portion->oid.hash, pc_item->ce->oid.hash);
+	oidcpy_with_padding(&fixed_portion->oid, &pc_item->ce->oid);
 
 	variant = data + sizeof(*fixed_portion);
 	if (working_tree_encoding_len) {
-- 
2.30.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] parallel-checkout: send the new object_id algo field to the workers
  2021-05-14 14:36 [RFC PATCH] parallel-checkout: send the new object_id algo field to the workers Matheus Tavares
@ 2021-05-14 19:53 ` brian m. carlson
  2021-05-17 16:54   ` Derrick Stolee
  2021-05-17 19:49 ` [PATCH v2] " Matheus Tavares
  1 sibling, 1 reply; 5+ messages in thread
From: brian m. carlson @ 2021-05-14 19:53 UTC (permalink / raw)
  To: Matheus Tavares; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

On 2021-05-14 at 14:36:00, Matheus Tavares wrote:
> An object_id storing a SHA-1 name has some unused bytes at the end of
> the hash array. Since these bytes are not used, they are usually not
> initialized to any value either. However, at
> parallel_checkout.c:send_one_item() the object_id of a cache entry is
> copied into a buffer which is later sent to a checkout worker through a
> pipe write(). This makes Valgrind complain about passing uninitialized
> bytes to a syscall. The worker won't use these uninitialized bytes
> either, but the warning could confuse someone trying to debug this code;
> So instead of using oidcpy(), send_one_item() uses hashcpy() to only
> copy the used/initialized bytes of the object_id, and leave the
> remaining part with zeros.
> 
> However, since cf0983213c ("hash: add an algo member to struct
> object_id", 2021-04-26), using hashcpy() is no longer sufficient here as
> it won't copy the new algo field from the object_id. Let's add and use a
> new function which meets both our requirements of copying all the
> important object_id data while still avoiding the uninitialized bytes,
> by padding the end of the hash array in the destination object_id. With
> this change, we also no longer need the destination buffer from
> send_one_item() to be initialized with zeros, so let's switch from
> xcalloc() to xmalloc() to make this clear.
> 
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
> 
> Hi, brian
> 
> I've read the hash transition plan, but I'm not confident to say that I
> fully understand it yet, so maybe this patch is not exactly what we need
> here. Mainly, I'm not sure I understand in which cases we will have an
> object_id.algo that is not the_hash_algo. Is it for the early transition
> phases, where we have a SHA-256 repo that accepts user input as SHA-1? 

Yes, that's correct, as well as for interoperability with remotes using
a different hash algorithm.

> Also, the object_id's copied here at send_one_item() always come from a
> `struct cache_entry`. In this case, can they still have different
> `algo`s or do we expect them to be the_hash_algo?

No, things in the index should always use the same algorithm..

The patch looks fine to me.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] parallel-checkout: send the new object_id algo field to the workers
  2021-05-14 19:53 ` brian m. carlson
@ 2021-05-17 16:54   ` Derrick Stolee
  2021-05-17 19:32     ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Derrick Stolee @ 2021-05-17 16:54 UTC (permalink / raw)
  To: brian m. carlson, Matheus Tavares, git

On 5/14/2021 3:53 PM, brian m. carlson wrote:
> On 2021-05-14 at 14:36:00, Matheus Tavares wrote:>>
>> I've read the hash transition plan, but I'm not confident to say that I
>> fully understand it yet, so maybe this patch is not exactly what we need
>> here. Mainly, I'm not sure I understand in which cases we will have an
>> object_id.algo that is not the_hash_algo. Is it for the early transition
>> phases, where we have a SHA-256 repo that accepts user input as SHA-1? 
> 
> Yes, that's correct, as well as for interoperability with remotes using
> a different hash algorithm.
> 
>> Also, the object_id's copied here at send_one_item() always come from a
>> `struct cache_entry`. In this case, can they still have different
>> `algo`s or do we expect them to be the_hash_algo?
> 
> No, things in the index should always use the same algorithm..
> 
> The patch looks fine to me.

Chiming in to say I agree that this is a good patch.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] parallel-checkout: send the new object_id algo field to the workers
  2021-05-17 16:54   ` Derrick Stolee
@ 2021-05-17 19:32     ` Junio C Hamano
  0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2021-05-17 19:32 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: brian m. carlson, Matheus Tavares, git

Derrick Stolee <stolee@gmail.com> writes:

>> The patch looks fine to me.
>
> Chiming in to say I agree that this is a good patch.

Me three?

Let's have the non-RFC final patch.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] parallel-checkout: send the new object_id algo field to the workers
  2021-05-14 14:36 [RFC PATCH] parallel-checkout: send the new object_id algo field to the workers Matheus Tavares
  2021-05-14 19:53 ` brian m. carlson
@ 2021-05-17 19:49 ` Matheus Tavares
  1 sibling, 0 replies; 5+ messages in thread
From: Matheus Tavares @ 2021-05-17 19:49 UTC (permalink / raw)
  To: gitster; +Cc: git, sandals, stolee

An object_id storing a SHA-1 name has some unused bytes at the end of
the hash array. Since these bytes are not used, they are usually not
initialized to any value either. However, at
parallel_checkout.c:send_one_item() the object_id of a cache entry is
copied into a buffer which is later sent to a checkout worker through a
pipe write(). This makes Valgrind complain about passing uninitialized
bytes to a syscall. The worker won't use these uninitialized bytes
either, but the warning could confuse someone trying to debug this code;
So instead of using oidcpy(), send_one_item() uses hashcpy() to only
copy the used/initialized bytes of the object_id, and leave the
remaining part with zeros.

However, since cf0983213c ("hash: add an algo member to struct
object_id", 2021-04-26), using hashcpy() is no longer sufficient here as
it won't copy the new algo field from the object_id. Let's add and use a
new function which meets both our requirements of copying all the
important object_id data while still avoiding the uninitialized bytes,
by padding the end of the hash array in the destination object_id. With
this change, we also no longer need the destination buffer from
send_one_item() to be initialized with zeros, so let's switch from
xcalloc() to xmalloc() to make this clear.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---

There is no change since v1, I'm just dropping the RFC status :)


 hash.h              | 16 ++++++++++++++++
 parallel-checkout.c | 13 ++++++-------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/hash.h b/hash.h
index 2986f991c6..9c6df4d952 100644
--- a/hash.h
+++ b/hash.h
@@ -263,6 +263,22 @@ static inline void oidcpy(struct object_id *dst, const struct object_id *src)
 	dst->algo = src->algo;
 }
 
+/* Like oidcpy() but zero-pads the unused bytes in dst's hash array. */
+static inline void oidcpy_with_padding(struct object_id *dst,
+				       struct object_id *src)
+{
+	size_t hashsz;
+
+	if (!src->algo)
+		hashsz = the_hash_algo->rawsz;
+	else
+		hashsz = hash_algos[src->algo].rawsz;
+
+	memcpy(dst->hash, src->hash, hashsz);
+	memset(dst->hash + hashsz, 0, GIT_MAX_RAWSZ - hashsz);
+	dst->algo = src->algo;
+}
+
 static inline struct object_id *oiddup(const struct object_id *src)
 {
 	struct object_id *dst = xmalloc(sizeof(struct object_id));
diff --git a/parallel-checkout.c b/parallel-checkout.c
index 6b1af32bb3..ddc0ff3c06 100644
--- a/parallel-checkout.c
+++ b/parallel-checkout.c
@@ -411,7 +411,7 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)
 	len_data = sizeof(struct pc_item_fixed_portion) + name_len +
 		   working_tree_encoding_len;
 
-	data = xcalloc(1, len_data);
+	data = xmalloc(len_data);
 
 	fixed_portion = (struct pc_item_fixed_portion *)data;
 	fixed_portion->id = pc_item->id;
@@ -421,13 +421,12 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)
 	fixed_portion->name_len = name_len;
 	fixed_portion->working_tree_encoding_len = working_tree_encoding_len;
 	/*
-	 * We use hashcpy() instead of oidcpy() because the hash[] positions
-	 * after `the_hash_algo->rawsz` might not be initialized. And Valgrind
-	 * would complain about passing uninitialized bytes to a syscall
-	 * (write(2)). There is no real harm in this case, but the warning could
-	 * hinder the detection of actual errors.
+	 * We pad the unused bytes in the hash array because, otherwise,
+	 * Valgrind would complain about passing uninitialized bytes to a
+	 * write() syscall. The warning doesn't represent any real risk here,
+	 * but it could hinder the detection of actual errors.
 	 */
-	hashcpy(fixed_portion->oid.hash, pc_item->ce->oid.hash);
+	oidcpy_with_padding(&fixed_portion->oid, &pc_item->ce->oid);
 
 	variant = data + sizeof(*fixed_portion);
 	if (working_tree_encoding_len) {
-- 
2.30.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-05-17 19:49 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-14 14:36 [RFC PATCH] parallel-checkout: send the new object_id algo field to the workers Matheus Tavares
2021-05-14 19:53 ` brian m. carlson
2021-05-17 16:54   ` Derrick Stolee
2021-05-17 19:32     ` Junio C Hamano
2021-05-17 19:49 ` [PATCH v2] " Matheus Tavares

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git