Re: [bug #28446] No checks are made for unteminated strings in RPC messages

[Carl Fredrik Hammar]

Hi,

On Sat, Jan 09, 2010 at 07:10:30PM +0100, olafBuddenhagen@gmx.net wrote:
> On Thu, Jan 07, 2010 at 04:45:57PM +0100, Carl Fredrik Hammar wrote:
> > On Mon, Jan 04, 2010 at 03:34:07PM +0100, olafBuddenhagen@gmx.net
> > wrote:
> > > On a technical side, you have to decide what error code MiG should
> > > return in this case. I don't think there is any appropriate one yet,
> > > which means you will have to add a new one, and make clients cope
> > > with it... That's what I mean by invasive -- it could have
> > > far-reaching consequences.
> > 
> > This would be invasive, but luckily there's already the
> > MIG_BAD_ARGUMENTS, which is used in all other failed server-side type
> > checks.
> 
> OK.
> 
> Still, there might be other implications...

I guess some clients might get even more confused to get MIG_BAD_ARGUMENTS
instead of ENOENT.  But it is almost certain it would result in an error
of some sort.

> > Technically, you can look up a path of arbitrary length using the
> > existing RPC interface, you just have to do it in multiple steps.
> 
> Right...
> 
> > Of course, glibc doesn't do anything to break it up, so effectively we
> > do have PATH_MAX=1024 :-/
> 
> It's even more confused in fact: If you have retries in between, you can
> actually use much longer paths... Only if a *single* server has to
> handle longer filename pieces, it breaks.

Actually, the path would get truncated to 1024 bytes and left unterminated
before it is sent to the server, which would return a retry with the
remainder of the truncated path.  The retry will most likely fail with
ENOENT, just as it would if the entire truncated path had been handled
by the first server.

The fact that client stubs does not terminate long strings is something
I discovered recently.  I mistakenly assumed that strncpy did that.
The client stub should also fail with an error if it is too long.

> > Of course, fixing MIG might actually be easier.
> 
> Not sure whether it would be easier... But it definitely does seem the
> right thing to do.

Well, it depends on the reason unbounded strings aren't supported.
I'm hoping it's completely arbitrary and only a matter of making
the parser accept the unbounded array syntax for strings as well.

I'm not sure how it would affect the resulting C type used for the stub
functions' arguments.  I think input arguments would remain the same,
but output arguments would become char** instead of char*.  Perhaps it
can be tweaked somehow.

> The "XXX" comment on the definition of string_t indicates that it *was*
> meant to be temporary only.

Indeed.

> > It does limit the length of a single component in the path though, but
> > again glibc already places tighter limits in the dirent structure by
> > using a 256 long char array when listing directories.  So there's a
> > lot more to this then just fixing MIG.
> 
> Indeed. This is less obvious, as actual disk filesystems have limits on
> that anyways... But it would be nice at least for virtual filesystems
> not to have such limitations.
> 
> (Whether it's worthwhile creating disk filesystems without such a
> limitation is another can of worms...)
> 
> BTW, there is no macro for the maximal length of a single file name
> component, is there?... I guess that would have been too useful, as it
> actually matches reality ;-)

Ugh, it seems I confused the Linux's dirent.h with the Hurd's.

I could swear I saw a sizeof(((struct dirent *) NULL)->d_name) somewhere
in ext2fs or libdiskfs, but I can't find it now.  Perhaps it was in a
dream or something, or should I say nightmare.  ;-)

Anyway, ext2fs just defines a limit of 256 on its own, and glibc's
dirent is variable sized.  It seems all is good, though I might test
the interface later to make sure it supports larger entries.

Regards,
  Fredrik