On 11/04/2021 13:43, Junio C Hamano wrote:

"Andrzej Hunt via GitGitGadget" [off-list ref] writes:

quoted

  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).

quoted

+	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?