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

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

[Zhiqiang Liu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb18030", Size: 1665 bytes --]

From: Jiawen Liu <liujiawen10@huawei.com>

In mount.cifs module, orgoptions and mountpoint in the main func
point to the memory allocated by func realpath and strndup respectively.
However, they are not freed before the main func returns so that the
memory leaks occurred.

The memory leak problem is reported by LeakSanitizer tool.
LeakSanitizer url: "https://github.com/google/sanitizers"

Here I free the pointers orgoptions and mountpoint before main
func returns.

Fixes£º7549ad5e7126 ("memory leaks: caused by func realpath and strndup")
Signed-off-by: Jiawen Liu <liujiawen10@huawei.com>
Reported-by: Jin Du <dujin1@huawei.com>
Reviewed-by: Saisai Zhang <zhangsaisai@huawei.com>
---
 mount.cifs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mount.cifs.c b/mount.cifs.c
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;
 }

@@ -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;
+	}

 	/*
 	 * 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);
 		return rc;
 	} else {
 		/* parent */
@@ -2149,5 +2154,6 @@ mount_exit:
 	}
 	free(options);
 	free(orgoptions);
+	free(mountpoint);
 	return rc;
 }
-- 
2.7.4

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

[Aurélien Aptel]

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:

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,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg)

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

[Zhiqiang Liu]

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,
>