@matheusbsbs / matheus-bagliano / commits / 3d20ed27b8

parallel-checkout: send the new object_id algo field to the workers

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Matheus Tavares committed May 17, 2021 at 16:49 UTC 3d20ed27b8bdac5c82f4f78af802f9afa499f651

2 files changed +22 -7

hash.h

+16

@@ -263,6 +263,22 @@ static inline void oidcpy(struct object_id *dst, const struct object_id *src)

263 dst->algo = src->algo;

264 }

265

266 +/* Like oidcpy() but zero-pads the unused bytes in dst's hash array. */

267 +static inline void oidcpy_with_padding(struct object_id *dst,

268 + struct object_id *src)

269 +{

270 + size_t hashsz;

271 +

272 + if (!src->algo)

273 + hashsz = the_hash_algo->rawsz;

274 + else

275 + hashsz = hash_algos[src->algo].rawsz;

276 +

277 + memcpy(dst->hash, src->hash, hashsz);

278 + memset(dst->hash + hashsz, 0, GIT_MAX_RAWSZ - hashsz);

279 + dst->algo = src->algo;

280 +}

281 +

282 static inline struct object_id *oiddup(const struct object_id *src)

283 {

284 struct object_id *dst = xmalloc(sizeof(struct object_id));

parallel-checkout.c

+6 -7

@@ -411,7 +411,7 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)

411 len_data = sizeof(struct pc_item_fixed_portion) + name_len +

412 working_tree_encoding_len;

413

414 - data = xcalloc(1, len_data);

414 + data = xmalloc(len_data);

415

416 fixed_portion = (struct pc_item_fixed_portion *)data;

417 fixed_portion->id = pc_item->id;

@@ -421,13 +421,12 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)

421 fixed_portion->name_len = name_len;

422 fixed_portion->working_tree_encoding_len = working_tree_encoding_len;

423 /*

424 - * We use hashcpy() instead of oidcpy() because the hash[] positions

425 - * after `the_hash_algo->rawsz` might not be initialized. And Valgrind

426 - * would complain about passing uninitialized bytes to a syscall

427 - * (write(2)). There is no real harm in this case, but the warning could

428 - * hinder the detection of actual errors.

424 + * We pad the unused bytes in the hash array because, otherwise,

425 + * Valgrind would complain about passing uninitialized bytes to a

426 + * write() syscall. The warning doesn't represent any real risk here,

427 + * but it could hinder the detection of actual errors.

428 */

430 - hashcpy(fixed_portion->oid.hash, pc_item->ce->oid.hash);

429 + oidcpy_with_padding(&fixed_portion->oid, &pc_item->ce->oid);

430

431 variant = data + sizeof(*fixed_portion);

432 if (working_tree_encoding_len) {