Re: [PATCH 01/20] mailinfo: fix leaking header data
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 20, 2024, 19:25 UTC
- Message-ID
- <xmqq5xrv8099.fsf@gitster.g>
- In-Reply-To
- <69e30ea5179eff6472be54ebba64ebca3e562f32.1724159575.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 9 quoted lines
> We populate the `mailinfo` arrays `p_hdr_data` and `s_hdr_data` with > data parsed from the mail headers. These arrays may end up being only > partially populated with gaps in case some of the headers do not parse > properly. This causes memory leaks because `strbuf_list_free()` will > stop iterating once it hits the first `NULL` pointer in the backing > array. > > Fix this by open-coding a variant of `strbuf_list_free()` that knows to > iterate through all headers.
Well spotted. An array of strbuf is often a wrong data structure for anything, but luckily we use it and suffer from a bug like this only rarely.
The fix makes sense. Will queue.
Show 47 quoted lines
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> mailinfo.c | 17 +++++++++++++++--
> t/t5100-mailinfo.sh | 1 +
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/mailinfo.c b/mailinfo.c
> index 94b9b0abf22..a4fa64994ac 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -1290,8 +1290,21 @@ void clear_mailinfo(struct mailinfo *mi)
> strbuf_release(&mi->inbody_header_accum);
> free(mi->message_id);
>
> - strbuf_list_free(mi->p_hdr_data);
> - strbuf_list_free(mi->s_hdr_data);
> + for (size_t i = 0; header[i]; i++) {
> + if (!mi->p_hdr_data[i])
> + continue;
> + strbuf_release(mi->p_hdr_data[i]);
> + free(mi->p_hdr_data[i]);
> + }
> + free(mi->p_hdr_data);
> +
> + for (size_t i = 0; header[i]; i++) {
> + if (!mi->s_hdr_data[i])
> + continue;
> + strbuf_release(mi->s_hdr_data[i]);
> + free(mi->s_hdr_data[i]);
> + }
> + free(mi->s_hdr_data);
>
> while (mi->content < mi->content_top) {
> free(*(mi->content_top));
> diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
> index c8d06554541..065156c1f39 100755
> --- a/t/t5100-mailinfo.sh
> +++ b/t/t5100-mailinfo.sh
> @@ -5,6 +5,7 @@
>
> test_description='git mailinfo and git mailsplit test'
>
> +TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
>
> DATA="$TEST_DIRECTORY/t5100"