Mixup with name_len & file_type in e2fsprogs

Mixup with name_len & file_type in e2fsprogs

[Jan Kara]

  Hello,

  I was looking into a bug where application using e2fslib was complaining
about file_type > 7. Now the problem is that this is on big endian system
and ext2fs_dir_iterate() ends up calling ext2fs_dirent_swab_in() without
EXT2_DIRBLOCK_V2_STRUCT flag set so name_len is treated as 2 byte and
swapped. There's actually no way to pass that flag from directory iteration
functions to ext2fs_dirent_swab_in() and even worse looking into various
directory handling code in libext2fs if that flag was *ever* used, they
will get seriously confused because they expect (dirent->name_len & 0xff)
to be the real name length but on big endian systems with
EXT2_DIRBLOCK_V2_STRUCT set that isn't true anymore (real name length ends
up in the high byte of the word).

The application could obviously just do what libext2fs does all the time
and use old struct ext2_dir_entry, extract file_type as (name_len >> 8), and
name length as (name_len & 0xff) but that just seems wrong... Rather
directory functions in libext2fs should use EXT2_FEATURE_INCOMPAT_FILETYPE
to recognize what directory structure to use and act accordingly. But that
would be a non-trivial change and we'll have to update also some user
interfaces. So would people find such cleanup useful?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Mixup with name_len & file_type in e2fsprogs

[Theodore Ts'o]

On Tue, Mar 05, 2013 at 01:18:56PM +0100, Jan Kara wrote:
>   Hello,
> 
>   I was looking into a bug where application using e2fslib was complaining
> about file_type > 7. Now the problem is that this is on big endian system
> and ext2fs_dir_iterate() ends up calling ext2fs_dirent_swab_in() without
> EXT2_DIRBLOCK_V2_STRUCT flag set so name_len is treated as 2 byte and
> swapped.

Well, the application most be passing a pointer to treating a pointer
to a struct ext2_dir_entry as a struct dir_entry_2, right?  So it's
technically doing something wrong it sounds like.

The ext2_dir_entry_2 was probably a mistake, and it's hardly used at
all in e2fsprogs.  I wonder if we would be better off not trying to
support it at all, and perhaps adding better accessor functions for
struct ext2_dir_entry instead.

						- Ted

Re: Mixup with name_len & file_type in e2fsprogs

[Jan Kara]

On Tue 05-03-13 13:29:46, Ted Tso wrote:
> On Tue, Mar 05, 2013 at 01:18:56PM +0100, Jan Kara wrote:
> >   Hello,
> > 
> >   I was looking into a bug where application using e2fslib was complaining
> > about file_type > 7. Now the problem is that this is on big endian system
> > and ext2fs_dir_iterate() ends up calling ext2fs_dirent_swab_in() without
> > EXT2_DIRBLOCK_V2_STRUCT flag set so name_len is treated as 2 byte and
> > swapped.
> 
> Well, the application most be passing a pointer to treating a pointer
> to a struct ext2_dir_entry as a struct dir_entry_2, right?  So it's
> technically doing something wrong it sounds like.
  Yes, that is correct. Although it is easy to think along the lines of
"I know the dirents are in v2 format but there's no way to iterate over a
directory and get v2 dirent so I will just cast the v1 argument." And
although that is wrong as you point out I did that myself a few times
because I didn't know a better way.
 
> The ext2_dir_entry_2 was probably a mistake, and it's hardly used at
> all in e2fsprogs.  I wonder if we would be better off not trying to
> support it at all, and perhaps adding better accessor functions for
> struct ext2_dir_entry instead.
  Yeah, that sounds like an easier solution. I'll write a patch for that.
Thanks for the idea.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Mixup with name_len & file_type in e2fsprogs

[Jan Kara]

On Tue 05-03-13 19:41:38, Jan Kara wrote:
> On Tue 05-03-13 13:29:46, Ted Tso wrote:
> > On Tue, Mar 05, 2013 at 01:18:56PM +0100, Jan Kara wrote:
> > >   Hello,
> > > 
> > >   I was looking into a bug where application using e2fslib was complaining
> > > about file_type > 7. Now the problem is that this is on big endian system
> > > and ext2fs_dir_iterate() ends up calling ext2fs_dirent_swab_in() without
> > > EXT2_DIRBLOCK_V2_STRUCT flag set so name_len is treated as 2 byte and
> > > swapped.
> > 
> > Well, the application most be passing a pointer to treating a pointer
> > to a struct ext2_dir_entry as a struct dir_entry_2, right?  So it's
> > technically doing something wrong it sounds like.
>   Yes, that is correct. Although it is easy to think along the lines of
> "I know the dirents are in v2 format but there's no way to iterate over a
> directory and get v2 dirent so I will just cast the v1 argument." And
> although that is wrong as you point out I did that myself a few times
> because I didn't know a better way.
>  
> > The ext2_dir_entry_2 was probably a mistake, and it's hardly used at
> > all in e2fsprogs.  I wonder if we would be better off not trying to
> > support it at all, and perhaps adding better accessor functions for
> > struct ext2_dir_entry instead.
>   Yeah, that sounds like an easier solution. I'll write a patch for that.
> Thanks for the idea.
  I was thinking some more about it. Won't it be cleaner (and not much
harder) if we always used ext2_dir_entry_2 in e2fsprogs and properly did
byte swapping of name_len only if FILETYPE feature isn't enabled? I've
checked and it would mean changing prototypes of like 8 functions in
ext2fs.h (or better provide new functions with struct ext2_dir_entry_2
argument) which doesn't seem too bad... This solution seems to be the least
surprising to the users of libext2fs (esp. if they do some scanning of
directory blocks themselves or so).

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Mixup with name_len & file_type in e2fsprogs

