[v4,0/2] setup: fix memory leaks with `struct repository_format`

Message

Martin Ågren Feb. 28, 2019, 8:36 p.m. UTC

This is a follow-up to v3 [1] from about a month ago. Patch 1 is
unchanged; patch 2 provides some additional documentation of the
initialization that is required, plus I've gotten rid of the compound
literal. Range-diff below.

Thanks Peff and brian for very helpful comments and discussion.

Martin
 
[1] https://public-inbox.org/git/[e-mail]/

Martin Ågren (2):
  setup: free old value before setting `work_tree`
  setup: fix memory leaks with `struct repository_format`

 cache.h           | 31 ++++++++++++++++++++++++++++---
 builtin/init-db.c |  3 ++-
 repository.c      |  3 ++-
 setup.c           | 40 ++++++++++++++++++++++++++++------------
 worktree.c        |  4 +++-
 5 files changed, 63 insertions(+), 18 deletions(-)

Range-diff against v3:
1:  13019979b8 = 1:  13019979b8 setup: free old value before setting `work_tree`
2:  e0c4a73119 ! 2:  b21704c1e4 setup: fix memory leaks with `struct repository_format`
    @@ -16,15 +16,16 @@
         they take from it, rather than stealing the pointers.
     
         Call `clear_...()` at the start of `read_...()` instead of just zeroing
    -    the struct, since we sometimes enter the function multiple times. This
    -    means that it is important to initialize the struct before calling
    -    `read_...()`, so document that. Teach `read_...()` to clear the struct
    -    upon an error, so that it is reset to a safe state, and document this.
    +    the struct, since we sometimes enter the function multiple times. Thus,
    +    it is important to initialize the struct before calling `read_...()`, so
    +    document that. It's also important because we might not even call
    +    `read_...()` before we call `clear_...()`, see, e.g., builtin/init-db.c.
     
    -    About that very last point: In `setup_git_directory_gently()`, we look
    -    at `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we
    +    Teach `read_...()` to clear the struct on error, so that it is reset to
    +    a safe state, and document this. (In `setup_git_directory_gently()`, we
    +    look at `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we
         weren't actually supposed to do per the API. After this commit, that's
    -    ok.
    +    ok.)
     
         We inherit the existing code's combining "error" and "no version found".
         Both are signalled through `version == -1` and now both cause us to
    @@ -34,11 +35,21 @@
         non-negative version number before using them.
     
         Signed-off-by: Martin Ågren <[e-mail]>
    -    Signed-off-by: Junio C Hamano <[e-mail]>
     
      diff --git a/cache.h b/cache.h
      --- a/cache.h
      +++ b/cache.h
    +@@
    + extern const char *core_partial_clone_filter_default;
    + extern int repository_format_worktree_config;
    + 
    ++/*
    ++ * You _have_ to initialize a `struct repository_format` using
    ++ * `= REPOSITORY_FORMAT_INIT` before calling `read_repository_format()`.
    ++ */
    + struct repository_format {
    + 	int version;
    + 	int precious_objects;
     @@
      	struct string_list unknown_extensions;
      };
    @@ -146,8 +157,15 @@
      	}
      
      	return 0;
    -@@
    + }
      
    ++static void init_repository_format(struct repository_format *format)
    ++{
    ++	const struct repository_format fresh = REPOSITORY_FORMAT_INIT;
    ++
    ++	memcpy(format, &fresh, sizeof(fresh));
    ++}
    ++
      int read_repository_format(struct repository_format *format, const char *path)
      {
     -	memset(format, 0, sizeof(*format));
    @@ -167,7 +185,7 @@
     +	string_list_clear(&format->unknown_extensions, 0);
     +	free(format->work_tree);
     +	free(format->partial_clone);
    -+	*format = (struct repository_format)REPOSITORY_FORMAT_INIT;
    ++	init_repository_format(format);
     +}
     +
      int verify_repository_format(const struct repository_format *format,

Comments

Jeff King March 6, 2019, 4:56 a.m. UTC | #1

On Thu, Feb 28, 2019 at 09:36:26PM +0100, Martin Ågren wrote:

> This is a follow-up to v3 [1] from about a month ago. Patch 1 is
> unchanged; patch 2 provides some additional documentation of the
> initialization that is required, plus I've gotten rid of the compound
> literal. Range-diff below.

Thanks, this version looks good to me.

-Peff