Re: [PATCH cifs-utils] mount.cifs.c: fix memory leaks in main func

[Zhiqiang Liu <liuzhiqiang26@huawei.com>]

On 2019/8/1 17:15, Aurélien Aptel wrote:
> Hi Zhiqiang,
> 
> You are on the right list :)
> 
> Unfortunately it seems you have sent the exact same patch as before so
> I'll post my comments again:
> 
Thanks for your reply.
I have just missed your mail with comments. Sorry for that.
I will modify the patch as your suggestion, then post the v2 patch.

> Zhiqiang Liu <liuzhiqiang26@huawei.com> writes:
>> index ae7a899..029f01a 100644
>> --- a/mount.cifs.c
>> +++ b/mount.cifs.c
>> @@ -1830,6 +1830,7 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
>>  	}
>>
>>  assemble_exit:
>> +	free(orgoptions);
>>  	return rc;
>>  }
> 
> Since orgoptions is allocated in main() you should also free it
> there. In fact it is already freed there so the return have to be
> changed to goto.
> 
>>
>> @@ -1994,8 +1995,11 @@ int main(int argc, char **argv)
>>
>>  	/* chdir into mountpoint as soon as possible */
>>  	rc = acquire_mountpoint(&mountpoint);
>> -	if (rc)
>> +	if (rc) {
>> +		free(mountpoint);
>> +		free(orgoptions);
>>  		return rc;
>> +	}
> 
> Since mountpoint is allocated in acquire_mountpoint() you should free it
> there if there's an error.
> 
>>  	/*
>>  	 * mount.cifs does privilege separation. Most of the code to handle
>> @@ -2014,6 +2018,7 @@ int main(int argc, char **argv)
>>  		/* child */
>>  		rc = assemble_mountinfo(parsed_info, thisprogram, mountpoint,
>>  					orig_dev, orgoptions);
>> +		free(mountpoint);
> 
> Since this code block is only run by the child I think it's ok to not
> use goto. Don't forget to free(orgoptions) if you remove it from
> assemble_mountinfo()
> 
>>  		return rc;
>>  	} else {
>>  		/* parent */
>> @@ -2149,5 +2154,6 @@ mount_exit:
>>  	}
>>  	free(options);
>>  	free(orgoptions);
>> +	free(mountpoint);
> 
> Cheers,
>