diff mbox series

[09/12] mailinfo: also free strbuf lists when clearing mailinfo

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

Commit Message

Andrzej Hunt April 9, 2021, 6:47 p.m. UTC 
From: Andrzej Hunt <ajrhunt@google.com>

mailinfo.p_hdr_info/s_hdr_info are null-terminated lists of strbuf's,
with entries pointing either to NULL or an allocated strbuf. Therefore
we need to free those strbuf's (and not just the data they contain)
whenever we're done with a given entry. (See handle_header() where those
new strbufs are malloc'd.)

Once we no longer need the list (and not just its entries) we can switch
over to strbuf_list_free() instead of manually iterating over the list,
which takes care of those additional details for us. We can only do this
in clear_mailinfo() - in handle_commit_message() we are only clearing the
array contents but want to reuse the array itself, hence we can't use
strbuf_list_free() there.

Leak output from t0023:

Direct leak of 72 byte(s) in 3 object(s) allocated from:
    #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9ac9f4 in do_xmalloc wrapper.c:41:8
    #2 0x9ac9ca in xmalloc wrapper.c:62:9
    #3 0x7f6cf7 in handle_header mailinfo.c:205:10
    #4 0x7f5abf in check_header mailinfo.c:583:4
    #5 0x7f5524 in mailinfo mailinfo.c:1197:3
    #6 0x4dcc95 in parse_mail builtin/am.c:1167:6
    #7 0x4d9070 in am_run builtin/am.c:1732:12
    #8 0x4d5b7a in cmd_am builtin/am.c:2398:3
    #9 0x4cd91d in run_builtin git.c:467:11
    #10 0x4cb5f3 in handle_builtin git.c:719:3
    #11 0x4ccf47 in run_argv git.c:808:4
    #12 0x4caf49 in cmd_main git.c:939:19
    #13 0x69e43e in main common-main.c:52:11
    #14 0x7fc1fadfa349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 72 byte(s) leaked in 3 allocation(s).

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
 mailinfo.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

Comments

Junio C Hamano April 11, 2021, 11:43 a.m. UTC | #1 
"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);
        }
Andrzej Hunt April 25, 2021, 1:15 p.m. UTC | #2 
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 mbox series

Patch

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