Re: [PATCH 02/12] load_subtree(): remove unnecessary conditional
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 26, 2017, 16:38 UTC
- Message-ID
- <xmqqh8wuqo6e.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <c21bedbee9487792f4a336a417aa9874578aaac2.1503734566.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> At this point in the code, len is *always* <= 20.
This is the kind of log message that makes me unconfortable, as it lacks "because", and the readers would need to find out themselves by following the same codepath the patch author already followed.
There is an assert earlier before the control gets in this loop
prefix_len = subtree->key_oid.hash[KEY_INDEX]; assert(prefix_len * 2 >= n); memcpy(object_oid.hash, subtree->key_oid.hash, prefix_len);
that tries to ensure there is sufficient number of prefix defined in that key, and the codeflow may ensure that prefix_len is both an even number and shorter than 20 (the correctness of the code depends on these, it seems, and if for some reason prefix_len is much larger, calls to get_oid_hex_segment() will overflow the oid.hash[] array without checking). I'd at least feel safer to have an assert next to the existing one that catches a bug to throw a randomly large value into subtree->key_oid.hash[KEY_INDEX]. Then we can safely say "at this point in the code, len is always <= 20", as that assert will makes it obvious without looking at anything other than this code and get_oid_hex_segment() implementaiton (combined with the fact that this function is the only one that coerces len and puts it into ->key_oid.hash[KEY_INDEX], but that is a weak assurance as we cannot tell where "subtree" came from---it may have full 20-byte oid in its key_oid field---without following the callchain a lot more widely).
Show 52 quoted lines
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> notes.c | 35 +++++++++++++++++------------------
> 1 file changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/notes.c b/notes.c
> index 00630a9396..f7ce64ff48 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: