Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH cifs-utils] mount.cifs.c: fix memory leaks in main func
@ 2019-08-01  2:02 Zhiqiang Liu
  2019-08-01  9:15 ` Aurélien Aptel
  0 siblings, 1 reply; 3+ messages in thread
From: Zhiqiang Liu @ 2019-08-01  2:02 UTC (permalink / raw)
  To: linux-cifs, Aurélien Aptel, liujiawen10, smfrench, pshilov,
	lsahlber, kdsouza, ab, palcantara, liujiawen10
  Cc: dujin1, Mingfangsen, zhangsaisai

[-- Warning: decoded text below may be mangled --]
[-- Attachment #0: 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



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH cifs-utils] mount.cifs.c: fix memory leaks in main func
  2019-08-01  2:02 [PATCH cifs-utils] mount.cifs.c: fix memory leaks in main func Zhiqiang Liu
@ 2019-08-01  9:15 ` Aurélien Aptel
  2019-08-01 14:06   ` Zhiqiang Liu
  0 siblings, 1 reply; 3+ messages in thread
From: Aurélien Aptel @ 2019-08-01  9:15 UTC (permalink / raw)
  To: Zhiqiang Liu, smfrench, liujiawen10, pshilov, kdsouza, lsahlber,
	ab, palcantara, linux-cifs
  Cc: dujin1, Mingfangsen, zhangsaisai

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)

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH cifs-utils] mount.cifs.c: fix memory leaks in main func
  2019-08-01  9:15 ` Aurélien Aptel
@ 2019-08-01 14:06   ` Zhiqiang Liu
  0 siblings, 0 replies; 3+ messages in thread
From: Zhiqiang Liu @ 2019-08-01 14:06 UTC (permalink / raw)
  To: Aurélien Aptel, smfrench, liujiawen10, pshilov, kdsouza,
	lsahlber, ab, palcantara, linux-cifs
  Cc: dujin1, Mingfangsen, zhangsaisai

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-01  2:02 [PATCH cifs-utils] mount.cifs.c: fix memory leaks in main func Zhiqiang Liu
2019-08-01  9:15 ` Aurélien Aptel
2019-08-01 14:06   ` Zhiqiang Liu

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org linux-cifs@archiver.kernel.org
	public-inbox-index linux-cifs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox