* [PATCH] commit: fix memory leak in `prepare_index()`
@ 2017-09-20 19:47 Martin Ågren
2017-09-20 19:55 ` Jeff King
0 siblings, 1 reply; 2+ messages in thread
From: Martin Ågren @ 2017-09-20 19:47 UTC (permalink / raw)
To: git
Release `pathspec` and the string list `partial`.
When we clear the string list, make sure we do not free the `util`
pointers. That would result in double-freeing, since we set them up as
`item->util = item` in `list_paths()`.
Initialize the string list early, so that we can always release it. That
introduces some unnecessary overhead in various code paths, but means
there is one and only one way out of the function. If we ever accumulate
more things we need to free, it should be straightforward to do so.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
builtin/commit.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 58f9747..721e5f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -335,7 +335,7 @@ static void refresh_cache_or_die(int refresh_flags)
static const char *prepare_index(int argc, const char **argv, const char *prefix,
const struct commit *current_head, int is_status)
{
- struct string_list partial;
+ struct string_list partial = STRING_LIST_INIT_DUP;
struct pathspec pathspec;
int refresh_flags = REFRESH_QUIET;
const char *ret;
@@ -380,7 +380,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
warning(_("Failed to update main cache tree"));
commit_style = COMMIT_NORMAL;
- return get_lock_file_path(&index_lock);
+ ret = get_lock_file_path(&index_lock);
+ goto out;
}
/*
@@ -403,7 +404,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
die(_("unable to write new_index file"));
commit_style = COMMIT_NORMAL;
- return get_lock_file_path(&index_lock);
+ ret = get_lock_file_path(&index_lock);
+ goto out;
}
/*
@@ -429,7 +431,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
rollback_lock_file(&index_lock);
}
commit_style = COMMIT_AS_IS;
- return get_index_file();
+ ret = get_index_file();
+ goto out;
}
/*
@@ -460,7 +463,6 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
die(_("cannot do a partial commit during a cherry-pick."));
}
- string_list_init(&partial, 1);
if (list_paths(&partial, !current_head ? NULL : "HEAD", prefix, &pathspec))
exit(1);
@@ -490,6 +492,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
discard_cache();
ret = get_lock_file_path(&false_lock);
read_cache_from(ret);
+out:
+ string_list_clear(&partial, 0);
+ clear_pathspec(&pathspec);
return ret;
}
--
2.14.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] commit: fix memory leak in `prepare_index()`
2017-09-20 19:47 [PATCH] commit: fix memory leak in `prepare_index()` Martin Ågren
@ 2017-09-20 19:55 ` Jeff King
0 siblings, 0 replies; 2+ messages in thread
From: Jeff King @ 2017-09-20 19:55 UTC (permalink / raw)
To: Martin Ågren; +Cc: git
On Wed, Sep 20, 2017 at 09:47:23PM +0200, Martin Ågren wrote:
> Release `pathspec` and the string list `partial`.
Makes sense.
> When we clear the string list, make sure we do not free the `util`
> pointers. That would result in double-freeing, since we set them up as
> `item->util = item` in `list_paths()`.
Also makes sense (and is kind of weird; it looks like we're just using
it as a magic flag. But that's outside the scope of your patch).
> Initialize the string list early, so that we can always release it. That
> introduces some unnecessary overhead in various code paths, but means
> there is one and only one way out of the function. If we ever accumulate
> more things we need to free, it should be straightforward to do so.
The overhead is fine. It's just writing the struct entries, not
allocating anything. I wondered if the pathspec needed a similar
initialization (since you can't tell just from reading the context), but
no, it's initialized as the first thing in the function.
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> builtin/commit.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
Looks good to me. Thanks.
-Peff
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-09-20 19:55 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-20 19:47 [PATCH] commit: fix memory leak in `prepare_index()` Martin Ågren
2017-09-20 19:55 ` Jeff King
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).