| Message ID | 69e30ea5179eff6472be54ebba64ebca3e562f32.1724159575.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.5) | expand |
Patrick Steinhardt <ps@pks.im> writes: > 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. > > 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"
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"
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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- mailinfo.c | 17 +++++++++++++++-- t/t5100-mailinfo.sh | 1 + 2 files changed, 16 insertions(+), 2 deletions(-)