From 10322a0aaf84382d8901f9ab59e59c39f0c035bb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 10 Jun 2014 17:39:11 -0400 Subject: do not create "struct commit" with xcalloc In both blame and merge-recursive, we sometimes create a "fake" commit struct for convenience (e.g., to represent the HEAD state as if we would commit it). By allocating ourselves rather than using alloc_commit_node, we do not properly set the "index" field of the commit. This can produce subtle bugs if we then use commit-slab on the resulting commit, as we will share the "0" index with another commit. We can fix this by using alloc_commit_node() to allocate. Note that we cannot free the result, as it is part of our commit allocator. However, both cases were already leaking the allocated commit anyway, so there's nothing to fix up. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- merge-recursive.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 4177092942..fc2a68a0d1 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -40,7 +40,7 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two, static struct commit *make_virtual_commit(struct tree *tree, const char *comment) { - struct commit *commit = xcalloc(1, sizeof(struct commit)); + struct commit *commit = alloc_commit_node(); struct merge_remote_desc *desc = xmalloc(sizeof(*desc)); desc->name = comment; -- cgit v1.2.3 From bc6b8fc1300ef79c4b4c3c2a79bb3c1e2e032963 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 10 Jun 2014 17:41:51 -0400 Subject: use get_commit_buffer everywhere Each of these sites assumes that commit->buffer is valid. Since they would segfault if this was not the case, they are likely to be correct in practice. However, we can future-proof them by using get_commit_buffer. And as a side effect, we abstract away the final bare uses of commit->buffer. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fast-export.c | 5 ++++- builtin/fmt-merge-msg.c | 5 ++++- builtin/log.c | 7 +++++-- fsck.c | 13 +++++++++++-- merge-recursive.c | 4 +++- notes-merge.c | 4 +++- sequencer.c | 4 +++- 7 files changed, 33 insertions(+), 9 deletions(-) (limited to 'merge-recursive.c') diff --git a/builtin/fast-export.c b/builtin/fast-export.c index b8d8a3aaf9..7ee5e08442 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -279,6 +279,7 @@ static const char *find_encoding(const char *begin, const char *end) static void handle_commit(struct commit *commit, struct rev_info *rev) { int saved_output_format = rev->diffopt.output_format; + const char *commit_buffer; const char *author, *author_end, *committer, *committer_end; const char *encoding, *message; char *reencoded = NULL; @@ -288,7 +289,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev) rev->diffopt.output_format = DIFF_FORMAT_CALLBACK; parse_commit_or_die(commit); - author = strstr(commit->buffer, "\nauthor "); + commit_buffer = get_commit_buffer(commit); + author = strstr(commit_buffer, "\nauthor "); if (!author) die ("Could not find author in commit %s", sha1_to_hex(commit->object.sha1)); @@ -335,6 +337,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev) ? strlen(message) : 0), reencoded ? reencoded : message ? message : ""); free(reencoded); + unuse_commit_buffer(commit, commit_buffer); for (i = 0, p = commit->parents; p; p = p->next) { int mark = get_object_mark(&p->item->object); diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 3906eda877..01f6d59eef 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -230,12 +230,14 @@ static void add_branch_desc(struct strbuf *out, const char *name) static void record_person(int which, struct string_list *people, struct commit *commit) { + const char *buffer; char *name_buf, *name, *name_end; struct string_list_item *elem; const char *field; field = (which == 'a') ? "\nauthor " : "\ncommitter "; - name = strstr(commit->buffer, field); + buffer = get_commit_buffer(commit); + name = strstr(buffer, field); if (!name) return; name += strlen(field); @@ -247,6 +249,7 @@ static void record_person(int which, struct string_list *people, if (name_end < name) return; name_buf = xmemdupz(name, name_end - name + 1); + unuse_commit_buffer(commit, buffer); elem = string_list_lookup(people, name_buf); if (!elem) { diff --git a/builtin/log.c b/builtin/log.c index 226f8f2980..2c742606bc 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -918,9 +918,12 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, log_write_email_headers(rev, head, &pp.subject, &pp.after_subject, &need_8bit_cte); - for (i = 0; !need_8bit_cte && i < nr; i++) - if (has_non_ascii(list[i]->buffer)) + for (i = 0; !need_8bit_cte && i < nr; i++) { + const char *buf = get_commit_buffer(list[i]); + if (has_non_ascii(buf)) need_8bit_cte = 1; + unuse_commit_buffer(list[i], buf); + } if (!branch_name) branch_name = find_branch_name(rev); diff --git a/fsck.c b/fsck.c index abed62bac7..8223780592 100644 --- a/fsck.c +++ b/fsck.c @@ -276,9 +276,10 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f return 0; } -static int fsck_commit(struct commit *commit, fsck_error error_func) +static int fsck_commit_buffer(struct commit *commit, const char *buffer, + fsck_error error_func) { - const char *buffer = commit->buffer, *tmp; + const char *tmp; unsigned char tree_sha1[20], sha1[20]; struct commit_graft *graft; int parents = 0; @@ -336,6 +337,14 @@ static int fsck_commit(struct commit *commit, fsck_error error_func) return 0; } +static int fsck_commit(struct commit *commit, fsck_error error_func) +{ + const char *buffer = get_commit_buffer(commit); + int ret = fsck_commit_buffer(commit, buffer, error_func); + unuse_commit_buffer(commit, buffer); + return ret; +} + static int fsck_tag(struct tag *tag, fsck_error error_func) { struct object *tagged = tag->tagged; diff --git a/merge-recursive.c b/merge-recursive.c index fc2a68a0d1..78908aaacc 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -190,9 +190,11 @@ static void output_commit_title(struct merge_options *o, struct commit *commit) printf(_("(bad commit)\n")); else { const char *title; - int len = find_commit_subject(commit->buffer, &title); + const char *msg = get_commit_buffer(commit); + int len = find_commit_subject(msg, &title); if (len) printf("%.*s\n", len, title); + unuse_commit_buffer(commit, msg); } } } diff --git a/notes-merge.c b/notes-merge.c index 697cec349a..e804db2d02 100644 --- a/notes-merge.c +++ b/notes-merge.c @@ -672,7 +672,8 @@ int notes_merge_commit(struct notes_merge_options *o, DIR *dir; struct dirent *e; struct strbuf path = STRBUF_INIT; - char *msg = strstr(partial_commit->buffer, "\n\n"); + const char *buffer = get_commit_buffer(partial_commit); + const char *msg = strstr(buffer, "\n\n"); int baselen; strbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE)); @@ -721,6 +722,7 @@ int notes_merge_commit(struct notes_merge_options *o, create_notes_commit(partial_tree, partial_commit->parents, msg, strlen(msg), result_sha1); + unuse_commit_buffer(partial_commit, buffer); if (o->verbosity >= 4) printf("Finalized notes merge commit: %s\n", sha1_to_hex(result_sha1)); diff --git a/sequencer.c b/sequencer.c index 7b1f87af35..69bcf3d801 100644 --- a/sequencer.c +++ b/sequencer.c @@ -662,10 +662,12 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list, int subject_len; for (cur = todo_list; cur; cur = cur->next) { + const char *commit_buffer = get_commit_buffer(cur->item); sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV); - subject_len = find_commit_subject(cur->item->buffer, &subject); + subject_len = find_commit_subject(commit_buffer, &subject); strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev, subject_len, subject); + unuse_commit_buffer(cur->item, commit_buffer); } return 0; } -- cgit v1.2.3 From 8597ea3afea067b39ba7d4adae7ec6c1ee0e7c91 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 10 Jun 2014 17:44:13 -0400 Subject: commit: record buffer length in cache Most callsites which use the commit buffer try to use the cached version attached to the commit, rather than re-reading from disk. Unfortunately, that interface provides only a pointer to the NUL-terminated buffer, with no indication of the original length. For the most part, this doesn't matter. People do not put NULs in their commit messages, and the log code is happy to treat it all as a NUL-terminated string. However, some code paths do care. For example, when checking signatures, we want to be very careful that we verify all the bytes to avoid malicious trickery. This patch just adds an optional "size" out-pointer to get_commit_buffer and friends. The existing callers all pass NULL (there did not seem to be any obvious sites where we could avoid an immediate strlen() call, though perhaps with some further refactoring we could). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/blame.c | 14 ++++++++++++- builtin/fast-export.c | 2 +- builtin/fmt-merge-msg.c | 2 +- builtin/index-pack.c | 2 +- builtin/log.c | 2 +- builtin/rev-list.c | 2 +- commit.c | 54 ++++++++++++++++++++++++++++++++----------------- commit.h | 8 ++++---- fsck.c | 2 +- log-tree.c | 2 +- merge-recursive.c | 2 +- notes-merge.c | 2 +- object.c | 4 ++-- pretty.c | 4 ++-- sequencer.c | 2 +- sha1_name.c | 2 +- 16 files changed, 68 insertions(+), 38 deletions(-) (limited to 'merge-recursive.c') diff --git a/builtin/blame.c b/builtin/blame.c index 857d98a324..b84e375b5c 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1998,6 +1998,18 @@ static void append_merge_parents(struct commit_list **tail) strbuf_release(&line); } +/* + * This isn't as simple as passing sb->buf and sb->len, because we + * want to transfer ownership of the buffer to the commit (so we + * must use detach). + */ +static void set_commit_buffer_from_strbuf(struct commit *c, struct strbuf *sb) +{ + size_t len; + void *buf = strbuf_detach(sb, &len); + set_commit_buffer(c, buf, len); +} + /* * Prepare a dummy commit that represents the work tree (or staged) item. * Note that annotating work tree item never works in the reverse. @@ -2046,7 +2058,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, ident, ident, path, (!contents_from ? path : (!strcmp(contents_from, "-") ? "standard input" : contents_from))); - set_commit_buffer(commit, strbuf_detach(&msg, NULL)); + set_commit_buffer_from_strbuf(commit, &msg); if (!contents_from || strcmp("-", contents_from)) { struct stat st; diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 7ee5e08442..05d161f19f 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -289,7 +289,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev) rev->diffopt.output_format = DIFF_FORMAT_CALLBACK; parse_commit_or_die(commit); - commit_buffer = get_commit_buffer(commit); + commit_buffer = get_commit_buffer(commit, NULL); author = strstr(commit_buffer, "\nauthor "); if (!author) die ("Could not find author in commit %s", diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 01f6d59eef..ef8b254ef2 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -236,7 +236,7 @@ static void record_person(int which, struct string_list *people, const char *field; field = (which == 'a') ? "\nauthor " : "\ncommitter "; - buffer = get_commit_buffer(commit); + buffer = get_commit_buffer(commit, NULL); name = strstr(buffer, field); if (!name) return; diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 42551ce4ff..459b9f07bb 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -774,7 +774,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry, } if (obj->type == OBJ_COMMIT) { struct commit *commit = (struct commit *) obj; - if (detach_commit_buffer(commit) != data) + if (detach_commit_buffer(commit, NULL) != data) die("BUG: parse_object_buffer transmogrified our buffer"); } obj->flags |= FLAG_CHECKED; diff --git a/builtin/log.c b/builtin/log.c index 2c742606bc..c599eacf72 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -919,7 +919,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, &need_8bit_cte); for (i = 0; !need_8bit_cte && i < nr; i++) { - const char *buf = get_commit_buffer(list[i]); + const char *buf = get_commit_buffer(list[i], NULL); if (has_non_ascii(buf)) need_8bit_cte = 1; unuse_commit_buffer(list[i], buf); diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 3fcbf21c03..ff84a825ff 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -106,7 +106,7 @@ static void show_commit(struct commit *commit, void *data) else putchar('\n'); - if (revs->verbose_header && get_cached_commit_buffer(commit)) { + if (revs->verbose_header && get_cached_commit_buffer(commit, NULL)) { struct strbuf buf = STRBUF_INIT; struct pretty_print_context ctx = {0}; ctx.abbrev = revs->abbrev; diff --git a/commit.c b/commit.c index e289c78327..a036e181c7 100644 --- a/commit.c +++ b/commit.c @@ -245,22 +245,31 @@ int unregister_shallow(const unsigned char *sha1) return 0; } -define_commit_slab(buffer_slab, void *); +struct commit_buffer { + void *buffer; + unsigned long size; +}; +define_commit_slab(buffer_slab, struct commit_buffer); static struct buffer_slab buffer_slab = COMMIT_SLAB_INIT(1, buffer_slab); -void set_commit_buffer(struct commit *commit, void *buffer) +void set_commit_buffer(struct commit *commit, void *buffer, unsigned long size) { - *buffer_slab_at(&buffer_slab, commit) = buffer; + struct commit_buffer *v = buffer_slab_at(&buffer_slab, commit); + v->buffer = buffer; + v->size = size; } -const void *get_cached_commit_buffer(const struct commit *commit) +const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep) { - return *buffer_slab_at(&buffer_slab, commit); + struct commit_buffer *v = buffer_slab_at(&buffer_slab, commit); + if (sizep) + *sizep = v->size; + return v->buffer; } -const void *get_commit_buffer(const struct commit *commit) +const void *get_commit_buffer(const struct commit *commit, unsigned long *sizep) { - const void *ret = get_cached_commit_buffer(commit); + const void *ret = get_cached_commit_buffer(commit, sizep); if (!ret) { enum object_type type; unsigned long size; @@ -271,29 +280,38 @@ const void *get_commit_buffer(const struct commit *commit) if (type != OBJ_COMMIT) die("expected commit for %s, got %s", sha1_to_hex(commit->object.sha1), typename(type)); + if (sizep) + *sizep = size; } return ret; } void unuse_commit_buffer(const struct commit *commit, const void *buffer) { - void *cached = *buffer_slab_at(&buffer_slab, commit); - if (cached != buffer) + struct commit_buffer *v = buffer_slab_at(&buffer_slab, commit); + if (v->buffer != buffer) free((void *)buffer); } void free_commit_buffer(struct commit *commit) { - void **b = buffer_slab_at(&buffer_slab, commit); - free(*b); - *b = NULL; + struct commit_buffer *v = buffer_slab_at(&buffer_slab, commit); + free(v->buffer); + v->buffer = NULL; + v->size = 0; } -const void *detach_commit_buffer(struct commit *commit) +const void *detach_commit_buffer(struct commit *commit, unsigned long *sizep) { - void **b = buffer_slab_at(&buffer_slab, commit); - void *ret = *b; - *b = NULL; + struct commit_buffer *v = buffer_slab_at(&buffer_slab, commit); + void *ret; + + ret = v->buffer; + if (sizep) + *sizep = v->size; + + v->buffer = NULL; + v->size = 0; return ret; } @@ -374,7 +392,7 @@ int parse_commit(struct commit *item) } ret = parse_commit_buffer(item, buffer, size); if (save_commit_buffer && !ret) { - set_commit_buffer(item, buffer); + set_commit_buffer(item, buffer, size); return 0; } free(buffer); @@ -589,7 +607,7 @@ static void record_author_date(struct author_date_slab *author_date, struct commit *commit) { const char *buf, *line_end, *ident_line; - const char *buffer = get_commit_buffer(commit); + const char *buffer = get_commit_buffer(commit, NULL); struct ident_split ident; char *date_end; unsigned long date; diff --git a/commit.h b/commit.h index e1c25692f1..61559a9d45 100644 --- a/commit.h +++ b/commit.h @@ -54,20 +54,20 @@ void parse_commit_or_die(struct commit *item); * Associate an object buffer with the commit. The ownership of the * memory is handed over to the commit, and must be free()-able. */ -void set_commit_buffer(struct commit *, void *buffer); +void set_commit_buffer(struct commit *, void *buffer, unsigned long size); /* * Get any cached object buffer associated with the commit. Returns NULL * if none. The resulting memory should not be freed. */ -const void *get_cached_commit_buffer(const struct commit *); +const void *get_cached_commit_buffer(const struct commit *, unsigned long *size); /* * Get the commit's object contents, either from cache or by reading the object * from disk. The resulting memory should not be modified, and must be given * to unuse_commit_buffer when the caller is done. */ -const void *get_commit_buffer(const struct commit *); +const void *get_commit_buffer(const struct commit *, unsigned long *size); /* * Tell the commit subsytem that we are done with a particular commit buffer. @@ -86,7 +86,7 @@ void free_commit_buffer(struct commit *); * Disassociate any cached object buffer from the commit, but do not free it. * The buffer (or NULL, if none) is returned. */ -const void *detach_commit_buffer(struct commit *); +const void *detach_commit_buffer(struct commit *, unsigned long *sizep); /* Find beginning and length of commit subject. */ int find_commit_subject(const char *commit_buffer, const char **subject); diff --git a/fsck.c b/fsck.c index 8223780592..a7233c8d0b 100644 --- a/fsck.c +++ b/fsck.c @@ -339,7 +339,7 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer, static int fsck_commit(struct commit *commit, fsck_error error_func) { - const char *buffer = get_commit_buffer(commit); + const char *buffer = get_commit_buffer(commit, NULL); int ret = fsck_commit_buffer(commit, buffer, error_func); unuse_commit_buffer(commit, buffer); return ret; diff --git a/log-tree.c b/log-tree.c index e9ef8abd37..444702163a 100644 --- a/log-tree.c +++ b/log-tree.c @@ -588,7 +588,7 @@ void show_log(struct rev_info *opt) show_mergetag(opt, commit); } - if (!get_cached_commit_buffer(commit)) + if (!get_cached_commit_buffer(commit, NULL)) return; if (opt->show_notes) { diff --git a/merge-recursive.c b/merge-recursive.c index 78908aaacc..a9ab328923 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -190,7 +190,7 @@ static void output_commit_title(struct merge_options *o, struct commit *commit) printf(_("(bad commit)\n")); else { const char *title; - const char *msg = get_commit_buffer(commit); + const char *msg = get_commit_buffer(commit, NULL); int len = find_commit_subject(msg, &title); if (len) printf("%.*s\n", len, title); diff --git a/notes-merge.c b/notes-merge.c index e804db2d02..fd5fae255d 100644 --- a/notes-merge.c +++ b/notes-merge.c @@ -672,7 +672,7 @@ int notes_merge_commit(struct notes_merge_options *o, DIR *dir; struct dirent *e; struct strbuf path = STRBUF_INIT; - const char *buffer = get_commit_buffer(partial_commit); + const char *buffer = get_commit_buffer(partial_commit, NULL); const char *msg = strstr(buffer, "\n\n"); int baselen; diff --git a/object.c b/object.c index 67b6e3533d..9c31e9a5e0 100644 --- a/object.c +++ b/object.c @@ -197,8 +197,8 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t if (commit) { if (parse_commit_buffer(commit, buffer, size)) return NULL; - if (!get_cached_commit_buffer(commit)) { - set_commit_buffer(commit, buffer); + if (!get_cached_commit_buffer(commit, NULL)) { + set_commit_buffer(commit, buffer, size); *eaten_p = 1; } obj = &commit->object; diff --git a/pretty.c b/pretty.c index 915bd1e2e9..b9fceedbb9 100644 --- a/pretty.c +++ b/pretty.c @@ -613,7 +613,7 @@ const char *logmsg_reencode(const struct commit *commit, static const char *utf8 = "UTF-8"; const char *use_encoding; char *encoding; - const char *msg = get_commit_buffer(commit); + const char *msg = get_commit_buffer(commit, NULL); char *out; if (!output_encoding || !*output_encoding) { @@ -642,7 +642,7 @@ const char *logmsg_reencode(const struct commit *commit, * the cached copy from get_commit_buffer, we need to duplicate it * to avoid munging the cached copy. */ - if (msg == get_cached_commit_buffer(commit)) + if (msg == get_cached_commit_buffer(commit, NULL)) out = xstrdup(msg); else out = (char *)msg; diff --git a/sequencer.c b/sequencer.c index 69bcf3d801..bbaddcb05a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -662,7 +662,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list, int subject_len; for (cur = todo_list; cur; cur = cur->next) { - const char *commit_buffer = get_commit_buffer(cur->item); + const char *commit_buffer = get_commit_buffer(cur->item, NULL); sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV); subject_len = find_commit_subject(commit_buffer, &subject); strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev, diff --git a/sha1_name.c b/sha1_name.c index 0a65d234de..c2c938c4e1 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -869,7 +869,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1, commit = pop_most_recent_commit(&list, ONELINE_SEEN); if (!parse_object(commit->object.sha1)) continue; - buf = get_commit_buffer(commit); + buf = get_commit_buffer(commit, NULL); p = strstr(buf, "\n\n"); matches = p && !regexec(®ex, p + 2, 0, NULL, 0); unuse_commit_buffer(commit, buf); -- cgit v1.2.3