Re: [PATCH v3 00/27] Memory leak fixes (pt.10, final)

[Toon Claes <toon@iotcl.com>]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this is the last part of my series of memory leak fixes. This series
> goes a bit further than past series:
>
>   - Patches 1 to 16 plug remaining memory leaks exposed by our test
>     suite.
>
>   - Patches 17 to 22 remove the last remaining `UNLEAK()` annotations
>     and ultimately remove the macro itself.
>
>   - Patch 23 works around a bug in the leak sanitizer itself.
>
>   - Patches 24 and 25 drop annotations where leak-free tests pass with
>     the leak sanitizer.
>
>   - Patches 26 and 27 unconditionally enable leak checking in all newly
>     added tests and then drop the `TEST_PASSES_SANITIZE_LEAK`
>     annotation.
>
> So once this series lands, the expectation is that any newly added test
> needs to be leak free by default. We still have an escape hatch in the
> form of the SANITIZE_LEAK prerequisite, but patch authors are expected
> to provide good arguments why their test cannot be made leak free.
>
> Changes in v3:
>
>   - Fix bounds checking in `strvec_splice()`.
>
>   - Rename `pos` to `idx` in `strvec_splice()`.
>
>   - Drop no-op code when finding bisection points with
>     `FIND_BISECT_ALL`.
>
> Link to v1: https://lore.kernel.org/r/cover.1730901926.git.ps@pks.im
> Link to v2: https://lore.kernel.org/r/20241111-b4-pks-leak-fixes-pt10-v2-0-6154bf91f0b0@pks.im
>
> Thanks!
>
> Patrick
>
> [...snip...]
>
> Range-diff versus v2:
>
>  1:  89f354b667 =  1:  08acbe8895 builtin/blame: fix leaking blame entries with `--incremental`
>  2:  e50952aba3 =  2:  49269d747b bisect: fix leaking good/bad terms when reading multipe times
>  3:  c38a4e15b8 =  3:  83cd85b609 bisect: fix leaking string in `handle_bad_merge_base()`
>  4:  d2b48a08c2 =  4:  88a218045c bisect: fix leaking `current_bad_oid`
>  5:  496421e0fe =  5:  1c1f77497f bisect: fix multiple leaks in `bisect_next_all()`
>  6:  aeb9cacf64 =  6:  f02bbfde18 bisect: fix leaking commit list items in `check_merge_base()`
>  7:  a8afd12467 !  7:  6e6d490b8a bisect: fix various cases where we leak commit list items
>     @@ Commit message
>      
>       ## bisect.c ##
>      @@ bisect.c: void find_bisection(struct commit_list **commit_list, int *reaches,
>     - 			free_commit_list(list->next);
>     - 			best = list;
>       			best->next = NULL;
>     -+		} else {
>     -+			for (p = list; p != best; p = next) {
>     -+				next = p->next;
>     -+				free(p);
>     -+			}
>       		}
>       		*reaches = weight(best);
>      +	} else {
>  8:  f503331f08 =  8:  13e2158c23 line-log: fix leak when rewriting commit parents
>  9:  3edb9e0fe9 !  9:  3a9b0fa44a strvec: introduce new `strvec_splice()` function
>     @@ strvec.c: void strvec_pushv(struct strvec *array, const char **items)
>       		strvec_push(array, *items);
>       }
>       
>     -+void strvec_splice(struct strvec *array, size_t pos, size_t len,
>     ++void strvec_splice(struct strvec *array, size_t idx, size_t len,
>      +		   const char **replacement, size_t replacement_len)
>      +{
>     -+	if (pos + len > array->alloc)
>     ++	if (idx + len > array->nr)
>      +		BUG("range outside of array boundary");
>      +	if (replacement_len > len)
>      +		ALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,
>      +			   array->alloc);
>      +	for (size_t i = 0; i < len; i++)
>     -+		free((char *)array->v[pos + i]);
>     ++		free((char *)array->v[idx + i]);
>      +	if (replacement_len != len) {
>     -+		memmove(array->v + pos + replacement_len, array->v + pos + len,
>     -+			(array->nr - pos - len + 1) * sizeof(char *));
>     ++		memmove(array->v + idx + replacement_len, array->v + idx + len,
>     ++			(array->nr - idx - len + 1) * sizeof(char *));
>      +		array->nr += (replacement_len - len);
>      +	}
>      +	for (size_t i = 0; i < replacement_len; i++)
>     -+		array->v[pos + i] = xstrdup(replacement[i]);
>     ++		array->v[idx + i] = xstrdup(replacement[i]);
>      +}
>      +
>       const char *strvec_replace(struct strvec *array, size_t idx, const char *replacement)
>     @@ strvec.h: void strvec_pushl(struct strvec *, ...);
>       void strvec_pushv(struct strvec *, const char **);
>       
>      +/*
>     -+ * Replace `len` values starting at `pos` with the provided replacement
>     -+ * strings. If `len` is zero this is effectively an insert at the given `pos`.
>     ++ * Replace `len` values starting at `idx` with the provided replacement
>     ++ * strings. If `len` is zero this is effectively an insert at the given `idx`.
>      + * If `replacement_len` is zero this is effectively a delete of `len` items
>     -+ * starting at `pos`.
>     ++ * starting at `idx`.
>      + */
>     -+void strvec_splice(struct strvec *array, size_t pos, size_t len,
>     ++void strvec_splice(struct strvec *array, size_t idx, size_t len,
>      +		   const char **replacement, size_t replacement_len);
>      +
>       /**
> 10:  f4c5b4b029 = 10:  0e30d61ee3 git: refactor alias handling to use a `struct strvec`
> 11:  a30376077d = 11:  9dc3f5da99 git: refactor builtin handling to use a `struct strvec`
> 12:  6fc481018e = 12:  6a7bb2d4ec split-index: fix memory leak in `move_cache_to_base_index()`
> 13:  011ee82856 = 13:  5147a45e70 builtin/sparse-checkout: fix leaking sanitized patterns
> 14:  41c6aa41ba = 14:  1e1f0025e2 help: refactor to not use globals for reading config
> 15:  d2b9042512 = 15:  92fb58121b help: fix leaking `struct cmdnames`
> 16:  75228ba160 = 16:  a8f1cb44f8 help: fix leaking return value from `help_unknown_cmd()`
> 17:  f2da0c4825 = 17:  9e76f20ad5 builtin/help: fix leaks in `check_git_cmd()`
> 18:  fb369114b7 = 18:  2d0fa8a922 builtin/init-db: fix leaking directory paths
> 19:  bf7e34819e = 19:  598e385ad3 builtin/branch: fix leaking sorting options
> 20:  ec0f1d5f44 = 20:  469adc7754 t/helper: fix leaking commit graph in "read-graph" subcommand
> 21:  fad027056e = 21:  fe36f95eee global: drop `UNLEAK()` annotation
> 22:  dc9b641e6a = 22:  3898a90c35 git-compat-util: drop now-unused `UNLEAK()` macro
> 23:  1a1d34e9d3 = 23:  52e17f51cb t5601: work around leak sanitizer issue
> 24:  3d89a0c792 = 24:  972a56f3d5 t: mark some tests as leak free
> 25:  d0925d3731 = 25:  2cad683eab t: remove unneeded !SANITIZE_LEAK prerequisites
> 26:  b9f4007910 = 26:  de43715991 test-lib: unconditionally enable leak checking
> 27:  96313e3e47 = 27:  59637d5fea t: remove TEST_PASSES_SANITIZE_LEAK annotations
>
> ---
> base-commit: b0c643d6a710e2b092902a3941655176b358bfd0
> change-id: 20241111-b4-pks-leak-fixes-pt10-a6fa657f4fac

Range-diff looks good to me, no further comments.

--
Toon