From 65eb8e0ca79d4a58768f9409b24c48a95d39bc55 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:01 +0200 Subject: [PATCH 01/13] notes: make GET_NIBBLE macro more robust Put parentheses around sha1. Otherwise it could fail for something like GET_NIBBLE(n, (unsigned char *)data); Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notes.c b/notes.c index 8f47c202c5..71cc664b50 100644 --- a/notes.c +++ b/notes.c @@ -64,7 +64,7 @@ struct non_note { #define CLR_PTR_TYPE(ptr) ((void *) ((uintptr_t) (ptr) & ~3)) #define SET_PTR_TYPE(ptr, type) ((void *) ((uintptr_t) (ptr) | (type))) -#define GET_NIBBLE(n, sha1) (((sha1[(n) >> 1]) >> ((~(n) & 0x01) << 2)) & 0x0f) +#define GET_NIBBLE(n, sha1) ((((sha1)[(n) >> 1]) >> ((~(n) & 0x01) << 2)) & 0x0f) #define KEY_INDEX (GIT_SHA1_RAWSZ - 1) #define FANOUT_PATH_SEPARATORS ((GIT_SHA1_HEXSZ / 2) - 1) From d3b0c6bebf3d8e85464b33d58e47a73894d882c7 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:02 +0200 Subject: [PATCH 02/13] load_subtree(): remove unnecessary conditional At this point in the code, len is *always* <= 20. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/notes.c b/notes.c index 71cc664b50..a8b9371736 100644 --- a/notes.c +++ b/notes.c @@ -446,25 +446,24 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, * If object SHA1 is incomplete (len < 20), and current * component consists of 2 hex chars, assume note subtree */ - if (len <= GIT_SHA1_RAWSZ) { - type = PTR_TYPE_NOTE; - l = (struct leaf_node *) - xcalloc(1, sizeof(struct leaf_node)); - oidcpy(&l->key_oid, &object_oid); - oidcpy(&l->val_oid, entry.oid); - if (len < GIT_SHA1_RAWSZ) { - if (!S_ISDIR(entry.mode) || path_len != 2) - goto handle_non_note; /* not subtree */ - l->key_oid.hash[KEY_INDEX] = (unsigned char) len; - type = PTR_TYPE_SUBTREE; - } - if (note_tree_insert(t, node, n, l, type, - combine_notes_concatenate)) - die("Failed to load %s %s into notes tree " - "from %s", - type == PTR_TYPE_NOTE ? "note" : "subtree", - oid_to_hex(&l->key_oid), t->ref); + type = PTR_TYPE_NOTE; + l = (struct leaf_node *) + xcalloc(1, sizeof(struct leaf_node)); + oidcpy(&l->key_oid, &object_oid); + oidcpy(&l->val_oid, entry.oid); + if (len < GIT_SHA1_RAWSZ) { + if (!S_ISDIR(entry.mode) || path_len != 2) + goto handle_non_note; /* not subtree */ + l->key_oid.hash[KEY_INDEX] = (unsigned char) len; + type = PTR_TYPE_SUBTREE; } + if (note_tree_insert(t, node, n, l, type, + combine_notes_concatenate)) + die("Failed to load %s %s into notes tree " + "from %s", + type == PTR_TYPE_NOTE ? "note" : "subtree", + oid_to_hex(&l->key_oid), t->ref); + continue; handle_non_note: From a281639262238a315843a629a7686cff453d6fac Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:03 +0200 Subject: [PATCH 03/13] load_subtree(): reduce the scope of some local variables Declare the variables inside the loop, to make it more obvious that their values are not carried across loop iterations. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/notes.c b/notes.c index a8b9371736..c33aead864 100644 --- a/notes.c +++ b/notes.c @@ -421,9 +421,6 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, void *buf; struct tree_desc desc; struct name_entry entry; - int len, path_len; - unsigned char type; - struct leaf_node *l; buf = fill_tree_descriptor(&desc, subtree->val_oid.hash); if (!buf) @@ -434,7 +431,10 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, assert(prefix_len * 2 >= n); memcpy(object_oid.hash, subtree->key_oid.hash, prefix_len); while (tree_entry(&desc, &entry)) { - path_len = strlen(entry.path); + unsigned char type; + struct leaf_node *l; + int len, path_len = strlen(entry.path); + len = get_oid_hex_segment(entry.path, path_len, object_oid.hash + prefix_len, GIT_SHA1_RAWSZ - prefix_len); if (len < 0) From cbeed9aaa500e2c4b8356a08facaa28121e635de Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:04 +0200 Subject: [PATCH 04/13] load_subtree(): fix incorrect comment This comment was added in 851c2b3791 (Teach notes code to properly preserve non-notes in the notes tree, 2010-02-13) when the corresponding code was added. But I believe it was incorrect even then. The condition `path_len != 2` a dozen lines up prevents a path like "dead/beef" from being converted to "de/ad/beef", and indeed the test added in commit 851c2b3 verifies that this case works correctly. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/notes.c b/notes.c index c33aead864..d9f92d59f8 100644 --- a/notes.c +++ b/notes.c @@ -468,23 +468,13 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, handle_non_note: /* - * Determine full path for this non-note entry: - * The filename is already found in entry.path, but the - * directory part of the path must be deduced from the subtree - * containing this entry. We assume here that the overall notes - * tree follows a strict byte-based progressive fanout - * structure (i.e. using 2/38, 2/2/36, etc. fanouts, and not - * e.g. 4/36 fanout). This means that if a non-note is found at - * path "dead/beef", the following code will register it as - * being found on "de/ad/beef". - * On the other hand, if you use such non-obvious non-note - * paths in the middle of a notes tree, you deserve what's - * coming to you ;). Note that for non-notes that are not - * SHA1-like at the top level, there will be no problems. - * - * To conclude, it is strongly advised to make sure non-notes - * have at least one non-hex character in the top-level path - * component. + * Determine full path for this non-note entry. The + * filename is already found in entry.path, but the + * directory part of the path must be deduced from the + * subtree containing this entry based on our + * knowledge that the overall notes tree follows a + * strict byte-based progressive fanout structure + * (i.e. using 2/38, 2/2/36, etc. fanouts). */ { struct strbuf non_note_path = STRBUF_INIT; From 98c9897d9e40f49cfb416801851656bac494ae40 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:05 +0200 Subject: [PATCH 05/13] load_subtree(): separate logic for internal vs. terminal entries There are only two legitimate notes path components: * A hexadecimal string that fills the rest of the SHA-1 * A two-digit hexadecimal string that constitutes another internal node. So handle those two cases at the top level, and reject others as non-notes without trying to parse them. The logic separation also simplifies upcoming changes. This prevents us from leaking memory for a leaf_node in the case of wrong-sized paths. There are still memory leaks in this code; they will be fixed in upcoming commits. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 50 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/notes.c b/notes.c index d9f92d59f8..5be570e398 100644 --- a/notes.c +++ b/notes.c @@ -433,30 +433,40 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, while (tree_entry(&desc, &entry)) { unsigned char type; struct leaf_node *l; - int len, path_len = strlen(entry.path); + int path_len = strlen(entry.path); - len = get_oid_hex_segment(entry.path, path_len, - object_oid.hash + prefix_len, GIT_SHA1_RAWSZ - prefix_len); - if (len < 0) - goto handle_non_note; /* entry.path is not a SHA1 */ - len += prefix_len; + if (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) { + /* This is potentially the remainder of the SHA-1 */ + if (get_oid_hex_segment(entry.path, path_len, + object_oid.hash + prefix_len, + GIT_SHA1_RAWSZ - prefix_len) < 0) + goto handle_non_note; /* entry.path is not a SHA1 */ + + type = PTR_TYPE_NOTE; + l = (struct leaf_node *) + xcalloc(1, sizeof(struct leaf_node)); + oidcpy(&l->key_oid, &object_oid); + oidcpy(&l->val_oid, entry.oid); + } else if (path_len == 2) { + /* This is potentially an internal node */ + if (get_oid_hex_segment(entry.path, 2, + object_oid.hash + prefix_len, + GIT_SHA1_RAWSZ - prefix_len) < 0) + goto handle_non_note; /* entry.path is not a SHA1 */ - /* - * If object SHA1 is complete (len == 20), assume note object - * If object SHA1 is incomplete (len < 20), and current - * component consists of 2 hex chars, assume note subtree - */ - type = PTR_TYPE_NOTE; - l = (struct leaf_node *) - xcalloc(1, sizeof(struct leaf_node)); - oidcpy(&l->key_oid, &object_oid); - oidcpy(&l->val_oid, entry.oid); - if (len < GIT_SHA1_RAWSZ) { - if (!S_ISDIR(entry.mode) || path_len != 2) - goto handle_non_note; /* not subtree */ - l->key_oid.hash[KEY_INDEX] = (unsigned char) len; type = PTR_TYPE_SUBTREE; + l = (struct leaf_node *) + xcalloc(1, sizeof(struct leaf_node)); + oidcpy(&l->key_oid, &object_oid); + oidcpy(&l->val_oid, entry.oid); + if (!S_ISDIR(entry.mode)) + goto handle_non_note; /* not subtree */ + l->key_oid.hash[KEY_INDEX] = (unsigned char) (prefix_len + 1); + } else { + /* This can't be part of a note */ + goto handle_non_note; } + if (note_tree_insert(t, node, n, l, type, combine_notes_concatenate)) die("Failed to load %s %s into notes tree " From 4d589b87e857015b4f78e47353f87d4aa717b1a0 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:06 +0200 Subject: [PATCH 06/13] load_subtree(): check earlier whether an internal node is a tree entry If an entry is not a tree entry, then it cannot possibly be an internal node. But the old code checked this condition only after allocating a leaf_node object and therefore leaked that memory. Instead, check before even entering this branch of the code. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/notes.c b/notes.c index 5be570e398..61a5001fc0 100644 --- a/notes.c +++ b/notes.c @@ -449,6 +449,11 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, oidcpy(&l->val_oid, entry.oid); } else if (path_len == 2) { /* This is potentially an internal node */ + + if (!S_ISDIR(entry.mode)) + /* internal nodes must be trees */ + goto handle_non_note; + if (get_oid_hex_segment(entry.path, 2, object_oid.hash + prefix_len, GIT_SHA1_RAWSZ - prefix_len) < 0) @@ -459,8 +464,6 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, xcalloc(1, sizeof(struct leaf_node)); oidcpy(&l->key_oid, &object_oid); oidcpy(&l->val_oid, entry.oid); - if (!S_ISDIR(entry.mode)) - goto handle_non_note; /* not subtree */ l->key_oid.hash[KEY_INDEX] = (unsigned char) (prefix_len + 1); } else { /* This can't be part of a note */ From 404321879585b392118c9f4c3c4c9e4ccd6ba09f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:07 +0200 Subject: [PATCH 07/13] load_subtree(): only consider blobs to be potential notes The old code converted any entry whose path constituted a full SHA-1 as a leaf node, without regard for the type of the entry. But only blobs can be notes. So treat entries whose paths *look like* notes paths but that are not blobs as non-notes. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/notes.c b/notes.c index 61a5001fc0..8bca18edbb 100644 --- a/notes.c +++ b/notes.c @@ -437,6 +437,11 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, if (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) { /* This is potentially the remainder of the SHA-1 */ + + if (!S_ISREG(entry.mode)) + /* notes must be blobs */ + goto handle_non_note; + if (get_oid_hex_segment(entry.path, path_len, object_oid.hash + prefix_len, GIT_SHA1_RAWSZ - prefix_len) < 0) From 67c9b422513ac601c68c191a026af968a2838ae1 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:08 +0200 Subject: [PATCH 08/13] get_oid_hex_segment(): return 0 on success Nobody cares about the return value of get_oid_hex_segment() except to check whether it failed. So just return 0 on success. And while we're updating its docstring, update it for some argument renaming that happened a while ago. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/notes.c b/notes.c index 8bca18edbb..930ae3a0d7 100644 --- a/notes.c +++ b/notes.c @@ -338,11 +338,10 @@ static void note_tree_free(struct int_node *tree) * Convert a partial SHA1 hex string to the corresponding partial SHA1 value. * - hex - Partial SHA1 segment in ASCII hex format * - hex_len - Length of above segment. Must be multiple of 2 between 0 and 40 - * - sha1 - Partial SHA1 value is written here - * - sha1_len - Max #bytes to store in sha1, Must be >= hex_len / 2, and < 20 - * Returns -1 on error (invalid arguments or invalid SHA1 (not in hex format)). - * Otherwise, returns number of bytes written to sha1 (i.e. hex_len / 2). - * Pads sha1 with NULs up to sha1_len (not included in returned length). + * - oid - Partial SHA1 value is written here + * - oid_len - Max #bytes to store in sha1, Must be >= hex_len / 2, and < 20 + * Return 0 on success or -1 on error (invalid arguments or input not + * in hex format). Pad oid with NULs up to oid_len. */ static int get_oid_hex_segment(const char *hex, unsigned int hex_len, unsigned char *oid, unsigned int oid_len) @@ -359,7 +358,7 @@ static int get_oid_hex_segment(const char *hex, unsigned int hex_len, } for (; i < oid_len; i++) *oid++ = 0; - return len; + return 0; } static int non_note_cmp(const struct non_note *a, const struct non_note *b) @@ -444,7 +443,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, if (get_oid_hex_segment(entry.path, path_len, object_oid.hash + prefix_len, - GIT_SHA1_RAWSZ - prefix_len) < 0) + GIT_SHA1_RAWSZ - prefix_len)) goto handle_non_note; /* entry.path is not a SHA1 */ type = PTR_TYPE_NOTE; @@ -461,7 +460,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, if (get_oid_hex_segment(entry.path, 2, object_oid.hash + prefix_len, - GIT_SHA1_RAWSZ - prefix_len) < 0) + GIT_SHA1_RAWSZ - prefix_len)) goto handle_non_note; /* entry.path is not a SHA1 */ type = PTR_TYPE_SUBTREE; From 4ebef533d75a197f660b38d615489ad8a233bba1 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:09 +0200 Subject: [PATCH 09/13] load_subtree(): combine some common code Write the length into `object_oid` (before copying) rather than `l->key_oid` (after copying). Then combine some code from the two `if` blocks. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/notes.c b/notes.c index 930ae3a0d7..0074bc9b89 100644 --- a/notes.c +++ b/notes.c @@ -447,10 +447,6 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, goto handle_non_note; /* entry.path is not a SHA1 */ type = PTR_TYPE_NOTE; - l = (struct leaf_node *) - xcalloc(1, sizeof(struct leaf_node)); - oidcpy(&l->key_oid, &object_oid); - oidcpy(&l->val_oid, entry.oid); } else if (path_len == 2) { /* This is potentially an internal node */ @@ -463,17 +459,17 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, GIT_SHA1_RAWSZ - prefix_len)) goto handle_non_note; /* entry.path is not a SHA1 */ + object_oid.hash[KEY_INDEX] = (unsigned char) (prefix_len + 1); + type = PTR_TYPE_SUBTREE; - l = (struct leaf_node *) - xcalloc(1, sizeof(struct leaf_node)); - oidcpy(&l->key_oid, &object_oid); - oidcpy(&l->val_oid, entry.oid); - l->key_oid.hash[KEY_INDEX] = (unsigned char) (prefix_len + 1); } else { /* This can't be part of a note */ goto handle_non_note; } + l = xcalloc(1, sizeof(*l)); + oidcpy(&l->key_oid, &object_oid); + oidcpy(&l->val_oid, entry.oid); if (note_tree_insert(t, node, n, l, type, combine_notes_concatenate)) die("Failed to load %s %s into notes tree " From d49852d6f89bfde3cb20b6ca5866f67f6c04a894 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:10 +0200 Subject: [PATCH 10/13] get_oid_hex_segment(): don't pad the rest of `oid` Remove the feature of `get_oid_hex_segment()` that it pads the rest of the `oid` argument with zeros. Instead, do this at the caller who needs it. This makes the functionality of this function more coherent and removes the need for its `oid_len` argument. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/notes.c b/notes.c index 0074bc9b89..e83f5e8136 100644 --- a/notes.c +++ b/notes.c @@ -339,15 +339,14 @@ static void note_tree_free(struct int_node *tree) * - hex - Partial SHA1 segment in ASCII hex format * - hex_len - Length of above segment. Must be multiple of 2 between 0 and 40 * - oid - Partial SHA1 value is written here - * - oid_len - Max #bytes to store in sha1, Must be >= hex_len / 2, and < 20 * Return 0 on success or -1 on error (invalid arguments or input not - * in hex format). Pad oid with NULs up to oid_len. + * in hex format). */ static int get_oid_hex_segment(const char *hex, unsigned int hex_len, - unsigned char *oid, unsigned int oid_len) + unsigned char *oid) { unsigned int i, len = hex_len >> 1; - if (hex_len % 2 != 0 || len > oid_len) + if (hex_len % 2 != 0) return -1; for (i = 0; i < len; i++) { unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]); @@ -356,8 +355,6 @@ static int get_oid_hex_segment(const char *hex, unsigned int hex_len, *oid++ = val; hex += 2; } - for (; i < oid_len; i++) - *oid++ = 0; return 0; } @@ -442,24 +439,29 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, goto handle_non_note; if (get_oid_hex_segment(entry.path, path_len, - object_oid.hash + prefix_len, - GIT_SHA1_RAWSZ - prefix_len)) + object_oid.hash + prefix_len)) goto handle_non_note; /* entry.path is not a SHA1 */ type = PTR_TYPE_NOTE; } else if (path_len == 2) { /* This is potentially an internal node */ + size_t len = prefix_len; if (!S_ISDIR(entry.mode)) /* internal nodes must be trees */ goto handle_non_note; if (get_oid_hex_segment(entry.path, 2, - object_oid.hash + prefix_len, - GIT_SHA1_RAWSZ - prefix_len)) + object_oid.hash + len++)) goto handle_non_note; /* entry.path is not a SHA1 */ - object_oid.hash[KEY_INDEX] = (unsigned char) (prefix_len + 1); + /* + * Pad the rest of the SHA-1 with zeros, + * except for the last byte, where we write + * the length: + */ + memset(object_oid.hash + len, 0, GIT_SHA1_RAWSZ - len - 1); + object_oid.hash[KEY_INDEX] = (unsigned char)len; type = PTR_TYPE_SUBTREE; } else { From cfdc88f1a34ccb8e59899501b0ede875afac0d83 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:11 +0200 Subject: [PATCH 11/13] hex_to_bytes(): simpler replacement for `get_oid_hex_segment()` Now that `get_oid_hex_segment()` does less, it makes sense to rename it and simplify its semantics: * Instead of a `hex_len` parameter, which was the number of hex characters (and had to be even), use a `len` parameter, which is the number of resulting bytes. This removes then need for the check that `hex_len` is even and to divide it by two to determine the number of bytes. For good hygiene, declare the `len` parameter to be `size_t` instead of `unsigned int`. * Change the order of the arguments to the more traditional (dst, src, len). * Rename the function to `hex_to_bytes()`. * Remove a loop variable: just count `len` down instead. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/notes.c b/notes.c index e83f5e8136..355669dcfb 100644 --- a/notes.c +++ b/notes.c @@ -335,25 +335,18 @@ static void note_tree_free(struct int_node *tree) } /* - * Convert a partial SHA1 hex string to the corresponding partial SHA1 value. - * - hex - Partial SHA1 segment in ASCII hex format - * - hex_len - Length of above segment. Must be multiple of 2 between 0 and 40 - * - oid - Partial SHA1 value is written here - * Return 0 on success or -1 on error (invalid arguments or input not - * in hex format). + * Read `len` pairs of hexadecimal digits from `hex` and write the + * values to `binary` as `len` bytes. Return 0 on success, or -1 if + * the input does not consist of hex digits). */ -static int get_oid_hex_segment(const char *hex, unsigned int hex_len, - unsigned char *oid) +static int hex_to_bytes(unsigned char *binary, const char *hex, size_t len) { - unsigned int i, len = hex_len >> 1; - if (hex_len % 2 != 0) - return -1; - for (i = 0; i < len; i++) { + for (; len; len--, hex += 2) { unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]); + if (val & ~0xff) return -1; - *oid++ = val; - hex += 2; + *binary++ = val; } return 0; } @@ -438,8 +431,8 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, /* notes must be blobs */ goto handle_non_note; - if (get_oid_hex_segment(entry.path, path_len, - object_oid.hash + prefix_len)) + if (hex_to_bytes(object_oid.hash + prefix_len, entry.path, + GIT_SHA1_RAWSZ - prefix_len)) goto handle_non_note; /* entry.path is not a SHA1 */ type = PTR_TYPE_NOTE; @@ -451,8 +444,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, /* internal nodes must be trees */ goto handle_non_note; - if (get_oid_hex_segment(entry.path, 2, - object_oid.hash + len++)) + if (hex_to_bytes(object_oid.hash + len++, entry.path, 1)) goto handle_non_note; /* entry.path is not a SHA1 */ /* From 06cfa75675ac605577c9a42154a9042bfd451937 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 26 Aug 2017 10:28:12 +0200 Subject: [PATCH 12/13] load_subtree(): declare some variables to be `size_t` * `prefix_len` * `path_len` * `i` It's good hygiene. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/notes.c b/notes.c index 355669dcfb..40d9ba6252 100644 --- a/notes.c +++ b/notes.c @@ -406,7 +406,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, struct int_node *node, unsigned int n) { struct object_id object_oid; - unsigned int prefix_len; + size_t prefix_len; void *buf; struct tree_desc desc; struct name_entry entry; @@ -422,7 +422,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, while (tree_entry(&desc, &entry)) { unsigned char type; struct leaf_node *l; - int path_len = strlen(entry.path); + size_t path_len = strlen(entry.path); if (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) { /* This is potentially the remainder of the SHA-1 */ @@ -486,7 +486,7 @@ handle_non_note: { struct strbuf non_note_path = STRBUF_INIT; const char *q = oid_to_hex(&subtree->key_oid); - int i; + size_t i; for (i = 0; i < prefix_len; i++) { strbuf_addch(&non_note_path, *q++); strbuf_addch(&non_note_path, *q++); From 396428152413f431cac18f68a7190827b4acb3b6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 8 Sep 2017 18:10:10 +0200 Subject: [PATCH 13/13] load_subtree(): check that `prefix_len` is in the expected range This value, which is stashed in the last byte of an object_id hash, gets handed around a lot. So add a sanity check before using it in `load_subtree()`. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- notes.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/notes.c b/notes.c index 40d9ba6252..27d232f294 100644 --- a/notes.c +++ b/notes.c @@ -417,7 +417,10 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, oid_to_hex(&subtree->val_oid)); prefix_len = subtree->key_oid.hash[KEY_INDEX]; - assert(prefix_len * 2 >= n); + if (prefix_len >= GIT_SHA1_RAWSZ) + BUG("prefix_len (%"PRIuMAX") is out of range", (uintmax_t)prefix_len); + if (prefix_len * 2 < n) + BUG("prefix_len (%"PRIuMAX") is too small", (uintmax_t)prefix_len); memcpy(object_oid.hash, subtree->key_oid.hash, prefix_len); while (tree_entry(&desc, &entry)) { unsigned char type;