On 11/04/2021 13:43, Junio C Hamano wrote:
"Andrzej Hunt via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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?
