lkml.org 
[lkml]   [2011]   [Aug]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3] Fix leak in drivers/target/iscsi/iscsi_target_parameters.c::iscsi_copy_param_list()
From
Date
On Tue, 2011-08-02 at 10:26 +0200, Jesper Juhl wrote:
> On Mon, 1 Aug 2011, Joe Perches wrote:
>
> > On Mon, 2011-08-01 at 22:32 +0200, Jesper Juhl wrote:
> > > diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
> > []
> > > @@ -572,6 +572,17 @@ int iscsi_copy_param_list(
> > []
> > > + if (!new_param->name || !new_param->value) {
> > []
> > > + pr_err("Unable to allocate memory for parameter "
> > > + "name/value.\n");
> >
> > I know you didn't change these lines, but could you please
> > remember to coalesce formats to a single line next time?
> >
>
> Ok, let me try once more. I hope this is now acceptable.
>
>
> From: Jesper Juhl <jj@chaosbits.net>
> Subject: [PATCH v3] Fix leak in drivers/target/iscsi/iscsi_target_parameters.c::iscsi_copy_param_list()
>
> We leak memory if the allocations for 'new_param->name' or
> 'new_param->value' fail.
>
> We also do a lot of variable assignments that are completely pointless
> if the allocations fail.
>
> So, let's move the allocations before the assignments and also make
> sure that we free whatever was allocated to one if the allocation fail.
>
> There's also some small CodingStyle fixups in there (curly braces on
> both branches of if statement, only one variable per line) since I was
> in the area anyway. And finally, error messages in the function are
> put on a single line for easy grep'abillity.
>
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>

Hi Jesper,

This patch has been committed as 83f8b803171 into
lio-core-2.6.git/master, and will be queued into the next set of target
updates.

Thanks!

--nab

> ---
> drivers/target/iscsi/iscsi_target_parameters.c | 43 +++++++++---------------
> 1 files changed, 16 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
> index 252e246..497b2e7 100644
> --- a/drivers/target/iscsi/iscsi_target_parameters.c
> +++ b/drivers/target/iscsi/iscsi_target_parameters.c
> @@ -545,13 +545,13 @@ int iscsi_copy_param_list(
> struct iscsi_param_list *src_param_list,
> int leading)
> {
> - struct iscsi_param *new_param = NULL, *param = NULL;
> + struct iscsi_param *param = NULL;
> + struct iscsi_param *new_param = NULL;
> struct iscsi_param_list *param_list = NULL;
>
> param_list = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);
> if (!param_list) {
> - pr_err("Unable to allocate memory for"
> - " struct iscsi_param_list.\n");
> + pr_err("Unable to allocate memory for struct iscsi_param_list.\n");
> goto err_out;
> }
> INIT_LIST_HEAD(&param_list->param_list);
> @@ -567,8 +567,17 @@ int iscsi_copy_param_list(
>
> new_param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL);
> if (!new_param) {
> - pr_err("Unable to allocate memory for"
> - " struct iscsi_param.\n");
> + pr_err("Unable to allocate memory for struct iscsi_param.\n");
> + goto err_out;
> + }
> +
> + new_param->name = kstrdup(param->name, GFP_KERNEL);
> + new_param->value = kstrdup(param->value, GFP_KERNEL);
> + if (!new_param->value || !new_param->name) {
> + kfree(new_param->value);
> + kfree(new_param->name);
> + kfree(new_param);
> + pr_err("Unable to allocate memory for parameter name/value.\n");
> goto err_out;
> }
>
> @@ -580,32 +589,12 @@ int iscsi_copy_param_list(
> new_param->use = param->use;
> new_param->type_range = param->type_range;
>
> - new_param->name = kzalloc(strlen(param->name) + 1, GFP_KERNEL);
> - if (!new_param->name) {
> - pr_err("Unable to allocate memory for"
> - " parameter name.\n");
> - goto err_out;
> - }
> -
> - new_param->value = kzalloc(strlen(param->value) + 1,
> - GFP_KERNEL);
> - if (!new_param->value) {
> - pr_err("Unable to allocate memory for"
> - " parameter value.\n");
> - goto err_out;
> - }
> -
> - memcpy(new_param->name, param->name, strlen(param->name));
> - new_param->name[strlen(param->name)] = '\0';
> - memcpy(new_param->value, param->value, strlen(param->value));
> - new_param->value[strlen(param->value)] = '\0';
> -
> list_add_tail(&new_param->p_list, &param_list->param_list);
> }
>
> - if (!list_empty(&param_list->param_list))
> + if (!list_empty(&param_list->param_list)) {
> *dst_param_list = param_list;
> - else {
> + } else {
> pr_err("No parameters allocated.\n");
> goto err_out;
> }
> --
> 1.7.6
>
>
> --
> Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
> Don't top-post http://www.catb.org/jargon/html/T/top-post.html
> Plain text mails only, please.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html



\
 
 \ /
  Last update: 2011-08-04 20:17    [W:0.118 / U:0.996 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site