From 77c8ee74ff0f0ef4462fd6e963bc993c7a9721b3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Mar 2017 08:59:30 +0100 Subject: [PATCH 1/2] checkout: fix double-free of checkout_data's mkdir_map We currently call `git_strmap_free` on `checkout_data.mkdir_map` in the `checkout_data_clear` function. The only thing protecting us from a double-free is that the `git_strmap_free` function is in fact not a function, but a macro that also sets the map to NULL. Remove the second call to `git_strmap_free` and explicitly set the map member to NULL. --- src/checkout.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/checkout.c b/src/checkout.c index af600da6c..9d1eed59f 100644 --- a/src/checkout.c +++ b/src/checkout.c @@ -2319,8 +2319,6 @@ static void checkout_data_clear(checkout_data *data) git__free(data->pfx); data->pfx = NULL; - git_strmap_free(data->mkdir_map); - git_buf_free(&data->target_path); git_buf_free(&data->tmp); @@ -2328,6 +2326,7 @@ static void checkout_data_clear(checkout_data *data) data->index = NULL; git_strmap_free(data->mkdir_map); + data->mkdir_map = NULL; git_attr_session__free(&data->attr_session); } From 94af9155cf908f402e07408d900177280c2e90bb Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Mar 2017 09:01:18 +0100 Subject: [PATCH 2/2] map: remove `*map_free` macros The `map_free` functions were not implemented as functions but instead as macros which also set the map to NULL. While this is most certainly sensible in most cases, we should prefer the more obvious behavior, namingly leaving the map pointer intact. Furthermore, this macro has been refactored incorrectly during the map-refactorings: the two statements are not actually grouped together by a `do { ... } while (0)` block, as it is required for macros to match the behavior of functions more closely. This has led to at least one subtle nesting error in `pack-objects.c`. The following code block ``` if (pb->object_ix) git_oidmap_free(pb->object_ix); ``` would be expanded to ``` if (pb->object_ix) git_oidmap__free(pb->object_ix); pb->object_ix = NULL; ``` which is not what one woudl expect. While it is not a bug here as it would simply become a no-op, the wrong implementation could lead to bugs in other occasions. Fix this by simply removing the macro altogether and replacing it with real function calls. This leaves the burden of setting the pointer to NULL afterwards to the caller, but this is actually expected and behaves like other `free` functions. --- src/idxmap.c | 2 +- src/idxmap.h | 3 +-- src/offmap.c | 2 +- src/offmap.h | 3 +-- src/oidmap.c | 2 +- src/oidmap.h | 3 +-- src/strmap.c | 2 +- src/strmap.h | 4 +--- 8 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/idxmap.c b/src/idxmap.c index 10f55ad39..45f0c2204 100644 --- a/src/idxmap.c +++ b/src/idxmap.c @@ -99,7 +99,7 @@ void git_idxmap_icase_resize(git_idxmap_icase *map, size_t size) kh_resize(idxicase, map, size); } -void git_idxmap__free(git_idxmap *map) +void git_idxmap_free(git_idxmap *map) { kh_destroy(idx, map); } diff --git a/src/idxmap.h b/src/idxmap.h index e4e9ec4cd..dc702c36e 100644 --- a/src/idxmap.h +++ b/src/idxmap.h @@ -39,8 +39,7 @@ int git_idxmap_has_data(git_idxmap *map, size_t idx); void git_idxmap_resize(git_idxmap *map, size_t size); void git_idxmap_icase_resize(git_idxmap_icase *map, size_t size); -#define git_idxmap_free(h) git_idxmap__free(h); (h) = NULL -void git_idxmap__free(git_idxmap *map); +void git_idxmap_free(git_idxmap *map); void git_idxmap_clear(git_idxmap *map); void git_idxmap_delete_at(git_idxmap *map, size_t idx); diff --git a/src/offmap.c b/src/offmap.c index 023c9b417..ab6649697 100644 --- a/src/offmap.c +++ b/src/offmap.c @@ -14,7 +14,7 @@ git_offmap *git_offmap_alloc(void) return kh_init(off); } -void git_offmap__free(git_offmap *map) +void git_offmap_free(git_offmap *map) { kh_destroy(off, map); } diff --git a/src/offmap.h b/src/offmap.h index 1f30cda6d..f9d2483a6 100644 --- a/src/offmap.h +++ b/src/offmap.h @@ -21,8 +21,7 @@ __KHASH_TYPE(off, git_off_t, void *) typedef khash_t(off) git_offmap; git_offmap *git_offmap_alloc(void); -#define git_offmap_free(h) git_offmap__free(h); (h) = NULL -void git_offmap__free(git_offmap *map); +void git_offmap_free(git_offmap *map); void git_offmap_clear(git_offmap *map); size_t git_offmap_num_entries(git_offmap *map); diff --git a/src/oidmap.c b/src/oidmap.c index 0df9a4f42..5f156a18e 100644 --- a/src/oidmap.c +++ b/src/oidmap.c @@ -21,7 +21,7 @@ git_oidmap *git_oidmap_alloc() return kh_init(oid); } -void git_oidmap__free(git_oidmap *map) +void git_oidmap_free(git_oidmap *map) { kh_destroy(oid, map); } diff --git a/src/oidmap.h b/src/oidmap.h index a3f8961dc..563222494 100644 --- a/src/oidmap.h +++ b/src/oidmap.h @@ -21,8 +21,7 @@ __KHASH_TYPE(oid, const git_oid *, void *) typedef khash_t(oid) git_oidmap; git_oidmap *git_oidmap_alloc(void); -#define git_oidmap_free(h) git_oidmap__free(h); (h) = NULL -void git_oidmap__free(git_oidmap *map); +void git_oidmap_free(git_oidmap *map); void git_oidmap_clear(git_oidmap *map); size_t git_oidmap_size(git_oidmap *map); diff --git a/src/strmap.c b/src/strmap.c index 678d98344..de6826d03 100644 --- a/src/strmap.c +++ b/src/strmap.c @@ -19,7 +19,7 @@ int git_strmap_alloc(git_strmap **map) return 0; } -void git_strmap__free(git_strmap *map) +void git_strmap_free(git_strmap *map) { kh_destroy(str, map); } diff --git a/src/strmap.h b/src/strmap.h index 389702c9b..802b92494 100644 --- a/src/strmap.h +++ b/src/strmap.h @@ -21,9 +21,7 @@ typedef khash_t(str) git_strmap; typedef khiter_t git_strmap_iter; int git_strmap_alloc(git_strmap **map); - -#define git_strmap_free(h) git_strmap__free(h); (h) = NULL -void git_strmap__free(git_strmap *map); +void git_strmap_free(git_strmap *map); void git_strmap_clear(git_strmap *map); size_t git_strmap_num_entries(git_strmap *map);