[U-Boot] Possible bug in UBIFS function ubifs_finddir

[U-Boot] Possible bug in UBIFS function ubifs_finddir

[Rod Boyce]

All,

Hello again it has been a while since I was here.
I am working on u-boot once again and think I may have found a bug in the
UBIFS sub-system.

The function is ubifs_finddir and the issue is that there seems to be a
free of a pointer in a structure that has already been freed.  This is
causing the free function to rightly crash.

The code is in the error path of the ubifs_finddir at the end of the
function line 363:

	if (file)
		free(file);
	if (dentry)
		free(dentry);
	if (dir)
		free(dir);

	if (file->private_data)
		kfree(file->private_data);
	file->private_data = NULL;
	file->f_pos = 2;

The issue is that we are free'ing the file pointer at the top of this
block and then trying to free the private_data element after the base
pointer.  I will fix and send a patch but before I do I just wanted to
make sure I was not missing the obvious.
Has this been discussed before and is there already a patch?

Regards,
Rod Boyce

[U-Boot] [Patch] For bug in UBIFS function ubifs_finddir

[Rod Boyce]

Free private_data member element before freeing file structure.  This
was causing malloc to crash.  Also remove unnecessary variable
assigments after file structure was free'd.

Signed-off-by: Rod Boyce <uboot@teamboyce.co.uk>
------------------------------- fs/ubifs/ubifs.c 
------------------------------
diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 5a5c739..61f70b2 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -360,6 +360,8 @@
          return err;
      }

+    if (file->private_data)
+        kfree(file->private_data);
      if (file)
          free(file);
      if (dentry)
@@ -367,10 +369,6 @@
      if (dir)
          free(dir);

-    if (file->private_data)
-        kfree(file->private_data);
-    file->private_data = NULL;
-    file->f_pos = 2;
      return 0;
  }

[U-Boot] [Patch] For bug in UBIFS function ubifs_finddir

[Stefan Roese]

Hi Rod,

On Saturday 18 June 2011 11:51:11 Rod Boyce wrote:
> Free private_data member element before freeing file structure.  This
> was causing malloc to crash.  Also remove unnecessary variable
> assigments after file structure was free'd.
> 
> Signed-off-by: Rod Boyce <uboot@teamboyce.co.uk>
> ------------------------------- fs/ubifs/ubifs.c
> ------------------------------
> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> index 5a5c739..61f70b2 100644
> --- a/fs/ubifs/ubifs.c
> +++ b/fs/ubifs/ubifs.c
> @@ -360,6 +360,8 @@
>           return err;
>       }
> 
> +    if (file->private_data)
> +        kfree(file->private_data);
>       if (file)
>           free(file);
>       if (dentry)
> @@ -367,10 +369,6 @@
>       if (dir)
>           free(dir);
> 
> -    if (file->private_data)
> -        kfree(file->private_data);
> -    file->private_data = NULL;
> -    file->f_pos = 2;
>       return 0;

This patch does not apply:

Applying: For bug in UBIFS function ubifs_finddir
Using index info to reconstruct a base tree...
error: patch failed: fs/ubifs/ubifs.c:360
error: fs/ubifs/ubifs.c: patch does not apply
Did you hand edit your patch?

How did you create this patch? I recommend to use "git format-patch". And send 
it via "git send-email".

Also, please change the patch subject and add "ubifs:":

- For  bug in UBIFS function ubifs_finddir
+ ubifs: Fix bug in function ubifs_finddir

Please also take a look at this page for patch submission:

http://www.denx.de/wiki/view/U-Boot/Patches

Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] [Patch] For bug in UBIFS function ubifs_finddir

[Detlev Zundel]

Hi Stefan,

[...]

> This patch does not apply:
>
> Applying: For bug in UBIFS function ubifs_finddir
> Using index info to reconstruct a base tree...
> error: patch failed: fs/ubifs/ubifs.c:360
> error: fs/ubifs/ubifs.c: patch does not apply
> Did you hand edit your patch?
>
> How did you create this patch? I recommend to use "git format-patch". And send 
> it via "git send-email".
>
> Also, please change the patch subject and add "ubifs:":
>
> - For  bug in UBIFS function ubifs_finddir
> + ubifs: Fix bug in function ubifs_finddir
>
> Please also take a look at this page for patch submission:
>
> http://www.denx.de/wiki/view/U-Boot/Patches
>
> Thanks.

Sorry for my duplication, I just saw that you beat me with your answer
;)

Cheers
  Detlev

