[PATCH 06/17] fs/squashfs: sqfs_read_directory_table: fix memory leak

[Richard Genoud]

Hi Miquel !
Thanks for your feedback.

Le 15/10/2020 à 15:54, Miquel Raynal a écrit :
> Hi Richard,
> 
> Richard Genoud <richard.genoud at posteo.net> wrote on Wed, 14 Oct 2020
> 10:06:11 +0200:
> 
>> pos_list wasn't freed on every error
>>
>> Signed-off-by: Richard Genoud <richard.genoud at posteo.net>
> 
> Same comment here (and probably after as well) as in patch 05/17, not
> sure this is actually relevant for the community but I prefer this:
> 
> 	bar = malloc();
> 	...
> 	if (ret)
> 		goto free_bar;
> 
> 	foo = malloc();
> 	...
> 	if (ret)
> 		goto free foo;
> 
> 	...
> 
> 	foo:
> 		kfree(foo);
> 	bar:
> 		kfree(bar);
> 
> than:
> 
> 	foo = NULL;
> 	bar = NULL;
> 
> 	...
> 	if (ret)
> 		goto out;
> 	...
> 	if (ret)
> 		goto out;
> 	...
> 		out:
> 	if (ret)
> 		kfree(...)

I guess it's a coding habit.
I personnaly prefer the later because I think it's less error-prone :
When moving code aroung, we don't have to move the labels and rename
the gotos.
Ex:
Let's say we have this code:
	bar = malloc();
	...
	if (ret)
		goto free_bar;

	foo = malloc();
	...
	if (ret)
		goto free_foo;
	ret = init_somthing();
	if (ret)
		goto free_foo;
	ret = dummy()
	if (ret)
		goto free_foo;

	...

	foo:
		kfree(foo);
	bar:
		kfree(bar);

And, we want to move, for whatever reason, init_something() and dummy()
before the foo allocation. We will have to change the code to:

	bar = malloc();
	...
	if (ret)
		goto free_bar;
	ret = init_somthing();
	if (ret)
		goto free_bar; // not free_foo anymore !
	ret = dummy()
	if (ret)
		goto free_bar; // ditto

	foo = malloc();
	...
	if (ret)
		goto free_foo;
	...

	foo:
		kfree(foo);
	bar:
		kfree(bar);

Worse, if we have to exchange bar and foo allocation, we'll also have
to exchange the deallocation of foo and bar and change all gotos beneath :
	foo = malloc();
	...
	if (ret)
		goto free_foo;

	bar = malloc();
	...
	if (ret)
		goto free_bar;

	ret = init_somthing();
	if (ret)
		goto free_foo; // not free_foo anymore
	ret = dummy()
	if (ret)
		goto free_foo; //ditto


	...

// oops ! we have to exchange that !
	foo:
		kfree(foo);
	bar:
		kfree(bar);


That's why I prefer only one label and setting NULL.
If I didn't convince you, I'll change it back to multiple labels :)

> 
>> ---
>>   fs/squashfs/sqfs.c | 31 +++++++++++++++++--------------
>>   1 file changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
>> index 55d183663a8..c4d74fd4d6d 100644
>> --- a/fs/squashfs/sqfs.c
>> +++ b/fs/squashfs/sqfs.c
>> @@ -722,6 +722,8 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list)
>>   	unsigned long dest_len = 0;
>>   	bool compressed;
>>   
>> +	*dir_table = NULL;
>> +	*pos_list = NULL;
>>   	/* DIRECTORY TABLE */
>>   	table_size = get_unaligned_le64(&sblk->fragment_table_start) -
>>   		get_unaligned_le64(&sblk->directory_table_start);
>> @@ -736,35 +738,31 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list)
>>   		return -ENOMEM;
>>   
>>   	if (sqfs_disk_read(start, n_blks, dtb) < 0)
>> -		goto free_dtb;
>> +		goto out;
>>   
>>   	/* Parse directory table (metadata block) header */
>>   	ret = sqfs_read_metablock(dtb, table_offset, &compressed, &src_len);
>>   	if (ret)
>> -		goto free_dtb;
>> +		goto out;
>>   
>>   	/* Calculate total size to store the whole decompressed table */
>>   	metablks_count = sqfs_count_metablks(dtb, table_offset, table_size);
>>   	if (metablks_count < 1)
>> -		goto free_dtb;
>> +		goto out;
>>   
>>   	*dir_table = malloc(metablks_count * SQFS_METADATA_BLOCK_SIZE);
>>   	if (!*dir_table)
>> -		goto free_dtb;
>> +		goto out;
>>   
>>   	*pos_list = malloc(metablks_count * sizeof(u32));
>> -	if (!*pos_list) {
>> -		free(*dir_table);
>> -		goto free_dtb;
>> -	}
>> +	if (!*pos_list)
>> +		goto out;
>>   
>>   	ret = sqfs_get_metablk_pos(*pos_list, dtb, table_offset,
>>   				   metablks_count);
>>   	if (ret) {
>>   		metablks_count = -1;
>> -		free(*dir_table);
>> -		free(*pos_list);
>> -		goto free_dtb;
>> +		goto out;
>>   	}
>>   
>>   	src_table = dtb + table_offset + SQFS_HEADER_SIZE;
>> @@ -780,8 +778,7 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list)
>>   					      &dest_len, src_table, src_len);
>>   			if (ret) {
>>   				metablks_count = -1;
>> -				free(*dir_table);
>> -				goto free_dtb;
>> +				goto out;
>>   			}
>>   
>>   			if (dest_len < SQFS_METADATA_BLOCK_SIZE) {
>> @@ -803,7 +800,13 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list)
>>   		src_table += src_len + SQFS_HEADER_SIZE;
>>   	}
>>   
>> -free_dtb:
>> +out:
>> +	if (metablks_count < 1) {
>> +		free(*dir_table);
>> +		free(*pos_list);
>> +		*dir_table = NULL;
>> +		*pos_list = NULL;
>> +	}
>>   	free(dtb);
>>   
>>   	return metablks_count;
> 
> Thanks,
> Miquèl
> 
Thanks !
Richard