read-cache: fix leaking hashfile when writing index fails
In `do_write_index()`, we use a `struct hashfile` to write the index with a trailer hash. In case the write fails though, we never clean up the allocated `hashfile` state and thus leak memory. Refactor the code to have a common exit path where we can free this and other allocated memory. While at it, refactor our use of `strbuf`s such that we reuse the same buffer to avoid some unneeded allocations. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Patrick Steinhardt committed
Aug 14, 2024 at 08:52 UTC
d1c53f6703f34eeb3442070016db6e4d1f5676d2
4 files changed
+62
-39
read-cache.c
+58
-39
@@ -2840,8 +2840,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
2840
int csum_fsync_flag;
2841
int ieot_entries = 1;
2842
struct index_entry_offset_table *ieot = NULL;
2843
- int nr, nr_threads;
2843
struct repository *r = istate->repo;
2844
+ struct strbuf sb = STRBUF_INIT;
2845
+ int nr, nr_threads, ret;
2846
2847
f = hashfd(tempfile->fd, tempfile->filename.buf);
2848
@@ -2962,8 +2963,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
2963
strbuf_release(&previous_name_buf);
2964
2965
if (err) {
2965
- free(ieot);
2966
- return err;
2966
+ ret = err;
2967
+ goto out;
2968
}
2969
2970
offset = hashfile_total(f);
@@ -2985,20 +2986,20 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
2986
* index.
2987
*/
2988
if (ieot) {
2988
- struct strbuf sb = STRBUF_INIT;
2989
+ strbuf_reset(&sb);
2990
2991
write_ieot_extension(&sb, ieot);
2992
err = write_index_ext_header(f, eoie_c, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0;
2993
hashwrite(f, sb.buf, sb.len);
2993
- strbuf_release(&sb);
2994
- free(ieot);
2995
- if (err)
2996
- return -1;
2994
+ if (err) {
2995
+ ret = -1;
2996
+ goto out;
2997
+ }
2998
}
2999
3000
if (write_extensions & WRITE_SPLIT_INDEX_EXTENSION &&
3001
istate->split_index) {
3001
- struct strbuf sb = STRBUF_INIT;
3002
+ strbuf_reset(&sb);
3003
3004
if (istate->sparse_index)
3005
die(_("cannot write split index for a sparse index"));
@@ -3007,59 +3008,66 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
3008
write_index_ext_header(f, eoie_c, CACHE_EXT_LINK,
3009
sb.len) < 0;
3010
hashwrite(f, sb.buf, sb.len);
3010
- strbuf_release(&sb);
3011
- if (err)
3012
- return -1;
3011
+ if (err) {
3012
+ ret = -1;
3013
+ goto out;
3014
+ }
3015
}
3016
if (write_extensions & WRITE_CACHE_TREE_EXTENSION &&
3017
!drop_cache_tree && istate->cache_tree) {
3016
- struct strbuf sb = STRBUF_INIT;
3018
+ strbuf_reset(&sb);
3019
3020
cache_tree_write(&sb, istate->cache_tree);
3021
err = write_index_ext_header(f, eoie_c, CACHE_EXT_TREE, sb.len) < 0;
3022
hashwrite(f, sb.buf, sb.len);
3021
- strbuf_release(&sb);
3022
- if (err)
3023
- return -1;
3023
+ if (err) {
3024
+ ret = -1;
3025
+ goto out;
3026
+ }
3027
}
3028
if (write_extensions & WRITE_RESOLVE_UNDO_EXTENSION &&
3029
istate->resolve_undo) {
3027
- struct strbuf sb = STRBUF_INIT;
3030
+ strbuf_reset(&sb);
3031
3032
resolve_undo_write(&sb, istate->resolve_undo);
3033
err = write_index_ext_header(f, eoie_c, CACHE_EXT_RESOLVE_UNDO,
3034
sb.len) < 0;
3035
hashwrite(f, sb.buf, sb.len);
3033
- strbuf_release(&sb);
3034
- if (err)
3035
- return -1;
3036
+ if (err) {
3037
+ ret = -1;
3038
+ goto out;
3039
+ }
3040
}
3041
if (write_extensions & WRITE_UNTRACKED_CACHE_EXTENSION &&
3042
istate->untracked) {
3039
- struct strbuf sb = STRBUF_INIT;
3043
+ strbuf_reset(&sb);
3044
3045
write_untracked_extension(&sb, istate->untracked);
3046
err = write_index_ext_header(f, eoie_c, CACHE_EXT_UNTRACKED,
3047
sb.len) < 0;
3048
hashwrite(f, sb.buf, sb.len);
3045
- strbuf_release(&sb);
3046
- if (err)
3047
- return -1;
3049
+ if (err) {
3050
+ ret = -1;
3051
+ goto out;
3052
+ }
3053
}
3054
if (write_extensions & WRITE_FSMONITOR_EXTENSION &&
3055
istate->fsmonitor_last_update) {
3051
- struct strbuf sb = STRBUF_INIT;
3056
+ strbuf_reset(&sb);
3057
3058
write_fsmonitor_extension(&sb, istate);
3059
err = write_index_ext_header(f, eoie_c, CACHE_EXT_FSMONITOR, sb.len) < 0;
3060
hashwrite(f, sb.buf, sb.len);
3056
- strbuf_release(&sb);
3057
- if (err)
3058
- return -1;
3061
+ if (err) {
3062
+ ret = -1;
3063
+ goto out;
3064
+ }
3065
}
3066
if (istate->sparse_index) {
3061
- if (write_index_ext_header(f, eoie_c, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0)
3062
- return -1;
3067
+ if (write_index_ext_header(f, eoie_c, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0) {
3068
+ ret = -1;
3069
+ goto out;
3070
+ }
3071
}
3072
3073
/*
@@ -3069,14 +3077,15 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
3077
* when loading the shared index.
3078
*/
3079
if (eoie_c) {
3072
- struct strbuf sb = STRBUF_INIT;
3080
+ strbuf_reset(&sb);
3081
3082
write_eoie_extension(&sb, eoie_c, offset);
3083
err = write_index_ext_header(f, NULL, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0;
3084
hashwrite(f, sb.buf, sb.len);
3077
- strbuf_release(&sb);
3078
- if (err)
3079
- return -1;
3085
+ if (err) {
3086
+ ret = -1;
3087
+ goto out;
3088
+ }
3089
}
3090
3091
csum_fsync_flag = 0;
@@ -3085,13 +3094,16 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
3094
3095
finalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_INDEX,
3096
CSUM_HASH_IN_STREAM | csum_fsync_flag);
3097
+ f = NULL;
3098
3099
if (close_tempfile_gently(tempfile)) {
3090
- error(_("could not close '%s'"), get_tempfile_path(tempfile));
3091
- return -1;
3100
+ ret = error(_("could not close '%s'"), get_tempfile_path(tempfile));
3101
+ goto out;
3102
+ }
3103
+ if (stat(get_tempfile_path(tempfile), &st)) {
3104
+ ret = -1;
3105
+ goto out;
3106
}
3093
- if (stat(get_tempfile_path(tempfile), &st))
3094
- return -1;
3107
istate->timestamp.sec = (unsigned int)st.st_mtime;
3108
istate->timestamp.nsec = ST_MTIME_NSEC(st);
3109
trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed);
@@ -3105,7 +3117,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
3117
trace2_data_intmax("index", the_repository, "write/cache_nr",
3118
istate->cache_nr);
3119
3108
- return 0;
3120
+ ret = 0;
3121
+
3122
+out:
3123
+ if (f)
3124
+ free_hashfile(f);
3125
+ strbuf_release(&sb);
3126
+ free(ieot);
3127
+ return ret;
3128
}
3129
3130
void set_alternate_index_output(const char *name)
t/t1601-index-bogus.sh
+2
@@ -1,6 +1,8 @@
1
#!/bin/sh
2
3
test_description='test handling of bogus index entries'
4
+
5
+TEST_PASSES_SANITIZE_LEAK=true
6
. ./test-lib.sh
7
8
test_expect_success 'create tree with null sha1' '
t/t2107-update-index-basic.sh
+1
@@ -5,6 +5,7 @@ test_description='basic update-index tests
5
Tests for command-line parsing and basic operation.
6
'
7
8
+TEST_PASSES_SANITIZE_LEAK=true
9
. ./test-lib.sh
10
11
test_expect_success 'update-index --nonsense fails' '
t/t7008-filter-branch-null-sha1.sh
+1
@@ -2,6 +2,7 @@
2
3
test_description='filter-branch removal of trees with null sha1'
4
5
+TEST_PASSES_SANITIZE_LEAK=true
6
. ./test-lib.sh
7
8
test_expect_success 'setup: base commits' '