[PATCH v2 0/4] Fix mem leaks of recent object store conversions.

[PATCH v2 0/4] Fix mem leaks of recent object store conversions.

[Stefan Beller]

This series replaces the two commits that were queued on sb/object-store-replace,
fixing memory leaks that were recently introduced.

Compared to v1, I merged the two independent series from yesterday,
rewrote the commit message to clear up Junios confusion and addresses Peffs
comments for the packfiles as well.
Also added another free to free the oidmap for the replaces themselves.

Thanks,
Stefan

Stefan Beller (4):
  packfile: close and free packs upon releasing an object store
  packfile.h: remove all extern keywords
  object.c: free replace map in raw_object_store_clear
  replace-object.c: remove the_repository from prepare_replace_object

 object.c         |  7 +++--
 packfile.c       | 13 ++++++++
 packfile.h       | 79 ++++++++++++++++++++++++------------------------
 replace-object.c |  2 +-
 4 files changed, 58 insertions(+), 43 deletions(-)

-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 1/4] packfile: close and free packs upon releasing an object store

[Stefan Beller]

In d0b59866223 (object-store: close all packs upon clearing the object
store, 2018-03-23), we made sure to close all packfiles on releasing
an object store, but we also have to free the memory of the closed packs.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    > Should that INIT_LIST_HEAD get moved down into that function?
    
    done.
    
    > Probably the same applies to setting NULL here; you're left with a
    > dangling pointer if you just call close_and_free_packs(). Should
    > that helper maybe just be a static function in object.c?
    
    I just realize that
    
    	while (o->packed_git) {
    		...
    		o->packed_git = p->next;
    		...
    	}
    
    will make sure that o->packed_git is NULL afterwards,
    hence I removed the explicit set to NULL in object.c
    and we rely on the code in replace-object.c
    
    Thanks,
    Stefan

 object.c   |  4 +---
 packfile.c | 13 +++++++++++++
 packfile.h |  1 +
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/object.c b/object.c
index 66cffaf6e51..242d922d953 100644
--- a/object.c
+++ b/object.c
@@ -484,7 +484,5 @@ void raw_object_store_clear(struct raw_object_store *o)
 	free_alt_odbs(o);
 	o->alt_odb_tail = NULL;
 
-	INIT_LIST_HEAD(&o->packed_git_mru);
-	close_all_packs(o);
-	o->packed_git = NULL;
+	close_and_free_packs(o);
 }
diff --git a/packfile.c b/packfile.c
index 6c3ddc3c31d..7f2a9e28a2b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -322,6 +322,19 @@ void close_all_packs(struct raw_object_store *o)
 			close_pack(p);
 }
 
+void close_and_free_packs(struct raw_object_store *o)
+{
+	close_all_packs(o);
+
+	INIT_LIST_HEAD(&o->packed_git_mru);
+
+	while (o->packed_git) {
+		struct packed_git *p = o->packed_git;
+		o->packed_git = p->next;
+		free(p);
+	}
+}
+
 /*
  * The LRU pack is the one with the oldest MRU window, preferring packs
  * with no used windows, or the oldest mtime if it has no windows allocated.
diff --git a/packfile.h b/packfile.h
index 9c2f8859945..cdab0557979 100644
--- a/packfile.h
+++ b/packfile.h
@@ -67,6 +67,7 @@ extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t
 extern void close_pack_windows(struct packed_git *);
 extern void close_pack(struct packed_git *);
 extern void close_all_packs(struct raw_object_store *o);
+extern void close_and_free_packs(struct raw_object_store *o);
 extern void unuse_pack(struct pack_window **);
 extern void clear_delta_base_cache(void);
 extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 2/4] packfile.h: remove all extern keywords

[Stefan Beller]

Per our coding guidelines we prefer to only use the extern keyword
when needed.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 packfile.h | 80 +++++++++++++++++++++++++++---------------------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/packfile.h b/packfile.h
index cdab0557979..eb3b1154501 100644
--- a/packfile.h
+++ b/packfile.h
@@ -10,32 +10,32 @@
  *
  * Example: odb_pack_name(out, sha1, "idx") => ".git/objects/pack/pack-1234..idx"
  */
-extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);
+char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);
 
 /*
  * Return the name of the (local) packfile with the specified sha1 in
  * its name.  The return value is a pointer to memory that is
  * overwritten each time this function is called.
  */
-extern char *sha1_pack_name(const unsigned char *sha1);
+char *sha1_pack_name(const unsigned char *sha1);
 
 /*
  * Return the name of the (local) pack index file with the specified
  * sha1 in its name.  The return value is a pointer to memory that is
  * overwritten each time this function is called.
  */
-extern char *sha1_pack_index_name(const unsigned char *sha1);
+char *sha1_pack_index_name(const unsigned char *sha1);
 
-extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
 /* A hook to report invalid files in pack directory */
 #define PACKDIR_FILE_PACK 1
 #define PACKDIR_FILE_IDX 2
 #define PACKDIR_FILE_GARBAGE 4
-extern void (*report_garbage)(unsigned seen_bits, const char *path);
+void (*report_garbage)(unsigned seen_bits, const char *path);
 
-extern void reprepare_packed_git(struct repository *r);
-extern void install_packed_git(struct repository *r, struct packed_git *pack);
+void reprepare_packed_git(struct repository *r);
+void install_packed_git(struct repository *r, struct packed_git *pack);
 
 struct packed_git *get_packed_git(struct repository *r);
 struct list_head *get_packed_git_mru(struct repository *r);
@@ -46,31 +46,31 @@ struct list_head *get_packed_git_mru(struct repository *r);
  */
 unsigned long approximate_object_count(void);
 
-extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
+struct packed_git *find_sha1_pack(const unsigned char *sha1,
 					 struct packed_git *packs);
 
-extern void pack_report(void);
+void pack_report(void);
 
 /*
  * mmap the index file for the specified packfile (if it is not
  * already mmapped).  Return 0 on success.
  */
-extern int open_pack_index(struct packed_git *);
+int open_pack_index(struct packed_git *);
 
 /*
  * munmap the index file for the specified packfile (if it is
  * currently mmapped).
  */
-extern void close_pack_index(struct packed_git *);
+void close_pack_index(struct packed_git *);
 
-extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);
-extern void close_pack_windows(struct packed_git *);
-extern void close_pack(struct packed_git *);
-extern void close_all_packs(struct raw_object_store *o);
-extern void close_and_free_packs(struct raw_object_store *o);
-extern void unuse_pack(struct pack_window **);
-extern void clear_delta_base_cache(void);
-extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);
+unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);
+void close_pack_windows(struct packed_git *);
+void close_pack(struct packed_git *);
+void close_all_packs(struct raw_object_store *o);
+void close_and_free_packs(struct raw_object_store *o);
+void unuse_pack(struct pack_window **);
+void clear_delta_base_cache(void);
+struct packed_git *add_packed_git(const char *path, size_t path_len, int local);
 
 /*
  * Make sure that a pointer access into an mmap'd index file is within bounds,
@@ -80,7 +80,7 @@ extern struct packed_git *add_packed_git(const char *path, size_t path_len, int
  * (like the 64-bit extended offset table), as we compare the size to the
  * fixed-length parts when we open the file.
  */
-extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);
+void check_pack_index_ptr(const struct packed_git *p, const void *ptr);
 
 /*
  * Perform binary search on a pack-index for a given oid. Packfile is expected to
@@ -96,51 +96,51 @@ int bsearch_pack(const struct object_id *oid, const struct packed_git *p, uint32
  * at the SHA-1 within the mmapped index.  Return NULL if there is an
  * error.
  */
-extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);
+const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);
 /*
  * Like nth_packed_object_sha1, but write the data into the object specified by
  * the the first argument.  Returns the first argument on success, and NULL on
  * error.
  */
-extern const struct object_id *nth_packed_object_oid(struct object_id *, struct packed_git *, uint32_t n);
+const struct object_id *nth_packed_object_oid(struct object_id *, struct packed_git *, uint32_t n);
 
 /*
  * Return the offset of the nth object within the specified packfile.
  * The index must already be opened.
  */
-extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);
+off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);
 
 /*
  * If the object named sha1 is present in the specified packfile,
  * return its offset within the packfile; otherwise, return 0.
  */
-extern off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);
+off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);
 
-extern int is_pack_valid(struct packed_git *);
-extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);
-extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
-extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
-extern int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);
+int is_pack_valid(struct packed_git *);
+void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);
+unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
+unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
+int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);
 
-extern void release_pack_memory(size_t);
+void release_pack_memory(size_t);
 
 /* global flag to enable extra checks when accessing packed objects */
-extern int do_check_packed_object_crc;
+int do_check_packed_object_crc;
 
-extern int packed_object_info(struct packed_git *pack, off_t offset, struct object_info *);
+int packed_object_info(struct packed_git *pack, off_t offset, struct object_info *);
 
-extern void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);
-extern const struct packed_git *has_packed_and_bad(const unsigned char *sha1);
+void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);
+const struct packed_git *has_packed_and_bad(const unsigned char *sha1);
 
 /*
  * Iff a pack file in the given repository contains the object named by sha1,
  * return true and store its location to e.
  */
-extern int find_pack_entry(struct repository *r, const unsigned char *sha1, struct pack_entry *e);
+int find_pack_entry(struct repository *r, const unsigned char *sha1, struct pack_entry *e);
 
-extern int has_sha1_pack(const unsigned char *sha1);
+int has_sha1_pack(const unsigned char *sha1);
 
