[PATCH] target: Fix memory leak in fd_set_configfs_dev_params().

[PATCH] target: Fix memory leak in fd_set_configfs_dev_params().

[Jesper Juhl]

match_strdup() dynamically allocates memory and it is the responsabillity 
of the caller to free that memory. In 
drivers/target/target_core_file.c::fd_set_configfs_dev_params() two calls 
are made to match_strdup() and in neither case is the allocated memory 
freed, but instead it is leaked.

This patch should take care of the problem by kfree()'ing the allocated 
memory once it is no longer needed. It also makes sure to return -ENOMEM 
if the memory allocation in match_strdup() should fail.

Please review and consider for inclusion.

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 target_core_file.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

  compile tested only.

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 0aaca88..676a010 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -537,15 +537,26 @@ static ssize_t fd_set_configfs_dev_params(
 		token = match_token(ptr, tokens, args);
 		switch (token) {
 		case Opt_fd_dev_name:
+			arg_p = match_strdup(&args[0]);
+			if (!arg_p) {
+				ret = -ENOMEM;
+				break;
+			}
 			snprintf(fd_dev->fd_dev_name, FD_MAX_DEV_NAME,
-					"%s", match_strdup(&args[0]));
+					"%s", arg_p);
+			kfree(arg_p);
 			printk(KERN_INFO "FILEIO: Referencing Path: %s\n",
 					fd_dev->fd_dev_name);
 			fd_dev->fbd_flags |= FBDF_HAS_PATH;
 			break;
 		case Opt_fd_dev_size:
 			arg_p = match_strdup(&args[0]);
+			if (!arg_p) {
+				ret = -ENOMEM;
+				break;
+			}
 			ret = strict_strtoull(arg_p, 0, &fd_dev->fd_dev_size);
+			kfree(arg_p);
 			if (ret < 0) {
 				printk(KERN_ERR "strict_strtoull() failed for"
 						" fd_dev_size=\n");


-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] target: Fix memory leak in fd_set_configfs_dev_params().

[Nicholas A. Bellinger]

On Sun, 2011-01-16 at 20:17 +0100, Jesper Juhl wrote:
> match_strdup() dynamically allocates memory and it is the responsabillity 
> of the caller to free that memory. In 
> drivers/target/target_core_file.c::fd_set_configfs_dev_params() two calls 
> are made to match_strdup() and in neither case is the allocated memory 
> freed, but instead it is leaked.
> 
> This patch should take care of the problem by kfree()'ing the allocated 
> memory once it is no longer needed. It also makes sure to return -ENOMEM 
> if the memory allocation in match_strdup() should fail.
> 
> Please review and consider for inclusion.
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>

Ugh, this was my fault during the recent v4.0 configfs parameter
conversion.  Committed as 5c45b37 in lio-core-2.6.git/linus-38-rc1 and I
will fix up the other handful of match_strdup() breakage and carry into
scsi-post-merge-2.6.git/for-jejb shortly.

Thanks for catching this..

--nab

> ---
>  target_core_file.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
>   compile tested only.
> 
> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 0aaca88..676a010 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -537,15 +537,26 @@ static ssize_t fd_set_configfs_dev_params(
>  		token = match_token(ptr, tokens, args);
>  		switch (token) {
>  		case Opt_fd_dev_name:
> +			arg_p = match_strdup(&args[0]);
> +			if (!arg_p) {
> +				ret = -ENOMEM;
> +				break;
> +			}
>  			snprintf(fd_dev->fd_dev_name, FD_MAX_DEV_NAME,
> -					"%s", match_strdup(&args[0]));
> +					"%s", arg_p);
> +			kfree(arg_p);
>  			printk(KERN_INFO "FILEIO: Referencing Path: %s\n",
>  					fd_dev->fd_dev_name);
>  			fd_dev->fbd_flags |= FBDF_HAS_PATH;
>  			break;
>  		case Opt_fd_dev_size:
>  			arg_p = match_strdup(&args[0]);
> +			if (!arg_p) {
> +				ret = -ENOMEM;
> +				break;
> +			}
>  			ret = strict_strtoull(arg_p, 0, &fd_dev->fd_dev_size);
> +			kfree(arg_p);
>  			if (ret < 0) {
>  				printk(KERN_ERR "strict_strtoull() failed for"
>  						" fd_dev_size=\n");
> 
> 

Re: [PATCH] target: Fix memory leak in fd_set_configfs_dev_params().

[Jesper Juhl]

On Sun, 16 Jan 2011, Nicholas A. Bellinger wrote:

> On Sun, 2011-01-16 at 20:17 +0100, Jesper Juhl wrote:
> > match_strdup() dynamically allocates memory and it is the responsabillity 
> > of the caller to free that memory. In 
> > drivers/target/target_core_file.c::fd_set_configfs_dev_params() two calls 
> > are made to match_strdup() and in neither case is the allocated memory 
> > freed, but instead it is leaked.
> > 
> > This patch should take care of the problem by kfree()'ing the allocated 
> > memory once it is no longer needed. It also makes sure to return -ENOMEM 
> > if the memory allocation in match_strdup() should fail.
> > 
> > Please review and consider for inclusion.
> > 
> > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> 
> Ugh, this was my fault during the recent v4.0 configfs parameter
> conversion.  Committed as 5c45b37 in lio-core-2.6.git/linus-38-rc1 and I
> will fix up the other handful of match_strdup() breakage and carry into
> scsi-post-merge-2.6.git/for-jejb shortly.
> 
> Thanks for catching this..
> 

You're welcome.
Yes, I did see a bunch of other match_strdup() related problems, but I 
thought I'd just fix up one file initially to see the general reaction, 
then do the rest later when this one had been merged. but if you want to 
fix up the remainder, feel free.

I guess we can merge this patch as a starting point?

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] target: Fix memory leak in fd_set_configfs_dev_params().

[Nicholas A. Bellinger]

On Sun, 2011-01-16 at 23:59 +0100, Jesper Juhl wrote:
> On Sun, 16 Jan 2011, Nicholas A. Bellinger wrote:
> 
> > On Sun, 2011-01-16 at 20:17 +0100, Jesper Juhl wrote:
> > > match_strdup() dynamically allocates memory and it is the responsabillity 
> > > of the caller to free that memory. In 
> > > drivers/target/target_core_file.c::fd_set_configfs_dev_params() two calls 
> > > are made to match_strdup() and in neither case is the allocated memory 
> > > freed, but instead it is leaked.
> > > 
> > > This patch should take care of the problem by kfree()'ing the allocated 
> > > memory once it is no longer needed. It also makes sure to return -ENOMEM 
> > > if the memory allocation in match_strdup() should fail.
> > > 
> > > Please review and consider for inclusion.
> > > 
> > > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > 
> > Ugh, this was my fault during the recent v4.0 configfs parameter
> > conversion.  Committed as 5c45b37 in lio-core-2.6.git/linus-38-rc1 and I
> > will fix up the other handful of match_strdup() breakage and carry into
> > scsi-post-merge-2.6.git/for-jejb shortly.
> > 
> > Thanks for catching this..
> > 
> 
> You're welcome.
> Yes, I did see a bunch of other match_strdup() related problems, but I 
> thought I'd just fix up one file initially to see the general reaction, 
> then do the rest later when this one had been merged. but if you want to 
> fix up the remainder, feel free.
> 
> I guess we can merge this patch as a starting point?
> 

Sure, the other two follow-up match_strdup() memory leak bugfix patches
for target/iblock and target_core_configfs.c APTPL metadata token
parsing have been included into the scsi-post-merge-2.6.git/for-jejb
queue.

Thanks again!

--nab

Re: [PATCH] target: Fix memory leak in fd_set_configfs_dev_params().

[Jesper Juhl]

On Sun, 16 Jan 2011, Nicholas A. Bellinger wrote:

> On Sun, 2011-01-16 at 23:59 +0100, Jesper Juhl wrote:
> > On Sun, 16 Jan 2011, Nicholas A. Bellinger wrote:
> > 
> > > On Sun, 2011-01-16 at 20:17 +0100, Jesper Juhl wrote:
> > > > match_strdup() dynamically allocates memory and it is the responsabillity 
> > > > of the caller to free that memory. In 
> > > > drivers/target/target_core_file.c::fd_set_configfs_dev_params() two calls 
> > > > are made to match_strdup() and in neither case is the allocated memory 
> > > > freed, but instead it is leaked.
> > > > 
> > > > This patch should take care of the problem by kfree()'ing the allocated 
> > > > memory once it is no longer needed. It also makes sure to return -ENOMEM 
> > > > if the memory allocation in match_strdup() should fail.
> > > > 
> > > > Please review and consider for inclusion.
> > > > 
> > > > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > > 
> > > Ugh, this was my fault during the recent v4.0 configfs parameter
> > > conversion.  Committed as 5c45b37 in lio-core-2.6.git/linus-38-rc1 and I
> > > will fix up the other handful of match_strdup() breakage and carry into
> > > scsi-post-merge-2.6.git/for-jejb shortly.
> > > 
> > > Thanks for catching this..
> > > 
> > 
> > You're welcome.
> > Yes, I did see a bunch of other match_strdup() related problems, but I 
> > thought I'd just fix up one file initially to see the general reaction, 
> > then do the rest later when this one had been merged. but if you want to 
> > fix up the remainder, feel free.
> > 
> > I guess we can merge this patch as a starting point?
> > 
> 
> Sure, the other two follow-up match_strdup() memory leak bugfix patches
> for target/iblock and target_core_configfs.c APTPL metadata token
> parsing have been included into the scsi-post-merge-2.6.git/for-jejb
> queue.
> 
Ok, I assume you'll add my patch there as well?

> Thanks again!
> 
np

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] target: Fix memory leak in fd_set_configfs_dev_params().

[Nicholas A. Bellinger]

On Mon, 2011-01-17 at 19:29 +0100, Jesper Juhl wrote:
> On Sun, 16 Jan 2011, Nicholas A. Bellinger wrote:
> 
> > On Sun, 2011-01-16 at 23:59 +0100, Jesper Juhl wrote:
> > > On Sun, 16 Jan 2011, Nicholas A. Bellinger wrote:
> > > 
> > > > On Sun, 2011-01-16 at 20:17 +0100, Jesper Juhl wrote:
> > > > > match_strdup() dynamically allocates memory and it is the responsabillity 
> > > > > of the caller to free that memory. In 
> > > > > drivers/target/target_core_file.c::fd_set_configfs_dev_params() two calls 
> > > > > are made to match_strdup() and in neither case is the allocated memory 
> > > > > freed, but instead it is leaked.
> > > > > 
> > > > > This patch should take care of the problem by kfree()'ing the allocated 
> > > > > memory once it is no longer needed. It also makes sure to return -ENOMEM 
> > > > > if the memory allocation in match_strdup() should fail.
> > > > > 
> > > > > Please review and consider for inclusion.
> > > > > 
> > > > > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > > > 
> > > > Ugh, this was my fault during the recent v4.0 configfs parameter
> > > > conversion.  Committed as 5c45b37 in lio-core-2.6.git/linus-38-rc1 and I
> > > > will fix up the other handful of match_strdup() breakage and carry into
> > > > scsi-post-merge-2.6.git/for-jejb shortly.
> > > > 
> > > > Thanks for catching this..
> > > > 
> > > 
> > > You're welcome.
> > > Yes, I did see a bunch of other match_strdup() related problems, but I 
> > > thought I'd just fix up one file initially to see the general reaction, 
> > > then do the rest later when this one had been merged. but if you want to 
> > > fix up the remainder, feel free.
> > > 
> > > I guess we can merge this patch as a starting point?
> > > 
> > 
> > Sure, the other two follow-up match_strdup() memory leak bugfix patches
> > for target/iblock and target_core_configfs.c APTPL metadata token
> > parsing have been included into the scsi-post-merge-2.6.git/for-jejb
> > queue.
> > 
> Ok, I assume you'll add my patch there as well?
> 

Your patch along with the two followup match_strdup() fixes and your
Reported-by' have been included in the LIO upstream tree (now merged
with mainline tracking linus-38-rc1), and in the queue for
scsi-post-merge-2.6.git/for-jejb branch for linux-scsi review ->
mainline for-38.

--nab

Re: [PATCH] target: Fix memory leak in fd_set_configfs_dev_params().

[Jesper Juhl]

On Mon, 17 Jan 2011, Nicholas A. Bellinger wrote:

> On Mon, 2011-01-17 at 19:29 +0100, Jesper Juhl wrote:
> > On Sun, 16 Jan 2011, Nicholas A. Bellinger wrote:
> > 
> > > On Sun, 2011-01-16 at 23:59 +0100, Jesper Juhl wrote:
> > > > On Sun, 16 Jan 2011, Nicholas A. Bellinger wrote:
> > > > 
> > > > > On Sun, 2011-01-16 at 20:17 +0100, Jesper Juhl wrote:
> > > > > > match_strdup() dynamically allocates memory and it is the responsabillity 
> > > > > > of the caller to free that memory. In 
> > > > > > drivers/target/target_core_file.c::fd_set_configfs_dev_params() two calls 
> > > > > > are made to match_strdup() and in neither case is the allocated memory 
> > > > > > freed, but instead it is leaked.
> > > > > > 
> > > > > > This patch should take care of the problem by kfree()'ing the allocated 
> > > > > > memory once it is no longer needed. It also makes sure to return -ENOMEM 
> > > > > > if the memory allocation in match_strdup() should fail.
> > > > > > 
> > > > > > Please review and consider for inclusion.
> > > > > > 
> > > > > > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > > > > 
> > > > > Ugh, this was my fault during the recent v4.0 configfs parameter
> > > > > conversion.  Committed as 5c45b37 in lio-core-2.6.git/linus-38-rc1 and I
> > > > > will fix up the other handful of match_strdup() breakage and carry into
> > > > > scsi-post-merge-2.6.git/for-jejb shortly.
> > > > > 
> > > > > Thanks for catching this..
> > > > > 
> > > > 
> > > > You're welcome.
> > > > Yes, I did see a bunch of other match_strdup() related problems, but I 
> > > > thought I'd just fix up one file initially to see the general reaction, 
> > > > then do the rest later when this one had been merged. but if you want to 
> > > > fix up the remainder, feel free.
> > > > 
> > > > I guess we can merge this patch as a starting point?
> > > > 
> > > 
> > > Sure, the other two follow-up match_strdup() memory leak bugfix patches
> > > for target/iblock and target_core_configfs.c APTPL metadata token
> > > parsing have been included into the scsi-post-merge-2.6.git/for-jejb
> > > queue.
> > > 
> > Ok, I assume you'll add my patch there as well?
> > 
> 
> Your patch along with the two followup match_strdup() fixes and your
> Reported-by' have been included in the LIO upstream tree (now merged
> with mainline tracking linus-38-rc1), and in the queue for
> scsi-post-merge-2.6.git/for-jejb branch for linux-scsi review ->
> mainline for-38.
> 

Thank you.

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.