[PATCH] UBIFS: fix memory leak on error path

[PATCH] UBIFS: fix memory leak on error path

[Sidney Amani]

UBIFS leaks memory on error path in 'mount_ubifs()'. In case of failure in
'ubifs_lpt_init()' or 'ubifs_fixup_free_space()', it does not call
'ubifs_lpt_free()' whereas LPT data structures can potentially be allocated.
The amount of memory leaked can be quite high -- see 'ubifs_lpt_init()'.

The bug was introduced when moving the LPT initialisation earlier in the
mount process (commit '781c5717a95a74b294beb38b8276943b0f8b5bb4').

CC: Ben Gardiner <bengardiner@nanometrics.ca>
Signed-off-by: Sidney Amani <seed95@gmail.com>
---
 fs/ubifs/super.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 76e4e05..50216ec 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1296,12 +1296,12 @@ static int mount_ubifs(struct ubifs_info *c)
 
 	err = ubifs_lpt_init(c, 1, !c->ro_mount);
 	if (err)
-		goto out_master;
+		goto out_lpt;
 
 	if (!c->ro_mount && c->space_fixup) {
 		err = ubifs_fixup_free_space(c);
 		if (err)
-			goto out_master;
+			goto out_lpt;
 	}
 
 	if (!c->ro_mount) {
-- 
1.7.5.4

[PATCH 1/2] UBIFS: make ubifs_lpt_init clean-up in case of failure

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Most functions in UBIFS follow the following designn pattern: if the function
allocates multiple resources, and failss at some point, it frees what it has
allocated and returns an error. So the caller can rely on the fact that the
callee has cleaned up everything after own failure.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/ubifs/lpt.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/lpt.c b/fs/ubifs/lpt.c
index 2054e81..b4280c4 100644
--- a/fs/ubifs/lpt.c
+++ b/fs/ubifs/lpt.c
@@ -1740,16 +1740,20 @@ int ubifs_lpt_init(struct ubifs_info *c, int rd, int wr)
 	if (rd) {
 		err = lpt_init_rd(c);
 		if (err)
-			return err;
+			goto out_err;
 	}
 
 	if (wr) {
 		err = lpt_init_wr(c);
 		if (err)
-			return err;
+			goto out_err;
 	}
 
 	return 0;
+
+out_err:
+	ubifs_lpt_free(c, 0);
+	return err;
 }
 
 /**
-- 
1.7.10

[PATCH 2/2] UBIFS: fix memory leak on error path

[Artem Bityutskiy]

From: Sidney Amani <seed95@gmail.com>

UBIFS leaks memory on error path in 'mount_ubifs()'. In case of failure in
'ubifs_fixup_free_space()', it does not call 'ubifs_lpt_free()' whereas LPT
data structures can potentially be allocated. The amount of memory leaked can
be quite high -- see 'ubifs_lpt_init()'.

The bug was introduced when moving the LPT initialisation earlier in the
mount process (commit '781c5717a95a74b294beb38b8276943b0f8b5bb4').

Signed-off-by: Sidney Amani <seed95@gmail.com>
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/ubifs/super.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 5b30c4d..675b781 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1301,7 +1301,7 @@ static int mount_ubifs(struct ubifs_info *c)
 	if (!c->ro_mount && c->space_fixup) {
 		err = ubifs_fixup_free_space(c);
 		if (err)
-			goto out_master;
+			goto out_lpt;
 	}
 
 	if (!c->ro_mount) {
-- 
1.7.10

Re: [PATCH] UBIFS: fix memory leak on error path

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]

On Thu, 2012-05-17 at 19:03 +1000, Sidney Amani wrote:
> UBIFS leaks memory on error path in 'mount_ubifs()'. In case of failure in
> 'ubifs_lpt_init()' or 'ubifs_fixup_free_space()', it does not call
> 'ubifs_lpt_free()' whereas LPT data structures can potentially be allocated.
> The amount of memory leaked can be quite high -- see 'ubifs_lpt_init()'.
> 
> The bug was introduced when moving the LPT initialisation earlier in the
> mount process (commit '781c5717a95a74b294beb38b8276943b0f8b5bb4').
> 
> CC: Ben Gardiner <bengardiner@nanometrics.ca>
> Signed-off-by: Sidney Amani <seed95@gmail.com>

I've replied to you with the counter-proposal patches which I think
should fix this issue in a bit better way. Please, take a look - If you
are fine with that, I can push it.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] UBIFS: make ubifs_lpt_init clean-up in case of failure

[Sidney Amani]

On Fri, May 18, 2012 at 9:32 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>
> Most functions in UBIFS follow the following designn pattern: if the function
> allocates multiple resources, and failss at some point, it frees what it has
> allocated and returns an error. So the caller can rely on the fact that the
> callee has cleaned up everything after own failure.
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Acked-by: Sidney Amani <seed95@gmail.com>

-- 
Sidney

Re: [PATCH] UBIFS: fix memory leak on error path

[Sidney Amani]

On Fri, May 18, 2012 at 9:38 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> I've replied to you with the counter-proposal patches which I think
> should fix this issue in a bit better way. Please, take a look - If you
> are fine with that, I can push it.
>

Agreed, your set of patches is better.

Cheers
-- 
Sidney