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

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

[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;
 	return buf == tmp ? strdup(buf) : buf;
 }
-- 
1.8.4

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

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 1591 bytes --]

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.

>  	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 

So I would think that strdup(buf) should be replaced by something like

strcpy(malloc(size), buf)

Jens

-- 
:: INRIA Nancy Grand Est :: http://www.loria.fr/~gustedt/   ::
:: AlGorille ::::::::::::::: office Nancy : +33 383593090   ::
:: ICube :::::::::::::: office Strasbourg : +33 368854536   ::
:: ::::::::::::::::::::::::::: gsm France : +33 651400183   ::
:: :::::::::::::::::::: gsm international : +49 15737185122 ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Rich Felker]

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?

Rich

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

[Justin Cormack]

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

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

[Rich Felker]

On Mon, Oct 07, 2013 at 06:15:24PM +0100, Justin Cormack wrote:
> > 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.

Well if the caller requested a size of 2*PATH_MAX and you only
allocated PATH_MAX, this could result in the program invoking UB at a
later time by trying to use the additional space (for whatever
purpose). And conversely, the application _could_ detect allocation of
too much space, if it expected the call to fail with an error but
instead the call succeeded, or if it simply expected that, on
successful return, strlen(getcwd(0, size))<size is true.

Rich

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

[Michael Forney]

On Mon, 07 Oct 2013 12:21:57 -0400, Rich Felker <dalias@aerifal.cx> wrote:
> In principle the kernel should be generating the EINVAL if size is 0,
> but maybe it does the wrong thing...?

Yeah, from what I could tell, it returns ERANGE in all cases where it
can't fit the cwd into the buffer.

-- 
Michael Forney <mforney@mforney.org>

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

[Rich Felker]

On Mon, Oct 07, 2013 at 04:29:53PM -0700, Michael Forney wrote:
> On Mon, 07 Oct 2013 12:21:57 -0400, Rich Felker <dalias@aerifal.cx> wrote:
> > In principle the kernel should be generating the EINVAL if size is 0,
> > but maybe it does the wrong thing...?
> 
> Yeah, from what I could tell, it returns ERANGE in all cases where it
> can't fit the cwd into the buffer.

OK, I think that clears it up then. For now I'll apply your patch
(and at the same time, replace the if branch with a proper block
instead of the ugly comma operator hack) and we can worry about the
glibc issue later.

Rich