[Qemu-devel] Use of PATH_MAX

[Qemu-devel] Use of PATH_MAX

[Ian Jackson]

There are a couple of places where we use PATH_MAX.  I don't think
this is right.  PATH_MAX is a #define specified by POSIX, SuSv3 etc.
But it isn't guaranteed to be defined or necessarily very useful.

In particular, it may be defined to a very large value (larger than a
practical static buffer).  Or on systems where the maximum pathname
length varies (for example, it depends on the underlying filesystem)
it may be not defined at all and applications which really need to
know are supposed to use pathconf.

I think it would be better to invent a new name for the maximum path
length supported by qemu's statically-sized buffers.  This would
replace both the uses of PATH_MAX (in block.c, linux-user/path.c, and
block-vvfat.c) but also direct use of (eg) 1024 in many places.

Before I submit a patch, which will necessarily be very large (and
before I store up future conflicts by pushing similar patches to the
various Xen branches) I thought I would ask here:
  * Do we agree that this is a good idea ?
  * What should the new name be ?  I propose  QEMU_PATH_MAX
    as a nice simple change.

The rune
  egrep -R 'char.*\[[0-9]' .
gives a list of possible numerically-defined static buffers.  Many of
these aren't filenames but some of them are.  Nearly all of the hits
from
  egrep -R 'char.*1024' .
are ripe for changing, so I would start with submitting a patch to
change those (and existing uses of PATH_MAX).

I'd appreciate a go-ahead from a qemu cvs committer before I prepare
(and locally commit) this blanket change.

Thanks,
Ian.

Re: [Qemu-devel] Use of PATH_MAX

[Anthony Liguori]

Ian Jackson wrote:
> There are a couple of places where we use PATH_MAX.  I don't think
> this is right.  PATH_MAX is a #define specified by POSIX, SuSv3 etc.
> But it isn't guaranteed to be defined or necessarily very useful.
>
> In particular, it may be defined to a very large value (larger than a
> practical static buffer).  Or on systems where the maximum pathname
> length varies (for example, it depends on the underlying filesystem)
> it may be not defined at all and applications which really need to
> know are supposed to use pathconf.
>
> I think it would be better to invent a new name for the maximum path
> length supported by qemu's statically-sized buffers.  This would
> replace both the uses of PATH_MAX (in block.c, linux-user/path.c, and
> block-vvfat.c) but also direct use of (eg) 1024 in many places.
>   

It would be far better to get rid of instances of PATH_MAX and replace 
them with dynamically allocated buffers.   The use of static sized 
buffers for filenames is just asking for subtle bugs (and possibly even 
security problems.

Regards,

Anthony Liguori

Re: [Qemu-devel] Use of PATH_MAX

[Paul Brook]

On Friday 16 May 2008, Ian Jackson wrote:
> There are a couple of places where we use PATH_MAX.  I don't think
> this is right.  PATH_MAX is a #define specified by POSIX, SuSv3 etc.
> But it isn't guaranteed to be defined or necessarily very useful.
>
> In particular, it may be defined to a very large value (larger than a
> practical static buffer).  Or on systems where the maximum pathname
> length varies (for example, it depends on the underlying filesystem)
> it may be not defined at all and applications which really need to
> know are supposed to use pathconf.
>
> I think it would be better to invent a new name for the maximum path
> length supported by qemu's statically-sized buffers.  This would
> replace both the uses of PATH_MAX (in block.c, linux-user/path.c, and
> block-vvfat.c) but also direct use of (eg) 1024 in many places.

Better would be to not have the static length buffers in the first place. 
That's the whole reason you're seeing this problem in the first place. 

And yes, I do have real situation where a 1024 character buffer isn't big 
enough.

Paul

Re: [Qemu-devel] Use of PATH_MAX

[Warner Losh]

From: Anthony Liguori <anthony@codemonkey.ws>
Subject: Re: [Qemu-devel] Use of PATH_MAX
Date: Fri, 16 May 2008 09:00:39 -0500

> Ian Jackson wrote:
> > There are a couple of places where we use PATH_MAX.  I don't think
> > this is right.  PATH_MAX is a #define specified by POSIX, SuSv3 etc.
> > But it isn't guaranteed to be defined or necessarily very useful.
> >
> > In particular, it may be defined to a very large value (larger than a
> > practical static buffer).  Or on systems where the maximum pathname
> > length varies (for example, it depends on the underlying filesystem)
> > it may be not defined at all and applications which really need to
> > know are supposed to use pathconf.
> >
> > I think it would be better to invent a new name for the maximum path
> > length supported by qemu's statically-sized buffers.  This would
> > replace both the uses of PATH_MAX (in block.c, linux-user/path.c, and
> > block-vvfat.c) but also direct use of (eg) 1024 in many places.
> >   
> 
> It would be far better to get rid of instances of PATH_MAX and replace 
> them with dynamically allocated buffers.   The use of static sized 
> buffers for filenames is just asking for subtle bugs (and possibly even 
> security problems.

As is the use of dynamic buffers.  If you don't always test system
call return value, you can get odd new failures.  If you don't provide
a sane upper bound, then you get DoS attacks...

Warner

Re: [Qemu-devel] Use of PATH_MAX

[Ian Jackson]

Paul Brook writes ("Re: [Qemu-devel] Use of PATH_MAX"):
> On Friday 16 May 2008, Ian Jackson wrote:
> > I think it would be better to invent a new name for the maximum path
> > length supported by qemu's statically-sized buffers.  This would
> > replace both the uses of PATH_MAX (in block.c, linux-user/path.c, and
> > block-vvfat.c) but also direct use of (eg) 1024 in many places.
> 
> Better would be to not have the static length buffers in the first place. 
> That's the whole reason you're seeing this problem in the first place. 

I agree that it would be better to get rid of the static buffers.  But
at the moment I'm just trying to stop a tide of PATH_MAX nonsense
enveloping the Xen semi-fork.  I don't have time right now to make
dozens of static buffers dynamically allocated.

> And yes, I do have real situation where a 1024 character buffer isn't big 
> enough.

If we changed the constant to have a different name, and used it
consistently, you could at least recompile to get a bigger limit ...

Ian.

Re: [Qemu-devel] Use of PATH_MAX

[Ian Jackson]

iwj writes ("Re: [Qemu-devel] Use of PATH_MAX"):
> I agree that it would be better to get rid of the static buffers.  But
> at the moment I'm just trying to stop a tide of PATH_MAX nonsense
> enveloping the Xen semi-fork.  [...]

Sorry, let me be clearer: in the Xen version of qemu, which I'm trying
to bring more into line with upstream, people are seeing PATH_MAX in
some places and changing fixed constants 1024 to PATH_MAX because they
think that PATH_MAX is the right thing to do.

If I can get agreement on a new name to use in qemu proper then it
will be easy to change to use that everywhere.

Of course if someone wants to do the work to make the buffers dynamic
then that is good but that's quite a bit of work and is likely to
introduce the occasional memory management bug, so I don't think that
changing 1024 and PATH_MAX to QEMU_PATH_MAX should wait for it.

Ian.

Re: [Qemu-devel] Use of PATH_MAX

[Anthony Liguori]

Ian Jackson wrote:
> iwj writes ("Re: [Qemu-devel] Use of PATH_MAX"):
>   
>> I agree that it would be better to get rid of the static buffers.  But
>> at the moment I'm just trying to stop a tide of PATH_MAX nonsense
>> enveloping the Xen semi-fork.  [...]
>>     
>
> Sorry, let me be clearer: in the Xen version of qemu, which I'm trying
> to bring more into line with upstream, people are seeing PATH_MAX in
> some places and changing fixed constants 1024 to PATH_MAX because they
> think that PATH_MAX is the right thing to do.
>
> If I can get agreement on a new name to use in qemu proper then it
> will be easy to change to use that everywhere.
>
> Of course if someone wants to do the work to make the buffers dynamic
> then that is good but that's quite a bit of work and is likely to
> introduce the occasional memory management bug, so I don't think that
> changing 1024 and PATH_MAX to QEMU_PATH_MAX should wait for it.
>   

I don't see any value in converting PATH_MAX to QEMU_PATH_MAX.

Both are equally wrong.

Regards,

Anthony Liguori

> Ian.
>
>
>   

Re: [Qemu-devel] Use of PATH_MAX

[Anthony Liguori]

Warner Losh wrote:
> From: Anthony Liguori <anthony@codemonkey.ws>
> Subject: Re: [Qemu-devel] Use of PATH_MAX
> Date: Fri, 16 May 2008 09:00:39 -0500
>
> As is the use of dynamic buffers.  If you don't always test system
> call return value, you can get odd new failures.  If you don't provide
> a sane upper bound, then you get DoS attacks...
>   

Guests don't provide filenames so no, there is no DoS attack.  As long 
as you handle allocation failures gracefully, it's fine.

The problem with static buffers is that we silently truncate filenames.  
At best, this means something that should work, won't.  At worst, this 
means that instead of opening the file you meant to open, you'll open a 
file that you didn't mean to open and overwrite the data.

Regards,

Anthony Liguori

> Warner
>   

Re: [Qemu-devel] Use of PATH_MAX

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] Use of PATH_MAX"):
> I don't see any value in converting PATH_MAX to QEMU_PATH_MAX.
> Both are equally wrong.

We can
  #define QEMU_PATH_MAX 1024
which is not wrong; it's just not the best most featureful possible
program.

Relying on the host headers to provide a sensible value of PATH_MAX
definitely is wrong - see the SuSv3 entry on <limits.h>.  PATH_MAX may
be not defined at all, or it may be defined to a ridiculously large
value (2^30ish or something).

Ian.

Re: [Qemu-devel] Use of PATH_MAX

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] Use of PATH_MAX"):
> The problem with static buffers is that we silently truncate filenames.

Obviously silently truncating pathnames is a bug.

But even being able to increase the size of the buffer so that the
truncation doesn't happen in practice is a helpful workaround, and not
(mis)using PATH_MAX is an improvement.

Please do not let my proposed change get stuck just because it doesn't
fix every bug and doesn't implement every currently-absent desirable
behaviour.

Ian.