global: ensure that object IDs are always padded

The `oidcmp()` and `oideq()` functions only compare the prefix length as
specified by the given hash algorithm. This mandates that the object IDs
have a valid hash algorithm set, or otherwise we wouldn't be able to
figure out that prefix. As we do not have a hash algorithm in many
cases, for example when handling null object IDs, this assumption cannot
always be fulfilled. We thus have a fallback in place that instead uses
`the_repository` to derive the hash function. This implicit dependency
is hidden away from callers and can be quite surprising, especially in
contexts where there may be no repository.

In theory, we can adapt those functions to always memcmp(3P) the whole
length of their hash arrays. But there exist a couple of sites where we
populate `struct object_id`s such that only the prefix of its hash that
is actually used by the hash algorithm is populated. The remaining bytes
are left uninitialized. The fact that those bytes are uninitialized also
leads to warnings under Valgrind in some places where we copy those
bytes.

Refactor callsites where we populate object IDs to always initialize all
bytes. This also allows us to get rid of `oidcpy_with_padding()`, for
one because the input is now fully initialized, and because `oidcpy()`
will now always copy the whole hash array.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/hash-ll.h b/hash-ll.h
index dbb96369fc88c67e8781cbff84a54c0186cc0b62..b72f84f4aed97490d253a345b2aa2edc6eb73959 100644 (file)
--- a/hash-ll.h
+++ b/hash-ll.h
@@ -288,6 +288,8 @@ static inline void oidread(struct object_id *oid, const unsigned char *hash,
                           const struct git_hash_algo *algop)
 {
        memcpy(oid->hash, hash, algop->rawsz);
+       if (algop->rawsz < GIT_MAX_RAWSZ)
+               memset(oid->hash + algop->rawsz, 0, GIT_MAX_RAWSZ - algop->rawsz);
        oid->algo = hash_algo_by_ptr(algop);
 }
 

diff --git a/hash.h b/hash.h
index 43623a0c8670c18c0457ceac158170a8ffc1bc58..e43e3d8b5a1e0ba0d905c5cda0660a64dcbaee7d 100644 (file)
--- a/hash.h
+++ b/hash.h
@@ -31,22 +31,6 @@ static inline int is_null_oid(const struct object_id *oid)
        return oideq(oid, null_oid());
 }
 
-/* Like oidcpy() but zero-pads the unused bytes in dst's hash array. */
-static inline void oidcpy_with_padding(struct object_id *dst,
-                                      const 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 int is_empty_blob_oid(const struct object_id *oid)
 {
        return oideq(oid, the_hash_algo->empty_blob);

diff --git a/hex.c b/hex.c
index d42262bdca71a8703ce191393f0217f0c589a75e..bc9e86a978e50482af62b10b15e73c3b88def1df 100644 (file)
--- a/hex.c
+++ b/hex.c
@@ -25,8 +25,12 @@ int get_oid_hex_algop(const char *hex, struct object_id *oid,
                      const struct git_hash_algo *algop)
 {
        int ret = get_hash_hex_algop(hex, oid->hash, algop);
-       if (!ret)
+       if (!ret) {
                oid_set_algo(oid, algop);
+               if (algop->rawsz != GIT_MAX_RAWSZ)
+                       memset(oid->hash + algop->rawsz, 0,
+                              GIT_MAX_RAWSZ - algop->rawsz);
+       }
        return ret;
 }
 

diff --git a/http-push.c b/http-push.c
index 86de238b84fa5b13dd10e6eb145b05cfcc8a7bb8..a97df4a1fb1838d68f7eec278379054ff440eeaa 100644 (file)
--- a/http-push.c
+++ b/http-push.c
@@ -1016,6 +1016,7 @@ static void remote_ls(const char *path, int flags,
 /* extract hex from sharded "xx/x{38}" filename */
 static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)
 {
+       memset(oid->hash, 0, GIT_MAX_RAWSZ);
        oid->algo = hash_algo_by_ptr(the_hash_algo);
 
        if (strlen(path) != the_hash_algo->hexsz + 1)

diff --git a/notes.c b/notes.c
index 3a8da92fb9c5d19b1ae3221730a501855f6164c0..afe2e2882e8df1e36bdd46be0f2d3bd69030d2f7 100644 (file)
--- a/notes.c
+++ b/notes.c
@@ -427,6 +427,8 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
                                         hashsz - prefix_len))
                                goto handle_non_note; /* entry.path is not a SHA1 */
 
+                       memset(object_oid.hash + hashsz, 0, GIT_MAX_RAWSZ - hashsz);
+
                        type = PTR_TYPE_NOTE;
                } else if (path_len == 2) {
                        /* This is potentially an internal node */

diff --git a/object-file.c b/object-file.c
index c161e3e2a52d3b0133d91166bddc1563f839974d..bb97f8a8090bac6b096e13be07110eb5adaad0a7 100644 (file)
--- a/object-file.c
+++ b/object-file.c
@@ -2743,6 +2743,8 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
                    !hex_to_bytes(oid.hash + 1, de->d_name,
                                  the_hash_algo->rawsz - 1)) {
                        oid_set_algo(&oid, the_hash_algo);
+                       memset(oid.hash + the_hash_algo->rawsz, 0,
+                              GIT_MAX_RAWSZ - the_hash_algo->rawsz);
                        if (obj_cb) {
                                r = obj_cb(&oid, path->buf, data);
                                if (r)

diff --git a/oidtree.c b/oidtree.c
index daef175dc71d6c7d55fc89ef1e286334afbe00b7..92d03b52dbe13f1b84a35d89356b0d5d83d3fbbe 100644 (file)
--- a/oidtree.c
+++ b/oidtree.c
@@ -42,7 +42,7 @@ void oidtree_insert(struct oidtree *ot, const struct object_id *oid)
         * Clear the padding and copy the result in separate steps to
         * respect the 4-byte alignment needed by struct object_id.
         */
-       oidcpy_with_padding(&k, oid);
+       oidcpy(&k, oid);
        memcpy(on->k, &k, sizeof(k));
 
        /*
@@ -60,7 +60,7 @@ int oidtree_contains(struct oidtree *ot, const struct object_id *oid)
        struct object_id k;
        size_t klen = sizeof(k);
 
-       oidcpy_with_padding(&k, oid);
+       oidcpy(&k, oid);
 
        if (oid->algo == GIT_HASH_UNKNOWN)
                klen -= sizeof(oid->algo);

diff --git a/parallel-checkout.c b/parallel-checkout.c
index b5a714c7111bda0d179077ef1614bbe5ce1570b5..08b960aac84b0dc327de0a9e6ab2ffce8b195a55 100644 (file)
--- a/parallel-checkout.c
+++ b/parallel-checkout.c
@@ -429,13 +429,7 @@ static void send_one_item(int fd, struct parallel_checkout_item *pc_item)
        fixed_portion->ident = pc_item->ca.ident;
        fixed_portion->name_len = name_len;
        fixed_portion->working_tree_encoding_len = working_tree_encoding_len;
-       /*
-        * 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.
-        */
-       oidcpy_with_padding(&fixed_portion->oid, &pc_item->ce->oid);
+       oidcpy(&fixed_portion->oid, &pc_item->ce->oid);
 
        variant = data + sizeof(*fixed_portion);
        if (working_tree_encoding_len) {