Message ID | 130ef89218a47adc7ee558e75672e0e4eb5f30ca.1617994052.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Fix all leaks in tests t0002-t0099: Part 1 | expand |
"Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes: > void clear_mailinfo(struct mailinfo *mi) > { > - int i; > - > strbuf_release(&mi->name); > strbuf_release(&mi->email); > strbuf_release(&mi->charset); > strbuf_release(&mi->inbody_header_accum); > free(mi->message_id); > > - if (mi->p_hdr_data) > - for (i = 0; mi->p_hdr_data[i]; i++) > - strbuf_release(mi->p_hdr_data[i]); > - free(mi->p_hdr_data); > - if (mi->s_hdr_data) > - for (i = 0; mi->s_hdr_data[i]; i++) > - strbuf_release(mi->s_hdr_data[i]); > - free(mi->s_hdr_data); So, the original allows mi->p_hdr_data to be NULL and does not do this freeing (the same for the .s_hdr_data member). > + strbuf_list_free(mi->p_hdr_data); > + strbuf_list_free(mi->s_hdr_data); Is it safe to feed NULL to the helper? void strbuf_list_free(struct strbuf **sbs) { struct strbuf **s = sbs; while (*s) { strbuf_release(*s); free(*s++); } free(sbs); }
On 11/04/2021 13:43, Junio C Hamano wrote: > "Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> void clear_mailinfo(struct mailinfo *mi) >> { >> - int i; >> - >> strbuf_release(&mi->name); >> strbuf_release(&mi->email); >> strbuf_release(&mi->charset); >> strbuf_release(&mi->inbody_header_accum); >> free(mi->message_id); >> >> - if (mi->p_hdr_data) >> - for (i = 0; mi->p_hdr_data[i]; i++) >> - strbuf_release(mi->p_hdr_data[i]); >> - free(mi->p_hdr_data); >> - if (mi->s_hdr_data) >> - for (i = 0; mi->s_hdr_data[i]; i++) >> - strbuf_release(mi->s_hdr_data[i]); >> - free(mi->s_hdr_data); > > So, the original allows mi->p_hdr_data to be NULL and does not do > this freeing (the same for the .s_hdr_data member). > >> + strbuf_list_free(mi->p_hdr_data); >> + strbuf_list_free(mi->s_hdr_data); > > Is it safe to feed NULL to the helper? > > void strbuf_list_free(struct strbuf **sbs) > { > struct strbuf **s = sbs; > > while (*s) { > strbuf_release(*s); > free(*s++); > } > free(sbs); > } > > Indeed: AFAIUI dereferencing NULL is undefined behaviour. I think the best solution is to add a NULL check in strbuf_list_free() - which is the pattern I've seen in several other *_free() helpers (there are also quite a few examples of *_free() helpers that are not NULL safe, but IMHO having a NULL check will lead to fewer unpleasant surprises). Incidentally I did run the entire test-suite against UBSAN, and it didn't find any issues here. This seems like something that UBSAN should be able to easily catch, so we probably don't have any tests exercising clear_mailinfo() with NULL p_hdr_info/s_hdr_info?
diff --git a/mailinfo.c b/mailinfo.c index 5681d9130db6..95ce191f385b 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -821,7 +821,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) for (i = 0; header[i]; i++) { if (mi->s_hdr_data[i]) strbuf_release(mi->s_hdr_data[i]); - mi->s_hdr_data[i] = NULL; + FREE_AND_NULL(mi->s_hdr_data[i]); } return 0; } @@ -1236,22 +1236,14 @@ void setup_mailinfo(struct mailinfo *mi) void clear_mailinfo(struct mailinfo *mi) { - int i; - strbuf_release(&mi->name); strbuf_release(&mi->email); strbuf_release(&mi->charset); strbuf_release(&mi->inbody_header_accum); free(mi->message_id); - if (mi->p_hdr_data) - for (i = 0; mi->p_hdr_data[i]; i++) - strbuf_release(mi->p_hdr_data[i]); - free(mi->p_hdr_data); - if (mi->s_hdr_data) - for (i = 0; mi->s_hdr_data[i]; i++) - strbuf_release(mi->s_hdr_data[i]); - free(mi->s_hdr_data); + strbuf_list_free(mi->p_hdr_data); + strbuf_list_free(mi->s_hdr_data); while (mi->content < mi->content_top) { free(*(mi->content_top));