Skip to contentSkip to conversation

Possible bug in UBIFS function ubifs_finddir

U-Boot New Rod Boyce 4 participants 9 messages Jun 16, 2011 – Aug 19, 2011 UBI file system (UBIFS)

lore
Share & tools
lore

Rod Boyce

author
#0 lore raw

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

author
#1 lore raw

PATCH 1/1 For bug in UBIFS function ubifs_finddir

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 <[email protected]>

------------------------------- fs/ubifs/ubifs.c
------------------------------

fs/ubifs/ubifs.c +2 -4
index 5a5c739..61f70b2 100644
@@ -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;
  }
#2 lore raw

PATCH 1/1 For bug in UBIFS function ubifs_finddir

Hi Rod,

Quoted 21 lines · 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 <[email protected]>

------------------------------- fs/ubifs/ubifs.c
------------------------------

fs/ubifs/ubifs.c +2 -0
index 5a5c739..61f70b2 100644
@@ -360,6 +360,8 @@
          return err;
      }
 
+    if (file->private_data)
+        kfree(file->private_data);
      if (file)
          free(file);
      if (dentry)

fs/ubifs/ubifs.c line 372

Quoted 8 lines
fs/ubifs/ubifs.c +0 -4
@@ -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

#3 lore raw

PATCH 1/1 For bug in UBIFS function ubifs_finddir

Hi Stefan,

[...]

Quoted 13 lines

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:":

Quoted 8 lines

- 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

Signature · 8 lines
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
#4 lore raw

PATCH 1/1 For bug in UBIFS function ubifs_finddir

In reply to Rod Boyce

Hi Rod,

Quoted 21 lines

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 <[email protected]>

------------------------------- fs/ubifs/ubifs.c
------------------------------

fs/ubifs/ubifs.c +2 -0
index 5a5c739..61f70b2 100644
@@ -360,6 +360,8 @@
          return err;
      }
 
+    if (file->private_data)
+        kfree(file->private_data);
      if (file)
          free(file);
      if (dentry)

fs/ubifs/ubifs.c line 373

Quoted 9 lines
fs/ubifs/ubifs.c +0 -4
@@ -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

Signature · 8 lines
... 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
#5 lore raw

PATCH 1/1 For bug in UBIFS function ubifs_finddir

In reply to Rod Boyce

Dear Rod Boyce,

Quoted 6 lines · In message <[email protected]> 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 <[email protected]>

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

Signature · 6 lines
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.
#6 lore raw

PATCH 1/1 ubifs: Fix bad free() sequence in ubifs_finddir()

In reply to Rod Boyce

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 <[email protected]>
Signed-off-by Wolfgang Denk <[email protected]>
Cc Stefan Roese <[email protected]>

---
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(-)
fs/ubifs/ubifs.c +2 -4
index 5a5c739..61f70b2 100644
@@ -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;
 }
Signature · 1 line
1.7.6
author
#7 lore raw

PATCH 1/1 ubifs: Fix bad free() sequence in ubifs_finddir()

Quoted 3 lines · Wolfgang Denk

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

#8 lore raw

PATCH 1/1 ubifs: Fix bad free() sequence in ubifs_finddir()

In reply to Wolfgang Denk
Quoted 3 lines · 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