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

Commit Message

Zhiqiang Liu Aug. 1, 2019, 2:02 a.m. UTC

From: Jiawen Liu <[e-mail]>

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 <[e-mail]>
Reported-by: Jin Du <[e-mail]>
Reviewed-by: Saisai Zhang <[e-mail]>
---
 mount.cifs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Aurélien Aptel Aug. 1, 2019, 9:15 a.m. UTC | #1

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 <[e-mail]> 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,

Zhiqiang Liu Aug. 1, 2019, 2:06 p.m. UTC | #2

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 <[e-mail]> 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,
>

Patch

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