-extern int has_pack_index(const unsigned char *sha1);
+int has_pack_index(const unsigned char *sha1);
 
 /*
  * Only iterate over packs obtained from the promisor remote.
@@ -156,13 +156,13 @@ typedef int each_packed_object_fn(const struct object_id *oid,
 				  struct packed_git *pack,
 				  uint32_t pos,
 				  void *data);
-extern int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn, void *data);
-extern int for_each_packed_object(each_packed_object_fn, void *, unsigned flags);
+int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn, void *data);
+int for_each_packed_object(each_packed_object_fn, void *, unsigned flags);
 
 /*
  * Return 1 if an object in a promisor packfile is or refers to the given
  * object, 0 otherwise.
  */
-extern int is_promisor_object(const struct object_id *oid);
+int is_promisor_object(const struct object_id *oid);
 
 #endif
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 3/4] object.c: free replace map in raw_object_store_clear

[Stefan Beller]

The replace map for objects was missed to free in the object store in
the conversion of c1274495 ("replace-object: eliminate replace objects
prepared flag", 2018-04-11). We also missed to free the replaced objects
that are put into the replace map in that whole series.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 object.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/object.c b/object.c
index 242d922d953..80d1cae53c0 100644
--- a/object.c
+++ b/object.c
@@ -481,6 +481,9 @@ void raw_object_store_clear(struct raw_object_store *o)
 	FREE_AND_NULL(o->objectdir);
 	FREE_AND_NULL(o->alternate_db);
 
+	oidmap_free(o->replace_map, 1);
+	FREE_AND_NULL(o->replace_map);
+
 	free_alt_odbs(o);
 	o->alt_odb_tail = NULL;
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 4/4] replace-object.c: remove the_repository from prepare_replace_object

[Stefan Beller]

This was missed in 5982da9d2ce (replace-object: allow
prepare_replace_object to handle arbitrary repositories, 2018-04-11)

Technically the code works correctly as the replace_map is the same
size in different repositories, however it is hard to read. So convert
the code to the familiar pattern of dereferencing the pointer that we
assign in the sizeof itself.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 replace-object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/replace-object.c b/replace-object.c
index 246b98cd4f1..801b5c16789 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -37,7 +37,7 @@ static void prepare_replace_object(struct repository *r)
 		return;
 
 	r->objects->replace_map =
-		xmalloc(sizeof(*the_repository->objects->replace_map));
+		xmalloc(sizeof(*r->objects->replace_map));
 	oidmap_init(r->objects->replace_map, 0);
 
 	for_each_replace_ref(r, register_replace_ref, NULL);
-- 
2.17.0.255.g8bfb7c0704

Re: [PATCH v2 0/4] Fix mem leaks of recent object store conversions.

[Jeff King]

On Thu, May 10, 2018 at 12:58:45PM -0700, Stefan Beller wrote:

> This series replaces the two commits that were queued on sb/object-store-replace,
> fixing memory leaks that were recently introduced.
> 
> Compared to v1, I merged the two independent series from yesterday,
> rewrote the commit message to clear up Junios confusion and addresses Peffs
> comments for the packfiles as well.

Mostly. :)

My one remaining complaint is that the bitmap code may hold on to a
dangling pointer to a packed_git after this series.

I think that is part of a larger problem, though, which is that the
bitmap code's globals need to be part of the struct raw_object_store.
I think this can already cause problems before your series if we were to
try to use bitmaps in both a superproject and a submodule in the same
process, though I think we'd at least hit the "ignoring extra bitmap
file" code path in open_pack_bitmap_1(). So right now it's an annoyance,
but after your series it becomes a potential segfault.

-Peff

Re: [PATCH v2 0/4] Fix mem leaks of recent object store conversions.

[Stefan Beller]

On Fri, May 11, 2018 at 1:37 AM, Jeff King <peff@peff.net> wrote:
> On Thu, May 10, 2018 at 12:58:45PM -0700, Stefan Beller wrote:
>
>> This series replaces the two commits that were queued on sb/object-store-replace,
>> fixing memory leaks that were recently introduced.
>>
>> Compared to v1, I merged the two independent series from yesterday,
>> rewrote the commit message to clear up Junios confusion and addresses Peffs
>> comments for the packfiles as well.
>
> Mostly. :)
>
> My one remaining complaint is that the bitmap code may hold on to a
> dangling pointer to a packed_git after this series.

Ok, I'll look into that.

>
> I think that is part of a larger problem, though, which is that the
> bitmap code's globals need to be part of the struct raw_object_store.
> I think this can already cause problems before your series if we were to
> try to use bitmaps in both a superproject and a submodule in the same
> process, though I think we'd at least hit the "ignoring extra bitmap
> file" code path in open_pack_bitmap_1(). So right now it's an annoyance,
> but after your series it becomes a potential segfault.

Ok, maybe we'll need to convert bitmaps into the object store for that.

Thanks for the pointer,
Stefan