Data corruption in ramfs

Data corruption in ramfs

[Øyvind Harboe]

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

This morning we discovered that ramfs started failing. There is something
you don't run into every day...

Could I have some comments on the attached patch?

The problem is that it is illegal to cache pointers returned from
findbuffer_node()
across findbuffer_node() calls, due to reallocing happening, ANAICT.

The patch closes a *tiny* window where the pointer can change. With longer
filenames it is more likely that this can happen, but I suppose
shorter filenames
are not immune to these problems...

Thanks to Laurentiu Cocanu(laurentiu.cocanu@zylin.com) for spotting this!

He has a magic computer that happened to build a romfs image which was copied
to ramfs where the order of the files just so happened to trigger this
problem :-)

-- 
Øyvind Harboe

Visit us at Embedded World, March 2nd-4th. IS2T's stand, HALL 10 - 118
http://www.zylin.com/events_embeddedworld.html

US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer

[-- Attachment #2: fixramfs.txt --]
[-- Type: text/plain, Size: 2559 bytes --]

? fixramfs.txt
Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/fs/ram/current/ChangeLog,v
retrieving revision 1.22
diff -w -u -5 -p -r1.22 ChangeLog
--- ChangeLog	28 Apr 2009 13:06:11 -0000	1.22
+++ ChangeLog	22 Feb 2010 14:31:39 -0000
@@ -1,5 +1,11 @@
+2010-02-22  Oyvind Harboe  <oyvind.harboe@zylin.com>
+
+	src/ramfs.c: Fix directory file name corruption.
+	Do not cache pointers returned from findbuffer_node(),
+	but do a fresh lookup based on position.
+
 2009-04-28  John Dallaway  <john@dallaway.org.uk>
 
 	cdl/ramfs.cdl: Use CYGPKG_IO_FILEIO as the parent.
 
 2008-04-02  Xinghua Yang <yxinghua@sunnorth.com.cn>
Index: src/ramfs.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/fs/ram/current/src/ramfs.c,v
retrieving revision 1.12
diff -w -u -5 -p -r1.12 ramfs.c
--- src/ramfs.c	29 Jan 2009 17:48:54 -0000	1.12
+++ src/ramfs.c	22 Feb 2010 14:31:40 -0000
@@ -1120,11 +1120,12 @@ static int add_direntry( ramfs_node *dir
                          int namelen,           // length of name
                          ramfs_node *node       // node to reference
                        )
 {
     off_t pos = 0;
-    ramfs_dirent *d = NULL, *dp = NULL;
+    off_t prev_pos = 0;
+    ramfs_dirent *d = NULL, *dp;
     cyg_bool isfirst = true;
 
     // Loop inserting fragments of the name into the directory until we
     // have found a home for them all.
     
@@ -1152,10 +1153,25 @@ static int add_direntry( ramfs_node *dir
             }
 
             break;
         }
 
+        /* Tricky! here we have to look up the previous segment as reallocating
+         * could have moved it. */
+        if (!isfirst)
+        {
+            cyg_uint8 *buf;
+            size_t size;
+            int err = findbuffer_node( dir, prev_pos, &buf, &size, false );
+            if( err != ENOERR ) return err;
+
+            dp = (ramfs_dirent *)buf;
+        } else
+        {
+        	dp = NULL;
+        }
+
         // d now points to a free dirent structure
 
         d->node         = node;
         d->inuse        = 1;
         d->first        = isfirst;
@@ -1165,10 +1181,11 @@ static int add_direntry( ramfs_node *dir
 
         memcpy( d->name, name, fraglen );
 
         name            += fraglen;
         namelen         -= fraglen;
+        prev_pos		= pos;
         pos             += sizeof(ramfs_dirent);
         dp              = d;
         isfirst         = false;
         
     }

Re: Data corruption in ramfs

[Sergei Gavrikov]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 465 bytes --]

On Mon, 22 Feb 2010, Øyvind Harboe wrote:

> The patch closes a *tiny* window where the pointer can change. With 
> longer filenames it is more likely that this can happen, but I suppose 
> shorter filenames are not immune to these problems...
>
> Thanks to Laurentiu Cocanu(laurentiu.cocanu@zylin.com) for spotting 
> this!

Now checked-in.

Acknowledgments

   Øyvind Harboe (original contribution)

   Jonathan Larmour for reasonable view


Thanks!

Sergei