Re: [PATCH] getcwd: Set errno to EINVAL when size == 0

[Justin Cormack <justin@specialbusservice.com>]

On Mon, Oct 7, 2013 at 5:21 PM, Rich Felker <dalias@aerifal.cx> wrote:
> On Mon, Oct 07, 2013 at 08:38:14AM +0200, Jens Gustedt wrote:
>> Hello,
>>
>> Am Sonntag, den 06.10.2013, 23:08 -0700 schrieb Michael Forney:
>> > According to POSIX,
>> >
>> >     The getcwd() function shall fail if:
>> >
>> >     [EINVAL]
>> >     The size argument is 0.
>> >     [ERANGE]
>> >     The size argument is greater than 0, but is smaller than the length
>> >     of the string +1.
>> > ---
>> >  src/unistd/getcwd.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/src/unistd/getcwd.c b/src/unistd/getcwd.c
>> > index 2e540cd..0238fa7 100644
>> > --- a/src/unistd/getcwd.c
>> > +++ b/src/unistd/getcwd.c
>> > @@ -8,6 +8,10 @@ char *getcwd(char *buf, size_t size)
>> >  {
>> >     char tmp[PATH_MAX];
>> >     if (!buf) buf = tmp, size = PATH_MAX;
>> > +   else if (size == 0) {
>> > +           errno = EINVAL;
>> > +           return 0;
>> > +   }
>> >     if (syscall(SYS_getcwd, buf, size) < 0) return 0;
>>
>> Is the new error check really necessary?  I would have expected the
>> error path to have triggered before when buf is !0 and size is 0 on
>> entry.
>
> In principle the kernel should be generating the EINVAL if size is 0,
> but maybe it does the wrong thing...?
>
>> >     return buf == tmp ? strdup(buf) : buf;
>>
>> This in turn doesn't seem to be consistent with the extension that
>> glibc offers. It says
>>
>> > In  this case, the allocated buffer has the length size
>
> You omitted the rest of that sentence: "unless size is zero, when buf
> is allocated as big as necessary."
>
>> So I would think that strdup(buf) should be replaced by something like
>>
>> strcpy(malloc(size), buf)
>
> This is definitely unsafe if size is less than strnel(buf)+1. I'm not
> convinced this aspect of the glibc behavior (using the size argument)
> is beneficial; the only possible case in which it would be benficial
> is when the caller wants the returned buffer to have space for
> appending a filename, which could be achieved by passing PATH_MAX.
> However, I thought the whole point of having getcwd accept a NULL
> argument was for the GNU HURD "no PATH_MAX limit" model, in which case
> you wouldn't even know the right length to pass in order to have space
> left over to append a filename.
>
> If it is deemed important to support this weird GNU behavior, I think
> it would be beneficial to always allocate MAX(strlen(buf)+1,size)
> rather than just size, to avoid spurious failure.
>
> Opinions from anyone else?

I can't see any way in which the user could detect (in the malloc
case) that you always allocated PATH_MAX not the provided size, so you
may as well just do that if they insist on using this stupid interface
in the first place.

Justin