-- 
Ich glaube, da? die Weltanschauung, die aus der modernen Physik hervorgeht,
mit unserer gegenw?rtigen  Gesellschaft unvereinbar ist,  weil sie den har-
monischen Zusammenh?ngen,  die wir in der Natur beaobachten, nicht Rechnung
tr?gt.                                  -- Fritjof Capra
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [Patch] For bug in UBIFS function ubifs_finddir

[Detlev Zundel]

Hi Rod,

> Free private_data member element before freeing file structure.  This
> was causing malloc to crash.  Also remove unnecessary variable
> assigments after file structure was free'd.
>
> Signed-off-by: Rod Boyce <uboot@teamboyce.co.uk>
> ------------------------------- fs/ubifs/ubifs.c 
> ------------------------------
> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> index 5a5c739..61f70b2 100644
> --- a/fs/ubifs/ubifs.c
> +++ b/fs/ubifs/ubifs.c
> @@ -360,6 +360,8 @@
>           return err;
>       }
>
> +    if (file->private_data)
> +        kfree(file->private_data);
>       if (file)
>           free(file);
>       if (dentry)
> @@ -367,10 +369,6 @@
>       if (dir)
>           free(dir);
>
> -    if (file->private_data)
> -        kfree(file->private_data);
> -    file->private_data = NULL;
> -    file->f_pos = 2;
>       return 0;
>   }

Actually the patch looks good - though I cannot apply the patch easily:

[dzu at pollux u-boot-testing (master)]$ git am -3 ~/transfer/p1
Applying: For bug in UBIFS function ubifs_finddir
Using index info to reconstruct a base tree...
error: patch failed: fs/ubifs/ubifs.c:360
error: fs/ubifs/ubifs.c: patch does not apply
Did you hand edit your patch?
It does not apply to blobs recorded in its index.
Cannot fall back to three-way merge.
Patch failed at 0001 For bug in UBIFS function ubifs_finddir
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Can you please resend the patch in a way that makes it possible to apply
to current master?  And if you do that, please change the subject line
to something more expressive.

Thanks
  Detlev

-- 
... the tools we are trying to use and the language or notation we are using to
express or record our thoughts,  are the major factors  determining what we can
think or express at all!
                                        -- Edsger W. Dijkstra
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [Patch] For bug in UBIFS function ubifs_finddir

[Wolfgang Denk]

Dear Rod Boyce,

In message <4DFC750F.7050605@teamboyce.co.uk> you wrote:
> 
> Free private_data member element before freeing file structure.  This
> was causing malloc to crash.  Also remove unnecessary variable
> assigments after file structure was free'd.
> 
> Signed-off-by: Rod Boyce <uboot@teamboyce.co.uk>

Changes have been requested for your patch.

Do you think you will find time to submit a fixed version any time
soon?

Thanks in advance.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If all the Chinese simultaneously jumped into the Pacific  off  a  10
foot platform erected 10 feet off their coast, it would cause a tidal
wave that would destroy everything in this country west of Nebraska.

[U-Boot] [PATCH] ubifs: Fix bad free() sequence in ubifs_finddir()

[Wolfgang Denk]

Free private_data member element before freeing file structure.
This was causing malloc to crash. Also remove unnecessary variable
assigments as file structure gets free'd as well.

Signed-off-by: Rod Boyce <uboot@teamboyce.co.uk>
Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Stefan Roese <sr@denx.de>
---
As Rod appears to have disappeared I took the frredom to jump in and
fix this. - wd

 fs/ubifs/ubifs.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 5a5c739..61f70b2 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -360,6 +360,8 @@ out:
 		return err;
 	}
 
+	if (file->private_data)
+		kfree(file->private_data);
 	if (file)
 		free(file);
 	if (dentry)
@@ -367,10 +369,6 @@ out:
 	if (dir)
 		free(dir);
 
-	if (file->private_data)
-		kfree(file->private_data);
-	file->private_data = NULL;
-	file->f_pos = 2;
 	return 0;
 }
 
-- 
1.7.6

[U-Boot] [PATCH] ubifs: Fix bad free() sequence in ubifs_finddir()

[Rod Boyce]

On 28/07/11 14:27, Wolfgang Denk wrote:
> As Rod appears to have disappeared I took the frredom to jump in and
> fix this. - wd
>

All,

Sorry about this I missed the e-mail asking me to reformat the patch.  
Thanks for sorting this out Wolfgang.

Regards,
Rod Boyce

[U-Boot] [PATCH] ubifs: Fix bad free() sequence in ubifs_finddir()

[Stefan Roese]

On Thursday 28 July 2011 15:27:22 Wolfgang Denk wrote:
> Free private_data member element before freeing file structure.
> This was causing malloc to crash. Also remove unnecessary variable
> assigments as file structure gets free'd as well.

Applied to u-boot-ubi/master. Thanks.

Best regards,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de