Re: [PATCH v2] fs: affs: fix a memory leak in affs_remount

[Navid Emamdoost]

Thanks David,

On Wed, Oct 2, 2019 at 4:22 AM David Sterba  wrote:
>
> On Mon, Sep 30, 2019 at 04:01:10PM -0500, Navid Emamdoost wrote:
> > In affs_remount if data is provided it is duplicated into new_opts.
> > The allocated memory for new_opts is only released if pare_options fail.
> > The release for new_opts is added.
>
> A variable that is allocated and freed without use should ring a bell to
> look closer at the code. There's a bit of history behind new_options,
> originally there was save/replace options on the VFS layer so the 'data'
> passed must not change (thus strdup), this got cleaned up in later
> patches. But not completely.
>
> There's no reason to do the strdup in cases where the filesystem does
> not need to reuse the 'data' again, because strsep would modify it
> directly.
>
> So new_opts should be removed.

I will send a new patch with the unused variable removed.


-- 
Navid.

Re: [PATCH v2] fs: affs: fix a memory leak in affs_remount

[David Sterba]

On Mon, Sep 30, 2019 at 04:01:10PM -0500, Navid Emamdoost wrote:
> In affs_remount if data is provided it is duplicated into new_opts.
> The allocated memory for new_opts is only released if pare_options fail.
> The release for new_opts is added.

A variable that is allocated and freed without use should ring a bell to
look closer at the code. There's a bit of history behind new_options,
originally there was save/replace options on the VFS layer so the 'data'
passed must not change (thus strdup), this got cleaned up in later
patches. But not completely.

There's no reason to do the strdup in cases where the filesystem does
not need to reuse the 'data' again, because strsep would modify it
directly.

So new_opts should be removed. 

Re: [PATCH v2] fs: affs: fix a memory leak in affs_remount

[Navid Emamdoost]

Hi Markus, thanks for your suggestions for improving the quality of
the patch. At the moment I prefer first get a confirmation from
contributors about the leak and then work on any possible improvements
for the patch.

Thanks,
Navid.

On Tue, Oct 1, 2019 at 3:31 AM Markus Elfring  wrote:
>
> > The allocated memory for new_opts is only released if pare_options fail.
>
> Can the following wording be nicer?
>
>   The allocated memory for the buffer “new_opts” will be released
>   only if a call of the function “parse_options” failed.
>
>
> > The release for new_opts is added.
>
> * How do you think about the change possibility to delete questionable
>   source code here?
>
> * Would you like to complete the data processing for corresponding options
>   any more?
>
>
> >   -- fix a type in title, …
>
> Please avoid typos also in your version comments.
>
>
> > ---
>
> I suggest to replace this second delimiter by a blank line.
>
> Regards,
> Markus



-- 
Navid.

Re: [PATCH v2] fs: affs: fix a memory leak in affs_remount

[Markus Elfring]

> The allocated memory for new_opts is only released if pare_options fail.

Can the following wording be nicer?

  The allocated memory for the buffer “new_opts” will be released
  only if a call of the function “parse_options” failed.


> The release for new_opts is added.

* How do you think about the change possibility to delete questionable
  source code here?

* Would you like to complete the data processing for corresponding options
  any more?


>   -- fix a type in title, …

Please avoid typos also in your version comments.


> ---

I suggest to replace this second delimiter by a blank line.

Regards,
Markus

[PATCH v2] fs: affs: fix a memory leak in affs_remount

[Navid Emamdoost]

In affs_remount if data is provided it is duplicated into new_opts.
The allocated memory for new_opts is only released if pare_options fail.
The release for new_opts is added.

Fixes: c8f33d0bec99 ("affs: kstrdup() memory handling")
Signed-off-by: Navid Emamdoost 
---
Changes in v2:
-- fix a type in title, and add fixes tag
---
 fs/affs/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/affs/super.c b/fs/affs/super.c
index cc463ae47c12..1d38fdbc5148 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -598,6 +598,8 @@ affs_remount(struct super_block *sb, int *flags, char *data)
memcpy(sbi->s_volume, volume, 32);
spin_unlock(&sbi->symlink_lock);
 
+   kfree(new_opts);
+
if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
return 0;
 
-- 
2.17.1