[Andreas Dilger]

On 2013-03-08, at 2:24 AM, Jan Kara wrote:
Won't it be cleaner (and not much
> harder) if we always used ext2_dir_entry_2 in e2fsprogs and properly
> did byte swapping of name_len only if FILETYPE feature isn't enabled?
> I've checked and it would mean changing prototypes of like 8
> functions in ext2fs.h (or better provide new functions with struct
> ext2_dir_entry_2 argument) which doesn't seem too bad... This
> solution seems to be the least surprising to the users of libext2fs
> (esp. if they do some scanning of directory blocks themselves or so).

I've wanted to do this for a while as well.  Or even go further and
just deprecate support for filesystems that don't have the FILETYPE
feature (which has been supported since Linux 1.2 or even earlier).

Cheers, Andreas

Re: Mixup with name_len & file_type in e2fsprogs

[Theodore Ts'o]

On Thu, Mar 07, 2013 at 07:24:44PM +0100, Jan Kara wrote:
>   I was thinking some more about it. Won't it be cleaner (and not much
> harder) if we always used ext2_dir_entry_2 in e2fsprogs and properly did
> byte swapping of name_len only if FILETYPE feature isn't enabled? I've
> checked and it would mean changing prototypes of like 8 functions in
> ext2fs.h (or better provide new functions with struct ext2_dir_entry_2
> argument) which doesn't seem too bad... This solution seems to be the least
> surprising to the users of libext2fs (esp. if they do some scanning of
> directory blocks themselves or so).

What worries me is potential ABI breackages if we try to redefine
existing interfaces as using ext2_dir_entry_2.  And I'm not convinced
it's worth it to create new versions of the functions that use the new
structure.  This is why I suggested using accessor functions....

	    	    	  	    	  	   - Ted

Re: Mixup with name_len & file_type in e2fsprogs

[Theodore Ts'o]

By the way, why was the application trying to access the directory
entries directly, as opposed to using some of the higher level
functions in libext2fs?  What was it trying to do?

Is there some new higher-level functionality we should be providing?
Or was this just an application that didn't know about some better
interface that it could have used?  Is this an open source
application?  Can you send me a pointer to it?

	     		      	     - Ted

Re: Mixup with name_len & file_type in e2fsprogs

[Jan Kara]

On Thu 07-03-13 19:46:20, Ted Tso wrote:
> By the way, why was the application trying to access the directory
> entries directly, as opposed to using some of the higher level
> functions in libext2fs?  What was it trying to do?
  It was btrfs-convert (I could have told that in my original email as well
but it didn't seem important) - i.e. inplace convertor from ext4 to btrfs.
So it really needed to get the file type from the directory block to fill
in appropriate btrfs structures.

> Is there some new higher-level functionality we should be providing?
> Or was this just an application that didn't know about some better
> interface that it could have used?  Is this an open source
> application?  Can you send me a pointer to it?
  btrfs-convert is part of btrfsprogs so feel free to have a look. I don't
think we can actually provide some high level functionality to serve its
needs. It really just needs to do readdir(2) on fs image which is what is
already implemented in libext2fs.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR