Re: RAMFS fixes: file permissions and lseek()

[Andrew Lunn <andrew at lunn dot ch>]

On Mon, Oct 03, 2005 at 02:19:08PM -1000, Dan Jakubiec wrote:
> Hi Andrew,
> 
> Thanks for the review and the comments.  Your suggestions look like good 
> improvements.  My responses and questions:
> 
> Andrew Lunn wrote:
> 
> >OK. But i really thing it is the application which is broken. eCos has
> >no concept of filesystem security, there is no way to set permissions,
> >owners, groups etc. So any application which uses this is probably
> >broken.
> >
> > 
> >
> Well, perhaps...  But consider this:  we ran into this permissions 
> problem when porting a POSIX application to eCos.  The application 
> happened to be the eCos mk_romfs.c host-side utility for generating 
> ROMFS images.  [our eCos environment needs the ability to generate ROMFS 
> images at runtime based on our RAMFS root filesystem]
> 
> This POSIX app (like others I'm sure), checks each file to make sure it 
> is readable prior to building its ROMFS image.  Without the fix, the 
> POSIX app won't proceed with the ROMFS generation because it expects 
> that its planned open() calls will fail.  We could hack mk_romfs to 
> behave differently under eCos, but I think there is a more elegant solution.
> 
> The RAMFS fix is rather reasonable because although file/directory 
> permissions can not be *set* (nor enforced), than can still be read by 
> applications and therefore should reflect the reality of the file 
> security.  In the eCos RAMFS filesystem:
> 
>   1. All files are readable and writable by anyone, so files should at
>      minimum report a permissions mask of 666.
>   2. The executable bit is debatable.  However, I don't think it hurts
>      to set it so I did.  This will keep happy those ported apps which
>      expect to find executable files with correct perms.  Of course,
>      these files aren't truly executable.  If you think 666 is more
>      logical here, then I'm open to your thinking.  777 just seemed
>      more all-encompassing.
>   3. For directories: all directories are readable, writable, and
>      viewable by all.  So 777 seemed the most appropriate.
>   4. A permission mask of 000 seems wrong on all accounts.  Because
>      although it won't protect the files from being used, it will
>      misrepresent their true read/write characteristics in the stat() call.

I don't disagree with this, which is why i accepted the patch. I just
think it might be better to return 000 so that the application is more
likely to fail. You then have to take a look at the code and work out
the implications of the lack of security and what is the application
trying to do. In your case it is not a problem there is no security,
but in other cases it might be a big problem....

> >I fixed this differently, in a more efficient way. For the default
> >"SIMPLE" allocation mechanism, there was already a memzero setting the
> >contents to zero. It just needed tweeking a little. For the "BLOCK"
> >allocation mechanism i implemented true holes. So you can do something
> >like
> >
> >fd = open("foobar", O_RDWR);
> >lseek(fd, 1024*1024, SEEK_SET);
> >write(fd, '1', 1);
> >
> >and it will only allocation one 256 byte block, not a megabyte.
> > 
> >
> >
> This sound like a better fix, but I had once concern.  I haven't 
> familiarized myself with the BLOCK approach, so perhaps you can fill in 
> the blanks here.  According to POSIX, once you write() to the file at 
> the post-EOF position, all reads between the old EOF and the new write 
> data must now return byte 0x00 in each unwritten location.  In other 
> words, the act of the write() is supposed to increase the file size by 
> the number of write bytes plus the gap size.  In other words:
> 
>   1. fd = open()
>   2. write(fd, "hi", 2);      //lpos at 2
>   3. lseek(fd, 4, SEEK_SET);  //lpos at 4
>   4. write(fd, "you", 3);     //lpos at 7
>   5. lseek(fd, 0, SEEK_SET);  //lpos at 0
>   6. read(fd, buffer, 100) ==> read() returns 7, buffer contains
>      "hi\x00\x00you"
> 
> I will study your improvements, but will they indeed return the 0x00 
> bytes as required by POSIX?

Yes, it should. 

How it works is that the code which finds the blocks when performing a
read will return a null pointer if the block does not exist. If i get
a null pointer i do a memset to zero. If a get something that is not a
null pointer i do a memcpy. It is only when you do a write does it
actually allocate the block. This lazy allocation of blocks is very
common in filesystems and often used in database systems. Take a look
at the man page of ndbm.

Having said that, your test case is actually a little different since
the writes will occur inside the same block since blocks default to
256 bytes. The first write will cause the block to the
allocated. During this allocation the entire block is zero'd.

My test case actually tests writes/reads to different blocks since i
use 1K reads/writes in my test. If you want to test your scenario
change the size of buf and buf1 from 1024 to 8 and it should then be
equivalent to what you said above.

        Andrew