Skip to content

Commit

Permalink
replace dangerous uses of strbuf_attach
Browse files Browse the repository at this point in the history
It is not a good idea to strbuf_attach an arbitrary pointer
just because a function you are calling wants a strbuf.
Attaching implies a transfer of memory ownership; if anyone
were to modify or release the resulting strbuf, we would
free() the pointer, leading to possible problems:

  1. Other users of the original pointer might access freed
     memory.

  2. The pointer might not be the start of a malloc'd
     area, so calling free() on it in the first place would
     be wrong.

In the two cases modified here, we are fortunate that nobody
touches the strbuf once it is attached, but it is an
accident waiting to happen.  Since the previous commit,
commit_tree and friends take a pointer/buf pair, so we can
just do away with the strbufs entirely.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Jeff King authored and Junio C Hamano committed Jun 12, 2014
1 parent 3ffefb5 commit e6dfcd6
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 8 deletions.
6 changes: 2 additions & 4 deletions notes-cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ int notes_cache_write(struct notes_cache *c)
{
unsigned char tree_sha1[20];
unsigned char commit_sha1[20];
struct strbuf msg = STRBUF_INIT;

if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)
return -1;
Expand All @@ -57,9 +56,8 @@ int notes_cache_write(struct notes_cache *c)

if (write_notes_tree(&c->tree, tree_sha1))
return -1;
strbuf_attach(&msg, c->validity,
strlen(c->validity), strlen(c->validity) + 1);
if (commit_tree(msg.buf, msg.len, tree_sha1, NULL, commit_sha1, NULL, NULL) < 0)
if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,
commit_sha1, NULL, NULL) < 0)
return -1;
if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
0, QUIET_ON_ERR) < 0)
Expand Down
5 changes: 1 addition & 4 deletions notes-merge.c
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,6 @@ int notes_merge_commit(struct notes_merge_options *o,
struct dirent *e;
struct strbuf path = STRBUF_INIT;
char *msg = strstr(partial_commit->buffer, "\n\n");
struct strbuf sb_msg = STRBUF_INIT;
int baselen;

strbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));
Expand Down Expand Up @@ -720,10 +719,8 @@ int notes_merge_commit(struct notes_merge_options *o,
strbuf_setlen(&path, baselen);
}

strbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);
create_notes_commit(partial_tree, partial_commit->parents,
sb_msg.buf, sb_msg.len,
result_sha1);
msg, strlen(msg), result_sha1);
if (o->verbosity >= 4)
printf("Finalized notes merge commit: %s\n",
sha1_to_hex(result_sha1));
Expand Down

0 comments on commit e6dfcd6

Please sign in to comment.