mailing list of musl libc
 help / color / mirror / Atom feed
* [musl] realpath without procfs -- should be ready for inclusion
@ 2020-11-22 22:56 Rich Felker
  2020-11-23  2:03 ` Alexey Izbyshev
  0 siblings, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-11-22 22:56 UTC (permalink / raw)
  To: musl

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

I put this aside for a while, but realpath without procfs was one of
the things I wanted to get in this release cycle, and I had it
essentially finished back in September. Here's the latest version.
Compared to the earlier draft, it handles // more consistently (//.
and //.. both resolve to //) and expands getcwd only as a final step
if needed, rather than doing it first. In the case where an absolute
symlink is reached via a relative path, this saves a syscall, avoids
repeated absolute path traversal at each step, and avoids spuriously
erroring out with ENAMETOOLONG when the working directory is deeper
than PATH_MAX but the absolute link target has a valid absolute
pathname.

To recap, the motivation for the rewrite without procfs dependency was
having things work before procfs is mounted or in chroot/namespace
where it's intentionally unavailable.

I originally considered keeping the procfs based version and only
using the new one as a fallback, but I discovered there are cases
(involving chroot, namespaces, etc.) where the answer from procfs is
wrong and validating it requires basically the same procedure as
implementing it manually (walking and performing readlink on each path
component).

Rich

[-- Attachment #2: realpath8.c --]
[-- Type: text/plain, Size: 2392 bytes --]

#define _GNU_SOURCE
#include <stdlib.h>
#include <limits.h>
#include <errno.h>
#include <unistd.h>
#include <string.h>

static inline int at_dotdot(const char *end, size_t len)
{
	if (len<2) return 0;
	if (len>2 && end[-3]!='/') return 0;
	return end[-1]=='.' && end[-2]=='.';
}

char *realpath(const char *restrict filename, char *restrict resolved)
{
	char stack[PATH_MAX];
	char buf[resolved ? 1 : PATH_MAX];
	char *output = resolved ? resolved : buf;
	size_t p, q, l, cnt=0;

	l = strnlen(filename, sizeof stack + 1);
	if (!l) {
		errno = ENOENT;
		return 0;
	}
	if (l >= sizeof stack) goto toolong;
	p = sizeof stack - l - 1;
	q = 0;
	memcpy(stack+p, filename, l+1);

	while (stack[p]) {
		int up = 0;
		if (stack[p] == '/') {
			q=0;
			output[q++] = '/';
			p++;
			/* Initial // is special. */
			if (stack[p] == '/' && stack[p+1] != '/') {
				output[q++] = '/';
			}
			while (stack[p] == '/') p++;
			continue;
		}
		char *z = __strchrnul(stack+p, '/');
		l = z-(stack+p);
		if (l==1 && stack[p]=='.') {
			p += l;
			while (stack[p] == '/') p++;
			continue;
		}
		if (at_dotdot(stack+p+l, l)) {
			if (q && !at_dotdot(output+q, q)) {
				while(q && output[q-1]!='/') q--;
				if (q>1 && (q>2 || output[0]!='/')) q--;
				p += l;
				while (stack[p] == '/') p++;
				continue;
			}
			up = 1;
		}
		if (q && output[q-1] != '/') {
			if (!p) goto toolong;
			stack[--p] = '/';
			l++;
		}
		if (q+l >= PATH_MAX) goto toolong;
		memcpy(output+q, stack+p, l);
		output[q+l] = 0;
		p += l;
		if (up) goto notlink;
		ssize_t k = readlink(output, stack, p);
		if (k==-1) {
			if (errno == EINVAL) {
notlink:
				q += l;
				while (stack[p] == '/') p++;
				continue;
			}
			return 0;
		}
		if (k==p) goto toolong;
		if (++cnt == SYMLOOP_MAX) {
			errno = ELOOP;
			return 0;
		}
		p -= k;
		memmove(stack+p, stack, k);
	}

	output[q] = 0;

	if (output[0] != '/') {
		if (!getcwd(stack, sizeof stack)) return 0;
		l = strlen(stack);
		/* Cancel any initial .. components. */
		p = 0;
		while (q-p>=2 && at_dotdot(output+p+2, p+2)) {
			while(l>1 && stack[l-1]!='/') l--;
			if (l>1) l--;
			p += 2;
			if (p<q) p++;
		}
		if (q-p && stack[l-1]!='/') output[--p] = '/';
		if (l + (q-p) + 1 >= PATH_MAX) goto toolong;
		memmove(output + l, output + p, q - p + 1);
		memcpy(output, stack, l);
	}

	return resolved ? resolved : strdup(output);

toolong:
	errno = ENAMETOOLONG;
	return 0;
}

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-22 22:56 [musl] realpath without procfs -- should be ready for inclusion Rich Felker
@ 2020-11-23  2:03 ` Alexey Izbyshev
  2020-11-23  3:17   ` Érico Nogueira
  2020-11-23  3:19   ` Rich Felker
  0 siblings, 2 replies; 20+ messages in thread
From: Alexey Izbyshev @ 2020-11-23  2:03 UTC (permalink / raw)
  To: musl

On 2020-11-23 01:56, Rich Felker wrote:
> I originally considered keeping the procfs based version and only
> using the new one as a fallback, but I discovered there are cases
> (involving chroot, namespaces, etc.) where the answer from procfs is
> wrong and validating it requires basically the same procedure as
> implementing it manually (walking and performing readlink on each path
> component).
> 
Pity that the simple and fast procfs-based implementation goes away. Do 
you have any specific example of a wrong answer from procfs at hand, or 
at least a more specific direction to look at than just 
"chroot/namespaces"?

> #define _GNU_SOURCE
> #include <stdlib.h>
> #include <limits.h>
> #include <errno.h>
> #include <unistd.h>
> #include <string.h>
> 
> static inline int at_dotdot(const char *end, size_t len)
> {
>    if (len<2) return 0;
>    if (len>2 && end[-3]!='/') return 0;
>    return end[-1]=='.' && end[-2]=='.';
> }
> 
> char *realpath(const char *restrict filename, char *restrict resolved)
> {
>    char stack[PATH_MAX];
>    char buf[resolved ? 1 : PATH_MAX];
>    char *output = resolved ? resolved : buf;
>    size_t p, q, l, cnt=0;
> 
>    l = strnlen(filename, sizeof stack + 1);

Why + 1?

>    if (!l) {
>        errno = ENOENT;
>        return 0;
>    }
>    if (l >= sizeof stack) goto toolong;
>    p = sizeof stack - l - 1;
>    q = 0;
>    memcpy(stack+p, filename, l+1);
> 
>    while (stack[p]) {
>        int up = 0;
>        if (stack[p] == '/') {
>            q=0;
>            output[q++] = '/';
>            p++;
>            /* Initial // is special. */
>            if (stack[p] == '/' && stack[p+1] != '/') {
>                output[q++] = '/';
>            }
>            while (stack[p] == '/') p++;
>            continue;
>        }
>        char *z = __strchrnul(stack+p, '/');
>        l = z-(stack+p);
>        if (l==1 && stack[p]=='.') {
>            p += l;
>            while (stack[p] == '/') p++;
>            continue;
>        }
>        if (at_dotdot(stack+p+l, l)) {
>            if (q && !at_dotdot(output+q, q)) {
>                while(q && output[q-1]!='/') q--;
>                if (q>1 && (q>2 || output[0]!='/')) q--;
>                p += l;
>                while (stack[p] == '/') p++;
>                continue;
>            }
>            up = 1;
>        }
>        if (q && output[q-1] != '/') {
>            if (!p) goto toolong;
>            stack[--p] = '/';
>            l++;
>        }
>        if (q+l >= PATH_MAX) goto toolong;
>        memcpy(output+q, stack+p, l);
>        output[q+l] = 0;
>        p += l;
>        if (up) goto notlink;
>        ssize_t k = readlink(output, stack, p);
>        if (k==-1) {
>            if (errno == EINVAL) {
> notlink:
>                q += l;
>                while (stack[p] == '/') p++;
>                continue;
>            }
>            return 0;
>        }
>        if (k==p) goto toolong;
>        if (++cnt == SYMLOOP_MAX) {
>            errno = ELOOP;
>            return 0;
>        }
>        p -= k;
>        memmove(stack+p, stack, k);

This isn't always correct if the symlink resolves to "/" because 
stack[p] might be '/', so two slashes will be preserved in the output. 
For example, "root/home" resolves to "//home" (where "root" -> "/").

>    }
> 
>    output[q] = 0;
> 
>    if (output[0] != '/') {
>        if (!getcwd(stack, sizeof stack)) return 0;
>        l = strlen(stack);
>        /* Cancel any initial .. components. */
>        p = 0;
>        while (q-p>=2 && at_dotdot(output+p+2, p+2)) {

This doesn't check that output+p+2 is the end of a path element, which 
is the prerequisite for at_dotdot(). So, for example, "..." resolves 
incorrectly.

>            while(l>1 && stack[l-1]!='/') l--;
>            if (l>1) l--;
>            p += 2;
>            if (p<q) p++;
>        }
>        if (q-p && stack[l-1]!='/') output[--p] = '/';
>        if (l + (q-p) + 1 >= PATH_MAX) goto toolong;
>        memmove(output + l, output + p, q - p + 1);
>        memcpy(output, stack, l);
>    }
> 
>    return resolved ? resolved : strdup(output);
> 
> toolong:
>    errno = ENAMETOOLONG;
>    return 0;
> }
> 

Alexey


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-23  2:03 ` Alexey Izbyshev
@ 2020-11-23  3:17   ` Érico Nogueira
  2020-11-23  3:34     ` Rich Felker
  2020-11-23  3:19   ` Rich Felker
  1 sibling, 1 reply; 20+ messages in thread
From: Érico Nogueira @ 2020-11-23  3:17 UTC (permalink / raw)
  To: musl

On Sun Nov 22, 2020 at 11:03 PM -03, Alexey Izbyshev wrote:
> On 2020-11-23 01:56, Rich Felker wrote:
> > I originally considered keeping the procfs based version and only
> > using the new one as a fallback, but I discovered there are cases
> > (involving chroot, namespaces, etc.) where the answer from procfs is
> > wrong and validating it requires basically the same procedure as
> > implementing it manually (walking and performing readlink on each path
> > component).
> > 
> Pity that the simple and fast procfs-based implementation goes away. Do
> you have any specific example of a wrong answer from procfs at hand, or
> at least a more specific direction to look at than just
> "chroot/namespaces"?

bubblewrap (when driven by Flatpak) is one such software. Void carries
a patch [1] with NetBSD's realpath impl to work around this. Without it,
launching flatpak applications sometimes didn't work at all.

- [1] https://github.com/void-linux/void-packages/blob/da86d30391e2b3535e8f9dfae452d2b362887e41/srcpkgs/bubblewrap/patches/realpath-workaround.patch

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-23  2:03 ` Alexey Izbyshev
  2020-11-23  3:17   ` Érico Nogueira
@ 2020-11-23  3:19   ` Rich Felker
  2020-11-23 18:56     ` Rich Felker
  2020-11-24  3:41     ` Alexey Izbyshev
  1 sibling, 2 replies; 20+ messages in thread
From: Rich Felker @ 2020-11-23  3:19 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

On Mon, Nov 23, 2020 at 05:03:25AM +0300, Alexey Izbyshev wrote:
> On 2020-11-23 01:56, Rich Felker wrote:
> >I originally considered keeping the procfs based version and only
> >using the new one as a fallback, but I discovered there are cases
> >(involving chroot, namespaces, etc.) where the answer from procfs is
> >wrong and validating it requires basically the same procedure as
> >implementing it manually (walking and performing readlink on each path
> >component).
> >
> Pity that the simple and fast procfs-based implementation goes away.
> Do you have any specific example of a wrong answer from procfs at
> hand, or at least a more specific direction to look at than just
> "chroot/namespaces"?

Assuming you even have procfs, the name read from readlink on procfs
will be relative to the real root of the mount namespace, not the
chroot. If the mount namespace has bind mounts over top of anything,
it can also give you a pathname that's no longer valid because
something is mounted over it.

There may be other ways this arises too. At the very least, you need
to do fstat/stat to match them up like we do now; otherwise you can
get wildly wrong results. But even if the stat matches up, it's still
possible that the resulting pathname is an absolute pathname outside
the chroot or behind the bind mount or whatever, but is also valid but
involving symlink traversal in when processed from in the current
process context. This means you return a result that does not satisfy
the contract to be symlink-free.

There's also a matter I didn't mention that the current code is wrong
in an unsafe way on per-O_PATH kernels. Other places we mitigate that
by using O_NOFOLLOW and O_NOCTTY to avoid *most* of the possible
unwanted side effects if opening an actual file on a kernel that
doesn't have O_PATH, but on realpath we specifically can't use
O_NOFOLLOW, and this makes it susceptible to tricking root (or any
user with read access) into opening device nodes, in ways that might
have side effects.

So, there are a lot of bad things about the current implementation.
Even the minor mitigations present now for some of them (the stat
check) along with the overhead (open/close) makes it questionable
whether it's faster for lots of inputs. For deep paths the new one is
probably slower, but for typical ones it's not as clear and I didn't
measure.


> 
> >#define _GNU_SOURCE
> >#include <stdlib.h>
> >#include <limits.h>
> >#include <errno.h>
> >#include <unistd.h>
> >#include <string.h>
> >
> >static inline int at_dotdot(const char *end, size_t len)
> >{
> >   if (len<2) return 0;
> >   if (len>2 && end[-3]!='/') return 0;
> >   return end[-1]=='.' && end[-2]=='.';
> >}
> >
> >char *realpath(const char *restrict filename, char *restrict resolved)
> >{
> >   char stack[PATH_MAX];
> >   char buf[resolved ? 1 : PATH_MAX];
> >   char *output = resolved ? resolved : buf;
> >   size_t p, q, l, cnt=0;
> >
> >   l = strnlen(filename, sizeof stack + 1);
> 
> Why + 1?

I was thinking it was to ensure that the largest possible result is
sufficient to detect ENAMETOOLONG condition, but even == PATH_MAX is
sufficient for that since PATH_MAX is a limit including null
termination. So I think the +1 can be removed.

> >   if (!l) {
> >       errno = ENOENT;
> >       return 0;
> >   }
> >   if (l >= sizeof stack) goto toolong;
> >   p = sizeof stack - l - 1;
> >   q = 0;
> >   memcpy(stack+p, filename, l+1);
> >
> >   while (stack[p]) {
> >       int up = 0;
> >       if (stack[p] == '/') {
> >           q=0;
> >           output[q++] = '/';
> >           p++;
> >           /* Initial // is special. */
> >           if (stack[p] == '/' && stack[p+1] != '/') {
> >               output[q++] = '/';
> >           }
> >           while (stack[p] == '/') p++;
> >           continue;
> >       }
> >       char *z = __strchrnul(stack+p, '/');
> >       l = z-(stack+p);
> >       if (l==1 && stack[p]=='.') {
> >           p += l;
> >           while (stack[p] == '/') p++;
> >           continue;
> >       }
> >       if (at_dotdot(stack+p+l, l)) {
> >           if (q && !at_dotdot(output+q, q)) {
> >               while(q && output[q-1]!='/') q--;
> >               if (q>1 && (q>2 || output[0]!='/')) q--;
> >               p += l;
> >               while (stack[p] == '/') p++;
> >               continue;
> >           }
> >           up = 1;
> >       }
> >       if (q && output[q-1] != '/') {
> >           if (!p) goto toolong;
> >           stack[--p] = '/';
> >           l++;
> >       }
> >       if (q+l >= PATH_MAX) goto toolong;
> >       memcpy(output+q, stack+p, l);
> >       output[q+l] = 0;
> >       p += l;
> >       if (up) goto notlink;
> >       ssize_t k = readlink(output, stack, p);
> >       if (k==-1) {
> >           if (errno == EINVAL) {
> >notlink:
> >               q += l;
> >               while (stack[p] == '/') p++;
> >               continue;
> >           }
> >           return 0;
> >       }
> >       if (k==p) goto toolong;
> >       if (++cnt == SYMLOOP_MAX) {
> >           errno = ELOOP;
> >           return 0;
> >       }
> >       p -= k;
> >       memmove(stack+p, stack, k);
> 
> This isn't always correct if the symlink resolves to "/" because
> stack[p] might be '/', so two slashes will be preserved in the
> output. For example, "root/home" resolves to "//home" (where "root"
> -> "/").

Thanks for catching that. I propose:

	if (stack[k-1]=='/') p++;

And that raises the point that k==0 should be handled, even though
Linux doesn't let you create such links, since in theory they could
come from an existing fs or from non-Linux kernels (see
https://lwn.net/Articles/551224/ for coverage of the topic). I propose
erroring out with ENOENT in this case right after the k==p check above
(since k==0 due to p==0 would bee toolong not ENOENT if it can
happen).

> >   output[q] = 0;
> >
> >   if (output[0] != '/') {
> >       if (!getcwd(stack, sizeof stack)) return 0;
> >       l = strlen(stack);
> >       /* Cancel any initial .. components. */
> >       p = 0;
> >       while (q-p>=2 && at_dotdot(output+p+2, p+2)) {
> 
> This doesn't check that output+p+2 is the end of a path element,
> which is the prerequisite for at_dotdot(). So, for example, "..."
> resolves incorrectly.

Thanks again. I don't see a really good way to reuse at_dotdot here,
do you? Probably just [0]=='.' && [1]=='.' && (![2] || [2]=='/') is
the cleanest.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-23  3:17   ` Érico Nogueira
@ 2020-11-23  3:34     ` Rich Felker
  0 siblings, 0 replies; 20+ messages in thread
From: Rich Felker @ 2020-11-23  3:34 UTC (permalink / raw)
  To: musl

On Mon, Nov 23, 2020 at 12:17:27AM -0300, Érico Nogueira wrote:
> On Sun Nov 22, 2020 at 11:03 PM -03, Alexey Izbyshev wrote:
> > On 2020-11-23 01:56, Rich Felker wrote:
> > > I originally considered keeping the procfs based version and only
> > > using the new one as a fallback, but I discovered there are cases
> > > (involving chroot, namespaces, etc.) where the answer from procfs is
> > > wrong and validating it requires basically the same procedure as
> > > implementing it manually (walking and performing readlink on each path
> > > component).
> > > 
> > Pity that the simple and fast procfs-based implementation goes away. Do
> > you have any specific example of a wrong answer from procfs at hand, or
> > at least a more specific direction to look at than just
> > "chroot/namespaces"?
> 
> bubblewrap (when driven by Flatpak) is one such software. Void carries
> a patch [1] with NetBSD's realpath impl to work around this. Without it,
> launching flatpak applications sometimes didn't work at all.
> 
> - [1] https://github.com/void-linux/void-packages/blob/da86d30391e2b3535e8f9dfae452d2b362887e41/srcpkgs/bubblewrap/patches/realpath-workaround.patch

FWIW this seems to be a reason for needing the real implementation
like proposed, but not a reason for getting rid of the proc-based code
path. The reasons for that I mostly covered in my reply.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-23  3:19   ` Rich Felker
@ 2020-11-23 18:56     ` Rich Felker
  2020-11-23 20:53       ` Rich Felker
  2020-11-24  3:41     ` Alexey Izbyshev
  1 sibling, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-11-23 18:56 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

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

On Sun, Nov 22, 2020 at 10:19:33PM -0500, Rich Felker wrote:
> On Mon, Nov 23, 2020 at 05:03:25AM +0300, Alexey Izbyshev wrote:
> > On 2020-11-23 01:56, Rich Felker wrote:
> > >I originally considered keeping the procfs based version and only
> > >using the new one as a fallback, but I discovered there are cases
> > >(involving chroot, namespaces, etc.) where the answer from procfs is
> > >wrong and validating it requires basically the same procedure as
> > >implementing it manually (walking and performing readlink on each path
> > >component).
> > >
> > Pity that the simple and fast procfs-based implementation goes away.
> > Do you have any specific example of a wrong answer from procfs at
> > hand, or at least a more specific direction to look at than just
> > "chroot/namespaces"?
> 
> Assuming you even have procfs, the name read from readlink on procfs
> will be relative to the real root of the mount namespace, not the
> chroot. If the mount namespace has bind mounts over top of anything,
> it can also give you a pathname that's no longer valid because
> something is mounted over it.
> 
> There may be other ways this arises too. At the very least, you need
> to do fstat/stat to match them up like we do now; otherwise you can
> get wildly wrong results. But even if the stat matches up, it's still
> possible that the resulting pathname is an absolute pathname outside
> the chroot or behind the bind mount or whatever, but is also valid but
> involving symlink traversal in when processed from in the current
> process context. This means you return a result that does not satisfy
> the contract to be symlink-free.
> 
> There's also a matter I didn't mention that the current code is wrong
> in an unsafe way on per-O_PATH kernels. Other places we mitigate that
> by using O_NOFOLLOW and O_NOCTTY to avoid *most* of the possible
> unwanted side effects if opening an actual file on a kernel that
> doesn't have O_PATH, but on realpath we specifically can't use
> O_NOFOLLOW, and this makes it susceptible to tricking root (or any
> user with read access) into opening device nodes, in ways that might
> have side effects.
> 
> So, there are a lot of bad things about the current implementation.
> Even the minor mitigations present now for some of them (the stat
> check) along with the overhead (open/close) makes it questionable
> whether it's faster for lots of inputs. For deep paths the new one is
> probably slower, but for typical ones it's not as clear and I didn't
> measure.
> 
> 
> > 
> > >#define _GNU_SOURCE
> > >#include <stdlib.h>
> > >#include <limits.h>
> > >#include <errno.h>
> > >#include <unistd.h>
> > >#include <string.h>
> > >
> > >static inline int at_dotdot(const char *end, size_t len)
> > >{
> > >   if (len<2) return 0;
> > >   if (len>2 && end[-3]!='/') return 0;
> > >   return end[-1]=='.' && end[-2]=='.';
> > >}
> > >
> > >char *realpath(const char *restrict filename, char *restrict resolved)
> > >{
> > >   char stack[PATH_MAX];
> > >   char buf[resolved ? 1 : PATH_MAX];
> > >   char *output = resolved ? resolved : buf;
> > >   size_t p, q, l, cnt=0;
> > >
> > >   l = strnlen(filename, sizeof stack + 1);
> > 
> > Why + 1?
> 
> I was thinking it was to ensure that the largest possible result is
> sufficient to detect ENAMETOOLONG condition, but even == PATH_MAX is
> sufficient for that since PATH_MAX is a limit including null
> termination. So I think the +1 can be removed.
> 
> > >   if (!l) {
> > >       errno = ENOENT;
> > >       return 0;
> > >   }
> > >   if (l >= sizeof stack) goto toolong;
> > >   p = sizeof stack - l - 1;
> > >   q = 0;
> > >   memcpy(stack+p, filename, l+1);
> > >
> > >   while (stack[p]) {
> > >       int up = 0;
> > >       if (stack[p] == '/') {
> > >           q=0;
> > >           output[q++] = '/';
> > >           p++;
> > >           /* Initial // is special. */
> > >           if (stack[p] == '/' && stack[p+1] != '/') {
> > >               output[q++] = '/';
> > >           }
> > >           while (stack[p] == '/') p++;
> > >           continue;
> > >       }
> > >       char *z = __strchrnul(stack+p, '/');
> > >       l = z-(stack+p);
> > >       if (l==1 && stack[p]=='.') {
> > >           p += l;
> > >           while (stack[p] == '/') p++;
> > >           continue;
> > >       }
> > >       if (at_dotdot(stack+p+l, l)) {
> > >           if (q && !at_dotdot(output+q, q)) {
> > >               while(q && output[q-1]!='/') q--;
> > >               if (q>1 && (q>2 || output[0]!='/')) q--;
> > >               p += l;
> > >               while (stack[p] == '/') p++;
> > >               continue;
> > >           }
> > >           up = 1;
> > >       }
> > >       if (q && output[q-1] != '/') {
> > >           if (!p) goto toolong;
> > >           stack[--p] = '/';
> > >           l++;
> > >       }
> > >       if (q+l >= PATH_MAX) goto toolong;
> > >       memcpy(output+q, stack+p, l);
> > >       output[q+l] = 0;
> > >       p += l;
> > >       if (up) goto notlink;
> > >       ssize_t k = readlink(output, stack, p);
> > >       if (k==-1) {
> > >           if (errno == EINVAL) {
> > >notlink:
> > >               q += l;
> > >               while (stack[p] == '/') p++;
> > >               continue;
> > >           }
> > >           return 0;
> > >       }
> > >       if (k==p) goto toolong;
> > >       if (++cnt == SYMLOOP_MAX) {
> > >           errno = ELOOP;
> > >           return 0;
> > >       }
> > >       p -= k;
> > >       memmove(stack+p, stack, k);
> > 
> > This isn't always correct if the symlink resolves to "/" because
> > stack[p] might be '/', so two slashes will be preserved in the
> > output. For example, "root/home" resolves to "//home" (where "root"
> > -> "/").
> 
> Thanks for catching that. I propose:
> 
> 	if (stack[k-1]=='/') p++;
> 
> And that raises the point that k==0 should be handled, even though
> Linux doesn't let you create such links, since in theory they could
> come from an existing fs or from non-Linux kernels (see
> https://lwn.net/Articles/551224/ for coverage of the topic). I propose
> erroring out with ENOENT in this case right after the k==p check above
> (since k==0 due to p==0 would bee toolong not ENOENT if it can
> happen).
> 
> > >   output[q] = 0;
> > >
> > >   if (output[0] != '/') {
> > >       if (!getcwd(stack, sizeof stack)) return 0;
> > >       l = strlen(stack);
> > >       /* Cancel any initial .. components. */
> > >       p = 0;
> > >       while (q-p>=2 && at_dotdot(output+p+2, p+2)) {
> > 
> > This doesn't check that output+p+2 is the end of a path element,
> > which is the prerequisite for at_dotdot(). So, for example, "..."
> > resolves incorrectly.
> 
> Thanks again. I don't see a really good way to reuse at_dotdot here,
> do you? Probably just [0]=='.' && [1]=='.' && (![2] || [2]=='/') is
> the cleanest.

And here are the proposed fixes as a patch. This should be good now.
Perhaps should add some comments.

Rich

[-- Attachment #2: realpath-fixes.diff --]
[-- Type: text/plain, Size: 887 bytes --]

--- realpath8.c	2020-11-22 17:52:17.586481571 -0500
+++ realpath9.c	2020-11-23 13:55:06.808458893 -0500
@@ -19,7 +19,7 @@
 	char *output = resolved ? resolved : buf;
 	size_t p, q, l, cnt=0;
 
-	l = strnlen(filename, sizeof stack + 1);
+	l = strnlen(filename, sizeof stack);
 	if (!l) {
 		errno = ENOENT;
 		return 0;
@@ -80,11 +80,16 @@
 			return 0;
 		}
 		if (k==p) goto toolong;
+		if (!k) {
+			errno = ENOENT;
+			return 0;
+		}
 		if (++cnt == SYMLOOP_MAX) {
 			errno = ELOOP;
 			return 0;
 		}
 		p -= k;
+		if (stack[k-1]=='/') p++;
 		memmove(stack+p, stack, k);
 	}
 
@@ -95,7 +100,8 @@
 		l = strlen(stack);
 		/* Cancel any initial .. components. */
 		p = 0;
-		while (q-p>=2 && at_dotdot(output+p+2, p+2)) {
+		while (output[p]=='.' && output[p+1]=='.'
+		  && (!output[p+2] || output[p+2]=='/')) {
 			while(l>1 && stack[l-1]!='/') l--;
 			if (l>1) l--;
 			p += 2;

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-23 18:56     ` Rich Felker
@ 2020-11-23 20:53       ` Rich Felker
  2020-11-24  3:39         ` Alexey Izbyshev
  0 siblings, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-11-23 20:53 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

On Mon, Nov 23, 2020 at 01:56:33PM -0500, Rich Felker wrote:
> On Sun, Nov 22, 2020 at 10:19:33PM -0500, Rich Felker wrote:
> > On Mon, Nov 23, 2020 at 05:03:25AM +0300, Alexey Izbyshev wrote:
> > > On 2020-11-23 01:56, Rich Felker wrote:
> > > >I originally considered keeping the procfs based version and only
> > > >using the new one as a fallback, but I discovered there are cases
> > > >(involving chroot, namespaces, etc.) where the answer from procfs is
> > > >wrong and validating it requires basically the same procedure as
> > > >implementing it manually (walking and performing readlink on each path
> > > >component).
> > > >
> > > Pity that the simple and fast procfs-based implementation goes away.
> > > Do you have any specific example of a wrong answer from procfs at
> > > hand, or at least a more specific direction to look at than just
> > > "chroot/namespaces"?
> > 
> > Assuming you even have procfs, the name read from readlink on procfs
> > will be relative to the real root of the mount namespace, not the
> > chroot. If the mount namespace has bind mounts over top of anything,
> > it can also give you a pathname that's no longer valid because
> > something is mounted over it.
> > 
> > There may be other ways this arises too. At the very least, you need
> > to do fstat/stat to match them up like we do now; otherwise you can
> > get wildly wrong results. But even if the stat matches up, it's still
> > possible that the resulting pathname is an absolute pathname outside
> > the chroot or behind the bind mount or whatever, but is also valid but
> > involving symlink traversal in when processed from in the current
> > process context. This means you return a result that does not satisfy
> > the contract to be symlink-free.
> > 
> > There's also a matter I didn't mention that the current code is wrong
> > in an unsafe way on per-O_PATH kernels. Other places we mitigate that
> > by using O_NOFOLLOW and O_NOCTTY to avoid *most* of the possible
> > unwanted side effects if opening an actual file on a kernel that
> > doesn't have O_PATH, but on realpath we specifically can't use
> > O_NOFOLLOW, and this makes it susceptible to tricking root (or any
> > user with read access) into opening device nodes, in ways that might
> > have side effects.
> > 
> > So, there are a lot of bad things about the current implementation.
> > Even the minor mitigations present now for some of them (the stat
> > check) along with the overhead (open/close) makes it questionable
> > whether it's faster for lots of inputs. For deep paths the new one is
> > probably slower, but for typical ones it's not as clear and I didn't
> > measure.
> > 
> > 
> > > 
> > > >#define _GNU_SOURCE
> > > >#include <stdlib.h>
> > > >#include <limits.h>
> > > >#include <errno.h>
> > > >#include <unistd.h>
> > > >#include <string.h>
> > > >
> > > >static inline int at_dotdot(const char *end, size_t len)
> > > >{
> > > >   if (len<2) return 0;
> > > >   if (len>2 && end[-3]!='/') return 0;
> > > >   return end[-1]=='.' && end[-2]=='.';
> > > >}
> > > >
> > > >char *realpath(const char *restrict filename, char *restrict resolved)
> > > >{
> > > >   char stack[PATH_MAX];
> > > >   char buf[resolved ? 1 : PATH_MAX];
> > > >   char *output = resolved ? resolved : buf;
> > > >   size_t p, q, l, cnt=0;
> > > >
> > > >   l = strnlen(filename, sizeof stack + 1);
> > > 
> > > Why + 1?
> > 
> > I was thinking it was to ensure that the largest possible result is
> > sufficient to detect ENAMETOOLONG condition, but even == PATH_MAX is
> > sufficient for that since PATH_MAX is a limit including null
> > termination. So I think the +1 can be removed.
> > 
> > > >   if (!l) {
> > > >       errno = ENOENT;
> > > >       return 0;
> > > >   }
> > > >   if (l >= sizeof stack) goto toolong;
> > > >   p = sizeof stack - l - 1;
> > > >   q = 0;
> > > >   memcpy(stack+p, filename, l+1);
> > > >
> > > >   while (stack[p]) {
> > > >       int up = 0;
> > > >       if (stack[p] == '/') {
> > > >           q=0;
> > > >           output[q++] = '/';
> > > >           p++;
> > > >           /* Initial // is special. */
> > > >           if (stack[p] == '/' && stack[p+1] != '/') {
> > > >               output[q++] = '/';
> > > >           }
> > > >           while (stack[p] == '/') p++;
> > > >           continue;
> > > >       }
> > > >       char *z = __strchrnul(stack+p, '/');
> > > >       l = z-(stack+p);
> > > >       if (l==1 && stack[p]=='.') {
> > > >           p += l;
> > > >           while (stack[p] == '/') p++;
> > > >           continue;
> > > >       }
> > > >       if (at_dotdot(stack+p+l, l)) {
> > > >           if (q && !at_dotdot(output+q, q)) {
> > > >               while(q && output[q-1]!='/') q--;
> > > >               if (q>1 && (q>2 || output[0]!='/')) q--;
> > > >               p += l;
> > > >               while (stack[p] == '/') p++;
> > > >               continue;
> > > >           }
> > > >           up = 1;
> > > >       }
> > > >       if (q && output[q-1] != '/') {
> > > >           if (!p) goto toolong;
> > > >           stack[--p] = '/';
> > > >           l++;
> > > >       }
> > > >       if (q+l >= PATH_MAX) goto toolong;
> > > >       memcpy(output+q, stack+p, l);
> > > >       output[q+l] = 0;
> > > >       p += l;
> > > >       if (up) goto notlink;
> > > >       ssize_t k = readlink(output, stack, p);
> > > >       if (k==-1) {
> > > >           if (errno == EINVAL) {
> > > >notlink:
> > > >               q += l;
> > > >               while (stack[p] == '/') p++;
> > > >               continue;
> > > >           }
> > > >           return 0;
> > > >       }
> > > >       if (k==p) goto toolong;
> > > >       if (++cnt == SYMLOOP_MAX) {
> > > >           errno = ELOOP;
> > > >           return 0;
> > > >       }
> > > >       p -= k;
> > > >       memmove(stack+p, stack, k);
> > > 
> > > This isn't always correct if the symlink resolves to "/" because
> > > stack[p] might be '/', so two slashes will be preserved in the
> > > output. For example, "root/home" resolves to "//home" (where "root"
> > > -> "/").
> > 
> > Thanks for catching that. I propose:
> > 
> > 	if (stack[k-1]=='/') p++;
> > 
> > And that raises the point that k==0 should be handled, even though
> > Linux doesn't let you create such links, since in theory they could
> > come from an existing fs or from non-Linux kernels (see
> > https://lwn.net/Articles/551224/ for coverage of the topic). I propose
> > erroring out with ENOENT in this case right after the k==p check above
> > (since k==0 due to p==0 would bee toolong not ENOENT if it can
> > happen).
> > 
> > > >   output[q] = 0;
> > > >
> > > >   if (output[0] != '/') {
> > > >       if (!getcwd(stack, sizeof stack)) return 0;
> > > >       l = strlen(stack);
> > > >       /* Cancel any initial .. components. */
> > > >       p = 0;
> > > >       while (q-p>=2 && at_dotdot(output+p+2, p+2)) {
> > > 
> > > This doesn't check that output+p+2 is the end of a path element,
> > > which is the prerequisite for at_dotdot(). So, for example, "..."
> > > resolves incorrectly.
> > 
> > Thanks again. I don't see a really good way to reuse at_dotdot here,
> > do you? Probably just [0]=='.' && [1]=='.' && (![2] || [2]=='/') is
> > the cleanest.
> 
> And here are the proposed fixes as a patch. This should be good now.
> Perhaps should add some comments.
> 
> Rich

> --- realpath8.c	2020-11-22 17:52:17.586481571 -0500
> +++ realpath9.c	2020-11-23 13:55:06.808458893 -0500
> @@ -19,7 +19,7 @@
>  	char *output = resolved ? resolved : buf;
>  	size_t p, q, l, cnt=0;
>  
> -	l = strnlen(filename, sizeof stack + 1);
> +	l = strnlen(filename, sizeof stack);
>  	if (!l) {
>  		errno = ENOENT;
>  		return 0;
> @@ -80,11 +80,16 @@
>  			return 0;
>  		}
>  		if (k==p) goto toolong;
> +		if (!k) {
> +			errno = ENOENT;
> +			return 0;
> +		}
>  		if (++cnt == SYMLOOP_MAX) {
>  			errno = ELOOP;
>  			return 0;
>  		}
>  		p -= k;
> +		if (stack[k-1]=='/') p++;
>  		memmove(stack+p, stack, k);

This is wrong and needs further consideration.

>  	}
>  
> @@ -95,7 +100,8 @@
>  		l = strlen(stack);
>  		/* Cancel any initial .. components. */
>  		p = 0;
> -		while (q-p>=2 && at_dotdot(output+p+2, p+2)) {
> +		while (output[p]=='.' && output[p+1]=='.'
> +		  && (!output[p+2] || output[p+2]=='/')) {
>  			while(l>1 && stack[l-1]!='/') l--;
>  			if (l>1) l--;
>  			p += 2;

OK, I have a better improvement for this: counting the number of
levels of .. as they're built at the head of output. Then it's just
while (nup--) here, and the condition for canceling .. in the first
loop no longer needs any string inspection; it's just (q>3*nup).

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-23 20:53       ` Rich Felker
@ 2020-11-24  3:39         ` Alexey Izbyshev
  2020-11-24  4:26           ` Rich Felker
  0 siblings, 1 reply; 20+ messages in thread
From: Alexey Izbyshev @ 2020-11-24  3:39 UTC (permalink / raw)
  To: musl

On 2020-11-23 23:53, Rich Felker wrote:
> On Mon, Nov 23, 2020 at 01:56:33PM -0500, Rich Felker wrote:
>> On Sun, Nov 22, 2020 at 10:19:33PM -0500, Rich Felker wrote:
>> --- realpath8.c	2020-11-22 17:52:17.586481571 -0500
>> +++ realpath9.c	2020-11-23 13:55:06.808458893 -0500
>> @@ -19,7 +19,7 @@
>>  	char *output = resolved ? resolved : buf;
>>  	size_t p, q, l, cnt=0;
>> 
>> -	l = strnlen(filename, sizeof stack + 1);
>> +	l = strnlen(filename, sizeof stack);
>>  	if (!l) {
>>  		errno = ENOENT;
>>  		return 0;
>> @@ -80,11 +80,16 @@
>>  			return 0;
>>  		}
>>  		if (k==p) goto toolong;
>> +		if (!k) {
>> +			errno = ENOENT;
>> +			return 0;
>> +		}
>>  		if (++cnt == SYMLOOP_MAX) {
>>  			errno = ELOOP;
>>  			return 0;
>>  		}
>>  		p -= k;
>> +		if (stack[k-1]=='/') p++;
>>  		memmove(stack+p, stack, k);
> 
> This is wrong and needs further consideration.
> 
Yes, now memmove() overwrites NUL if p was at the end and stack[k-1] == 
'/'. Is it true per POSIX that "rr/home" must resolve to "//home" if 
"rr" -> "//"? If so, maybe something like the following instead:

+               while (stack[p] == '/') p++;
+               if (stack[p] && stack[k-1] != '/') p--;
                 p -= k;
-               if (stack[k-1]=='/') p++;

>>  	}
>> 
>> @@ -95,7 +100,8 @@
>>  		l = strlen(stack);
>>  		/* Cancel any initial .. components. */
>>  		p = 0;
>> -		while (q-p>=2 && at_dotdot(output+p+2, p+2)) {
>> +		while (output[p]=='.' && output[p+1]=='.'
>> +		  && (!output[p+2] || output[p+2]=='/')) {
>>  			while(l>1 && stack[l-1]!='/') l--;
>>  			if (l>1) l--;
>>  			p += 2;
> 
> OK, I have a better improvement for this: counting the number of
> levels of .. as they're built at the head of output. Then it's just
> while (nup--) here, and the condition for canceling .. in the first
> loop no longer needs any string inspection; it's just (q>3*nup).
> 
Sounds good.

I've missed the last time that the immediately following code is also 
broken:

>               if (q-p && stack[l-1]!='/') output[--p] = '/';

It will underflow the output in case of a simple relative path that 
doesn't start with "..".

I've also noticed other issues to be fixed, per POSIX:

* ENOENT should be returned if filename is NULL

* ENOTDIR should be returned if the last component is not a directory  
and the path has one or more trailing slashes

Alexey

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-23  3:19   ` Rich Felker
  2020-11-23 18:56     ` Rich Felker
@ 2020-11-24  3:41     ` Alexey Izbyshev
  1 sibling, 0 replies; 20+ messages in thread
From: Alexey Izbyshev @ 2020-11-24  3:41 UTC (permalink / raw)
  To: musl

On 2020-11-23 06:19, Rich Felker wrote:
> On Mon, Nov 23, 2020 at 05:03:25AM +0300, Alexey Izbyshev wrote:
>> On 2020-11-23 01:56, Rich Felker wrote:
>> >I originally considered keeping the procfs based version and only
>> >using the new one as a fallback, but I discovered there are cases
>> >(involving chroot, namespaces, etc.) where the answer from procfs is
>> >wrong and validating it requires basically the same procedure as
>> >implementing it manually (walking and performing readlink on each path
>> >component).
>> >
>> Pity that the simple and fast procfs-based implementation goes away.
>> Do you have any specific example of a wrong answer from procfs at
>> hand, or at least a more specific direction to look at than just
>> "chroot/namespaces"?
> 
> Assuming you even have procfs, the name read from readlink on procfs
> will be relative to the real root of the mount namespace, not the
> chroot.

Probably I misunderstand what you mean, but when I tried a 
straightforward test on a 5.4-based kernel (with procfs-based musl, of 
course), I failed:

$ mkdir -p chr/proc
$ cat test.c
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[]) {
   puts(realpath(argv[1], NULL));
}
$ musl-gcc -static -std=gnu99 test.c -o chr/a.out
$ unshare -Urm
# mount --rbind /proc chr/proc
# chroot chr ./a.out ./a.out
/a.out

> If the mount namespace has bind mounts over top of anything,
> it can also give you a pathname that's no longer valid because
> something is mounted over it.
> 
I see how this can be the case if a bind mount was created on top of 
something *after* musl opened the fd in realpath(), but in presence of 
concurrent FS modifications the new implementation can return a stale 
result too.

> There may be other ways this arises too. At the very least, you need
> to do fstat/stat to match them up like we do now; otherwise you can
> get wildly wrong results. But even if the stat matches up, it's still
> possible that the resulting pathname is an absolute pathname outside
> the chroot or behind the bind mount or whatever, but is also valid but
> involving symlink traversal in when processed from in the current
> process context. This means you return a result that does not satisfy
> the contract to be symlink-free.
> 
If a path that is not meaningful in the context of the current process 
can be returned by the kernel in absence of concurrent FS modifications, 
it is disturbing. The kernel code for open() does more-or-less the same 
as the new realpath() implementation, and it's not clear to me what goes 
wrong when the kernel walks the parent chain back to rebuild the path 
(and AFAIR, the kernel does remember the chain of mount points that it 
traversed while resolving a path, and it certainly does account for 
chroot).

> There's also a matter I didn't mention that the current code is wrong
> in an unsafe way on per-O_PATH kernels. Other places we mitigate that
> by using O_NOFOLLOW and O_NOCTTY to avoid *most* of the possible
> unwanted side effects if opening an actual file on a kernel that
> doesn't have O_PATH, but on realpath we specifically can't use
> O_NOFOLLOW, and this makes it susceptible to tricking root (or any
> user with read access) into opening device nodes, in ways that might
> have side effects.
> 
Ugh, yes, pre-O_PATH kernels are a problem. Apart from what you 
described, you can't open a non-readable file in a readable directory 
without O_PATH, and you'll have trouble with Unix domain sockets as 
well. But at least this class of problems will go away with time.

> So, there are a lot of bad things about the current implementation.
> Even the minor mitigations present now for some of them (the stat
> check) along with the overhead (open/close) makes it questionable
> whether it's faster for lots of inputs. For deep paths the new one is
> probably slower, but for typical ones it's not as clear and I didn't
> measure.
> 
Thanks for the detailed response. While I was aware about problems with 
procfs-based path resolution for long-lived fds (i.e. when FS tree 
changes after the file was opened), I'm a bit baffled that, according to 
your description, things can go wrong even in realpath() use case.

Alexey

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-24  3:39         ` Alexey Izbyshev
@ 2020-11-24  4:26           ` Rich Felker
  2020-11-24  5:13             ` Alexey Izbyshev
  2020-11-24 20:31             ` Rich Felker
  0 siblings, 2 replies; 20+ messages in thread
From: Rich Felker @ 2020-11-24  4:26 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

On Tue, Nov 24, 2020 at 06:39:59AM +0300, Alexey Izbyshev wrote:
> On 2020-11-23 23:53, Rich Felker wrote:
> >On Mon, Nov 23, 2020 at 01:56:33PM -0500, Rich Felker wrote:
> >>On Sun, Nov 22, 2020 at 10:19:33PM -0500, Rich Felker wrote:
> >>--- realpath8.c	2020-11-22 17:52:17.586481571 -0500
> >>+++ realpath9.c	2020-11-23 13:55:06.808458893 -0500
> >>@@ -19,7 +19,7 @@
> >> 	char *output = resolved ? resolved : buf;
> >> 	size_t p, q, l, cnt=0;
> >>
> >>-	l = strnlen(filename, sizeof stack + 1);
> >>+	l = strnlen(filename, sizeof stack);
> >> 	if (!l) {
> >> 		errno = ENOENT;
> >> 		return 0;
> >>@@ -80,11 +80,16 @@
> >> 			return 0;
> >> 		}
> >> 		if (k==p) goto toolong;
> >>+		if (!k) {
> >>+			errno = ENOENT;
> >>+			return 0;
> >>+		}
> >> 		if (++cnt == SYMLOOP_MAX) {
> >> 			errno = ELOOP;
> >> 			return 0;
> >> 		}
> >> 		p -= k;
> >>+		if (stack[k-1]=='/') p++;
> >> 		memmove(stack+p, stack, k);
> >
> >This is wrong and needs further consideration.
> >
> Yes, now memmove() overwrites NUL if p was at the end and stack[k-1]
> == '/'. Is it true per POSIX that "rr/home" must resolve to "//home"
> if "rr" -> "//"?

I don't think // is even required be distinct from /, just permitted,
but I think allowing it in userspace and handling it consistently is
the right behavior in case you ever run on a kernel that does make use
of the distinction.

> If so, maybe something like the following instead:
> 
> +               while (stack[p] == '/') p++;
> +               if (stack[p] && stack[k-1] != '/') p--;
>                 p -= k;
> -               if (stack[k-1]=='/') p++;

Rather just:

	/* If link contents end in /, strip any slashes already on
	 * stack to avoid /->// or //->/// or spurious toolong. */
	if (stack[k-1]=='/') while (stack[p]=='/') p++;

should work (before the p-=k;)

> >> 	}
> >>
> >>@@ -95,7 +100,8 @@
> >> 		l = strlen(stack);
> >> 		/* Cancel any initial .. components. */
> >> 		p = 0;
> >>-		while (q-p>=2 && at_dotdot(output+p+2, p+2)) {
> >>+		while (output[p]=='.' && output[p+1]=='.'
> >>+		  && (!output[p+2] || output[p+2]=='/')) {
> >> 			while(l>1 && stack[l-1]!='/') l--;
> >> 			if (l>1) l--;
> >> 			p += 2;
> >
> >OK, I have a better improvement for this: counting the number of
> >levels of .. as they're built at the head of output. Then it's just
> >while (nup--) here, and the condition for canceling .. in the first
> >loop no longer needs any string inspection; it's just (q>3*nup).
> >
> Sounds good.
> 
> I've missed the last time that the immediately following code is
> also broken:
> 
> >              if (q-p && stack[l-1]!='/') output[--p] = '/';
> 
> It will underflow the output in case of a simple relative path that
> doesn't start with "..".

Thanks. This logic just looks wrong; I'll rework it.

> I've also noticed other issues to be fixed, per POSIX:
> 
> * ENOENT should be returned if filename is NULL

Rather it looks like it's:

	[EINVAL] The file_name argument is a null pointer.

ENOENT is only for empty string or ENOENT somewhere in the path
traversal process.

> * ENOTDIR should be returned if the last component is not a
> directory  and the path has one or more trailing slashes

Yes, that's precisely what I've been working on the past couple hours.
I think you missed but .. will also erase a path component that's not
a dir (e.g. /dev/null/.. -> /dev) and these are both instances of a
common problem. I thought use of readlink covered all the ENOTDIR
cases but it doesn't when the next component isn't covered by readlink
or isn't present at all.

It's trivial to fix with a check after each component but that doubles
the number of syscalls and mostly isn't necessary. I have a reworked
draft to fix the problem by advancing over /(/|./|.$)* rather than just
/+ after each component, so that we can lookahead and do an extra
readlink in the cases that need it.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-24  4:26           ` Rich Felker
@ 2020-11-24  5:13             ` Alexey Izbyshev
  2020-11-24  6:30               ` Rich Felker
  2020-11-24 20:31             ` Rich Felker
  1 sibling, 1 reply; 20+ messages in thread
From: Alexey Izbyshev @ 2020-11-24  5:13 UTC (permalink / raw)
  To: musl

On 2020-11-24 07:26, Rich Felker wrote:
> On Tue, Nov 24, 2020 at 06:39:59AM +0300, Alexey Izbyshev wrote:
>> On 2020-11-23 23:53, Rich Felker wrote:
>> >On Mon, Nov 23, 2020 at 01:56:33PM -0500, Rich Felker wrote:
>> >>On Sun, Nov 22, 2020 at 10:19:33PM -0500, Rich Felker wrote:
>> >>--- realpath8.c	2020-11-22 17:52:17.586481571 -0500
>> >>+++ realpath9.c	2020-11-23 13:55:06.808458893 -0500
>> >>@@ -19,7 +19,7 @@
>> >> 	char *output = resolved ? resolved : buf;
>> >> 	size_t p, q, l, cnt=0;
>> >>
>> >>-	l = strnlen(filename, sizeof stack + 1);
>> >>+	l = strnlen(filename, sizeof stack);
>> >> 	if (!l) {
>> >> 		errno = ENOENT;
>> >> 		return 0;
>> >>@@ -80,11 +80,16 @@
>> >> 			return 0;
>> >> 		}
>> >> 		if (k==p) goto toolong;
>> >>+		if (!k) {
>> >>+			errno = ENOENT;
>> >>+			return 0;
>> >>+		}
>> >> 		if (++cnt == SYMLOOP_MAX) {
>> >> 			errno = ELOOP;
>> >> 			return 0;
>> >> 		}
>> >> 		p -= k;
>> >>+		if (stack[k-1]=='/') p++;
>> >> 		memmove(stack+p, stack, k);
>> >
>> >This is wrong and needs further consideration.
>> >
>> Yes, now memmove() overwrites NUL if p was at the end and stack[k-1]
>> == '/'. Is it true per POSIX that "rr/home" must resolve to "//home"
>> if "rr" -> "//"?
> 
> I don't think // is even required be distinct from /, just permitted,
> but I think allowing it in userspace and handling it consistently is
> the right behavior in case you ever run on a kernel that does make use
> of the distinction.
> 
>> If so, maybe something like the following instead:
>> 
>> +               while (stack[p] == '/') p++;
>> +               if (stack[p] && stack[k-1] != '/') p--;
>>                 p -= k;
>> -               if (stack[k-1]=='/') p++;
> 
> Rather just:
> 
> 	/* If link contents end in /, strip any slashes already on
> 	 * stack to avoid /->// or //->/// or spurious toolong. */
> 	if (stack[k-1]=='/') while (stack[p]=='/') p++;
> 
> should work (before the p-=k;)
> 
Yes, that looks good.

>> I've also noticed other issues to be fixed, per POSIX:
>> 
>> * ENOENT should be returned if filename is NULL
> 
> Rather it looks like it's:
> 
> 	[EINVAL] The file_name argument is a null pointer.
> 
> ENOENT is only for empty string or ENOENT somewhere in the path
> traversal process.
> 
Uh, yes, that was bad copy-paste or something.

>> * ENOTDIR should be returned if the last component is not a
>> directory  and the path has one or more trailing slashes
> 
> Yes, that's precisely what I've been working on the past couple hours.
> I think you missed but .. will also erase a path component that's not
> a dir (e.g. /dev/null/.. -> /dev) and these are both instances of a
> common problem. I thought use of readlink covered all the ENOTDIR
> cases but it doesn't when the next component isn't covered by readlink
> or isn't present at all.
> 
Yes, initially I forgot about this whole ENOTDIR issue completely, and 
after noticing the problem with the last component, didn't look further.

Alexey

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-24  5:13             ` Alexey Izbyshev
@ 2020-11-24  6:30               ` Rich Felker
  2020-11-24  9:21                 ` Alexey Izbyshev
  0 siblings, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-11-24  6:30 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

On Tue, Nov 24, 2020 at 08:13:56AM +0300, Alexey Izbyshev wrote:
> On 2020-11-24 07:26, Rich Felker wrote:
> >On Tue, Nov 24, 2020 at 06:39:59AM +0300, Alexey Izbyshev wrote:
> >>On 2020-11-23 23:53, Rich Felker wrote:
> >>>On Mon, Nov 23, 2020 at 01:56:33PM -0500, Rich Felker wrote:
> >>>>On Sun, Nov 22, 2020 at 10:19:33PM -0500, Rich Felker wrote:
> >>>>--- realpath8.c	2020-11-22 17:52:17.586481571 -0500
> >>>>+++ realpath9.c	2020-11-23 13:55:06.808458893 -0500
> >>>>@@ -19,7 +19,7 @@
> >>>> 	char *output = resolved ? resolved : buf;
> >>>> 	size_t p, q, l, cnt=0;
> >>>>
> >>>>-	l = strnlen(filename, sizeof stack + 1);
> >>>>+	l = strnlen(filename, sizeof stack);
> >>>> 	if (!l) {
> >>>> 		errno = ENOENT;
> >>>> 		return 0;
> >>>>@@ -80,11 +80,16 @@
> >>>> 			return 0;
> >>>> 		}
> >>>> 		if (k==p) goto toolong;
> >>>>+		if (!k) {
> >>>>+			errno = ENOENT;
> >>>>+			return 0;
> >>>>+		}
> >>>> 		if (++cnt == SYMLOOP_MAX) {
> >>>> 			errno = ELOOP;
> >>>> 			return 0;
> >>>> 		}
> >>>> 		p -= k;
> >>>>+		if (stack[k-1]=='/') p++;
> >>>> 		memmove(stack+p, stack, k);
> >>>
> >>>This is wrong and needs further consideration.
> >>>
> >>Yes, now memmove() overwrites NUL if p was at the end and stack[k-1]
> >>== '/'. Is it true per POSIX that "rr/home" must resolve to "//home"
> >>if "rr" -> "//"?
> >
> >I don't think // is even required be distinct from /, just permitted,
> >but I think allowing it in userspace and handling it consistently is
> >the right behavior in case you ever run on a kernel that does make use
> >of the distinction.
> >
> >>If so, maybe something like the following instead:
> >>
> >>+               while (stack[p] == '/') p++;
> >>+               if (stack[p] && stack[k-1] != '/') p--;
> >>                p -= k;
> >>-               if (stack[k-1]=='/') p++;
> >
> >Rather just:
> >
> >	/* If link contents end in /, strip any slashes already on
> >	 * stack to avoid /->// or //->/// or spurious toolong. */
> >	if (stack[k-1]=='/') while (stack[p]=='/') p++;
> >
> >should work (before the p-=k;)
> >
> Yes, that looks good.
> 
> >>I've also noticed other issues to be fixed, per POSIX:
> >>
> >>* ENOENT should be returned if filename is NULL
> >
> >Rather it looks like it's:
> >
> >	[EINVAL] The file_name argument is a null pointer.
> >
> >ENOENT is only for empty string or ENOENT somewhere in the path
> >traversal process.
> >
> Uh, yes, that was bad copy-paste or something.
> 
> >>* ENOTDIR should be returned if the last component is not a
> >>directory  and the path has one or more trailing slashes
> >
> >Yes, that's precisely what I've been working on the past couple hours.
> >I think you missed but .. will also erase a path component that's not
> >a dir (e.g. /dev/null/.. -> /dev) and these are both instances of a
> >common problem. I thought use of readlink covered all the ENOTDIR
> >cases but it doesn't when the next component isn't covered by readlink
> >or isn't present at all.
> >
> Yes, initially I forgot about this whole ENOTDIR issue completely,
> and after noticing the problem with the last component, didn't look
> further.

I think before this goes upstream we should have a good set of
testcases that can be contributed to libc-test. Do you have ideas for
coverage? Some that come to mind:

- Absolute argument starting with /, //, and ///
- Absolute symlink target starting with /, //, and ///
- Final / after symlink-to-dir, dir, symlink-to-nondir, nondir
- Final / in link contents after [the above]
- Initial .. in argument, cwd root or non-root
- Initial .. in symlink target, symlink in root or non-root
- Initial ...
- .. following symlink-to-dir, dir, symlink-to-nondir, nondir
- More .. than path depth
- Null argument
- Empty string argument
- Empty string link contents (testable only with seccomp hack)
- Argument valid abs path exact length PATH_MAX-1
- Argument valid rel path exact length PATH_MAX-1 to short abs path

Some of these require namespace gymnastics to set up without running
the tests as root.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-24  6:30               ` Rich Felker
@ 2020-11-24  9:21                 ` Alexey Izbyshev
  2020-11-24 14:35                   ` Rich Felker
  2020-11-25 15:02                   ` Rich Felker
  0 siblings, 2 replies; 20+ messages in thread
From: Alexey Izbyshev @ 2020-11-24  9:21 UTC (permalink / raw)
  To: musl

On 2020-11-24 09:30, Rich Felker wrote:
> I think before this goes upstream we should have a good set of
> testcases that can be contributed to libc-test. Do you have ideas for
> coverage? Some that come to mind:
> 
Added some more ideas.

> - Absolute argument starting with /, //, and ///
- Absolute argument equal to one of /, //, and ///
> - Absolute symlink target starting with /, //, and ///
- Absolute symlink target equal to one of /, //, and ///, with the link 
separated from the following component with /, //
> - Final / after symlink-to-dir, dir, symlink-to-nondir, nondir
- Intermediate / after symlink-to-nondir, nondir
> - Final / in link contents after [the above]
- Multiple / after ., .., normal component
> - Initial .. in argument, cwd root or non-root
> - Initial .. in symlink target, symlink in root or non-root
> - Initial ...
> - .. following symlink-to-dir, dir, symlink-to-nondir, nondir
- . following symlink-to-dir, dir, symlink-to-nondir, nondir
> - More .. than path depth
> - Null argument
> - Empty string argument
- Argument consisting of a single ., ..
> - Empty string link contents (testable only with seccomp hack)
> - Argument valid abs path exact length PATH_MAX-1
> - Argument valid rel path exact length PATH_MAX-1 to short abs path
- Argument with PATH_MAX length (ENAMETOOLONG)
- A relative symlink in the argument such that the length of the result 
is PATH_MAX-1 (valid path), PATH_MAX (ENAMETOOLONG)
- An absolute symlink in the argument similar to the above
- A relative argument with the current directory similar to the above
- An argument consisting of a single (relative, absolute) symlink with 
the target having length PATH_MAX-1
- An argument ending with a relative symlink with the target having 
length PATH_MAX-1 (ENAMETOOLONG)
- An argument ending with an absolute symlink with the target having 
length PATH_MAX-1 (valid path)

Hm, the last one doesn't work now. Since p is the position of NUL 
instead of the size of stack, "if (k==p) goto toolong;" forbids symlinks 
with the length of the target == PATH_MAX-1.
> 
> Some of these require namespace gymnastics to set up without running
> the tests as root.
> 
Alexey

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-24  9:21                 ` Alexey Izbyshev
@ 2020-11-24 14:35                   ` Rich Felker
  2020-11-24 20:17                     ` Rich Felker
  2020-11-25 15:02                   ` Rich Felker
  1 sibling, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-11-24 14:35 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

On Tue, Nov 24, 2020 at 12:21:36PM +0300, Alexey Izbyshev wrote:
> On 2020-11-24 09:30, Rich Felker wrote:
> >I think before this goes upstream we should have a good set of
> >testcases that can be contributed to libc-test. Do you have ideas for
> >coverage? Some that come to mind:
> >
> Added some more ideas.
> 
> [...]
> - An argument ending with an absolute symlink with the target having
> length PATH_MAX-1 (valid path)
> 
> Hm, the last one doesn't work now. Since p is the position of NUL
> instead of the size of stack, "if (k==p) goto toolong;" forbids
> symlinks with the length of the target == PATH_MAX-1.

This should be fixable just by increasing size of stack to PATH_MAX+1.
In theory it doesn't need to be null-terminated but then strchrnul
won't work. memchr would work instead but it's slightly less
convenient to use.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-24 14:35                   ` Rich Felker
@ 2020-11-24 20:17                     ` Rich Felker
  0 siblings, 0 replies; 20+ messages in thread
From: Rich Felker @ 2020-11-24 20:17 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

On Tue, Nov 24, 2020 at 09:35:55AM -0500, Rich Felker wrote:
> On Tue, Nov 24, 2020 at 12:21:36PM +0300, Alexey Izbyshev wrote:
> > On 2020-11-24 09:30, Rich Felker wrote:
> > >I think before this goes upstream we should have a good set of
> > >testcases that can be contributed to libc-test. Do you have ideas for
> > >coverage? Some that come to mind:
> > >
> > Added some more ideas.
> > 
> > [...]
> > - An argument ending with an absolute symlink with the target having
> > length PATH_MAX-1 (valid path)
> > 
> > Hm, the last one doesn't work now. Since p is the position of NUL
> > instead of the size of stack, "if (k==p) goto toolong;" forbids
> > symlinks with the length of the target == PATH_MAX-1.
> 
> This should be fixable just by increasing size of stack to PATH_MAX+1.
> In theory it doesn't need to be null-terminated but then strchrnul
> won't work. memchr would work instead but it's slightly less
> convenient to use.

One thing I noticed while working out test ideas: my very early
observation that we can use the caller's buffer for output is wrong.
The spec allows it to have been clobbered on failure but not on
success; there's nothing allowing write past the resulting string
size.

It's not too bad to have two 4k buffers, but I think we can actually
put them together. PATH_MAX+1 isn't quite enough because, when
expanding a link, we momentatily need space for both the link name and
contents. However PATH_MAX+NAME_MAX+2 should suffice, since at most
a NAME_MAX part is being removed before pushing the link contents onto
the stack. I don't really feel like making this improvement now
though; it's better done as a change later if desired.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-24  4:26           ` Rich Felker
  2020-11-24  5:13             ` Alexey Izbyshev
@ 2020-11-24 20:31             ` Rich Felker
  2020-11-25  5:40               ` Alexey Izbyshev
  1 sibling, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-11-24 20:31 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

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

On Mon, Nov 23, 2020 at 11:26:46PM -0500, Rich Felker wrote:
> On Tue, Nov 24, 2020 at 06:39:59AM +0300, Alexey Izbyshev wrote:
> > * ENOTDIR should be returned if the last component is not a
> > directory  and the path has one or more trailing slashes
> 
> Yes, that's precisely what I've been working on the past couple hours.
> I think you missed but .. will also erase a path component that's not
> a dir (e.g. /dev/null/.. -> /dev) and these are both instances of a
> common problem. I thought use of readlink covered all the ENOTDIR
> cases but it doesn't when the next component isn't covered by readlink
> or isn't present at all.
> 
> It's trivial to fix with a check after each component but that doubles
> the number of syscalls and mostly isn't necessary. I have a reworked
> draft to fix the problem by advancing over /(/|./|.$)* rather than just
> /+ after each component, so that we can lookahead and do an extra
> readlink in the cases that need it.

While this worked, it ended up being the wrong thing to do, making two
places where readlink is called, one of them with a dummy buffer. The
right way to do it is rework the flow so that the existing readlink is
"naturally" hit where needed. This amounts to:

- Letting .. processing that cancels path components go through the
  same code path as new path components, rather than handling it
  early, and just skipping the actual readlink if we already know we
  have a dir.

- Also treating a zero-length final component as something that goes
  through the readlink code path.

There was a fair amount of reorganizing needed to make this work out,
but the end result is clean and non-redundant and code size is almost
the same as before with the missing-ENOTDIR bugs.

Speaking of code size, on 32-bit archs the proposed explicit realpath
is roughly the same size as stat+fstat+fstatat (a little over 1k on
i386), which were needed to implement the old lazy realpath in terms
of procfs. So for minimal static linking, resulting code size may be
same or smaller. (Of course it's larger if stat is already linked for
other reasons.)

New draft attached. It's possible that there are regressions since I
haven't put together an automated testset. I'm not sure if I'll try to
merge it in this release cycle still or not; that probably depends on
how easy or difficult automating these tests ends up being.

Rich

[-- Attachment #2: realpath17.c --]
[-- Type: text/plain, Size: 3614 bytes --]

#define _GNU_SOURCE
#include <stdlib.h>
#include <limits.h>
#include <errno.h>
#include <unistd.h>
#include <string.h>

static size_t slash_len(const char *s)
{
	const char *s0 = s;
	while (*s == '/') s++;
	return s-s0;
}

char *realpath(const char *restrict filename, char *restrict resolved)
{
	char stack[PATH_MAX+1];
	char output[PATH_MAX];
	size_t p, q, l, l0, cnt=0, nup=0;
	int check_dir=0;

	if (!filename) {
		errno = EINVAL;
		return 0;
	}
	l = strnlen(filename, sizeof stack);
	if (!l) {
		errno = ENOENT;
		return 0;
	}
	if (l >= PATH_MAX) goto toolong;
	p = sizeof stack - l - 1;
	q = 0;
	memcpy(stack+p, filename, l+1);

	/* Main loop. Each iteration pops the next part from stack of
	 * remaining path components and consumes any slashes that follow.
	 * If not a link, it's moved to output; if a link, contents are
	 * pushed to the stack. */
restart:
	for (; ; p+=slash_len(stack+p)) {
		/* If stack starts with /, the whole component is / or //
		 * and the output state must be reset. */
		if (stack[p] == '/') {
			check_dir=0;
			nup=0;
			q=0;
			output[q++] = '/';
			p++;
			/* Initial // is special. */
			if (stack[p] == '/' && stack[p+1] != '/')
				output[q++] = '/';
			continue;
		}

		char *z = __strchrnul(stack+p, '/');
		l0 = l = z-(stack+p);

		if (!l && !check_dir) break;

		/* Skip any . component but preserve check_dir status. */
		if (l==1 && stack[p]=='.') {
			p += l;
			continue;
		}

		/* Copy next component onto output at least temporarily, to
		 * call readlink, but wait to advance output position until
		 * determining it's not a link. */
		if (q && output[q-1] != '/') {
			if (!p) goto toolong;
			stack[--p] = '/';
			l++;
		}
		if (q+l >= PATH_MAX) goto toolong;
		memcpy(output+q, stack+p, l);
		output[q+l] = 0;
		p += l;

		int up = 0;
		if (l0==2 && stack[p-2]=='.' && stack[p-1]=='.') {
			up = 1;
			/* Any non-.. path components we could cancel start
			 * after nup repetitions of the 3-byte string "../";
			 * if there are none, accumulate .. components to
			 * later apply to cwd, if needed. */
			if (q <= 3*nup) {
				nup++;
				q += l;
				continue;
			}
			/* When previous components are already known to be
			 * directories, processing .. can skip readlink. */
			if (!check_dir) goto skip_readlink;
		}
		ssize_t k = readlink(output, stack, p);
		if (k==p) goto toolong;
		if (!k) {
			errno = ENOENT;
			return 0;
		}
		if (k<0) {
			if (errno != EINVAL) return 0;
skip_readlink:
			check_dir = 0;
			if (up) {
				while(q && output[q-1]!='/') q--;
				if (q>1 && (q>2 || output[0]!='/')) q--;
				continue;
			}
			if (l0) q += l;
			check_dir = stack[p];
			continue;
		}
		if (++cnt == SYMLOOP_MAX) {
			errno = ELOOP;
			return 0;
		}

		/* If link contents end in /, strip any slashes already on
		 * stack to avoid /->// or //->/// or spurious toolong. */
		if (stack[k-1]=='/') while (stack[p]=='/') p++;
		p -= k;
		memmove(stack+p, stack, k);

		/* Skip the stack advancement in case we have a new
		 * absolute base path. */
		goto restart;
	}

 	output[q] = 0;

	if (output[0] != '/') {
		if (!getcwd(stack, sizeof stack)) return 0;
		l = strlen(stack);
		/* Cancel any initial .. components. */
		p = 0;
		while (nup--) {
			while(l>1 && stack[l-1]!='/') l--;
			if (l>1) l--;
			p += 2;
			if (p<q) p++;
		}
		if (q-p && stack[l-1]!='/') stack[l++] = '/';
		if (l + (q-p) + 1 >= PATH_MAX) goto toolong;
		memmove(output + l, output + p, q - p + 1);
		memcpy(output, stack, l);
		q = l + q-p;
	}

	if (resolved) return memcpy(resolved, output, q+1);
	else return strdup(output);

toolong:
	errno = ENAMETOOLONG;
	return 0;
}

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-24 20:31             ` Rich Felker
@ 2020-11-25  5:40               ` Alexey Izbyshev
  2020-11-25 15:03                 ` Rich Felker
  0 siblings, 1 reply; 20+ messages in thread
From: Alexey Izbyshev @ 2020-11-25  5:40 UTC (permalink / raw)
  To: musl

On 2020-11-24 23:31, Rich Felker wrote:
> On Mon, Nov 23, 2020 at 11:26:46PM -0500, Rich Felker wrote:
>> On Tue, Nov 24, 2020 at 06:39:59AM +0300, Alexey Izbyshev wrote:
>> > * ENOTDIR should be returned if the last component is not a
>> > directory  and the path has one or more trailing slashes
>> 
>> Yes, that's precisely what I've been working on the past couple hours.
>> I think you missed but .. will also erase a path component that's not
>> a dir (e.g. /dev/null/.. -> /dev) and these are both instances of a
>> common problem. I thought use of readlink covered all the ENOTDIR
>> cases but it doesn't when the next component isn't covered by readlink
>> or isn't present at all.
>> 
>> It's trivial to fix with a check after each component but that doubles
>> the number of syscalls and mostly isn't necessary. I have a reworked
>> draft to fix the problem by advancing over /(/|./|.$)* rather than 
>> just
>> /+ after each component, so that we can lookahead and do an extra
>> readlink in the cases that need it.
> 
> While this worked, it ended up being the wrong thing to do, making two
> places where readlink is called, one of them with a dummy buffer. The
> right way to do it is rework the flow so that the existing readlink is
> "naturally" hit where needed. This amounts to:
> 
> - Letting .. processing that cancels path components go through the
>   same code path as new path components, rather than handling it
>   early, and just skipping the actual readlink if we already know we
>   have a dir.
> 
> - Also treating a zero-length final component as something that goes
>   through the readlink code path.
> 
> There was a fair amount of reorganizing needed to make this work out,
> but the end result is clean and non-redundant and code size is almost
> the same as before with the missing-ENOTDIR bugs.
> 
> Speaking of code size, on 32-bit archs the proposed explicit realpath
> is roughly the same size as stat+fstat+fstatat (a little over 1k on
> i386), which were needed to implement the old lazy realpath in terms
> of procfs. So for minimal static linking, resulting code size may be
> same or smaller. (Of course it's larger if stat is already linked for
> other reasons.)
> 
> New draft attached. It's possible that there are regressions since I
> haven't put together an automated testset. I'm not sure if I'll try to
> merge it in this release cycle still or not; that probably depends on
> how easy or difficult automating these tests ends up being.
> 
The new draft looks good to me. I've also done some basic manual testing 
(not covering all proposed cases) and haven't found any issues.

I don't see why the size of stack has to be PATH_MAX+1 though. To 
address the issue with symlink targets of PATH_MAX-1 length, it seems 
sufficient to just do the following:

-               ssize_t k = readlink(output, stack, p);
-               if (k==p) goto toolong;
+               ssize_t k = readlink(output, stack, p+1);
+               if (k==p+1) goto toolong;

Since p is never past the end of the stack, there is no harm in allowing 
k == p.

Alexey

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-24  9:21                 ` Alexey Izbyshev
  2020-11-24 14:35                   ` Rich Felker
@ 2020-11-25 15:02                   ` Rich Felker
  2020-11-25 19:40                     ` Alexey Izbyshev
  1 sibling, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-11-25 15:02 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

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

On Tue, Nov 24, 2020 at 12:21:36PM +0300, Alexey Izbyshev wrote:
> On 2020-11-24 09:30, Rich Felker wrote:
> >I think before this goes upstream we should have a good set of
> >testcases that can be contributed to libc-test. Do you have ideas for
> >coverage? Some that come to mind:
> >
> Added some more ideas.
> 
> >- Absolute argument starting with /, //, and ///
> - Absolute argument equal to one of /, //, and ///
> >- Absolute symlink target starting with /, //, and ///
> - Absolute symlink target equal to one of /, //, and ///, with the
> link separated from the following component with /, //
> >- Final / after symlink-to-dir, dir, symlink-to-nondir, nondir
> - Intermediate / after symlink-to-nondir, nondir
> >- Final / in link contents after [the above]
> - Multiple / after ., .., normal component
> >- Initial .. in argument, cwd root or non-root
> >- Initial .. in symlink target, symlink in root or non-root
> >- Initial ...
> >- .. following symlink-to-dir, dir, symlink-to-nondir, nondir
> - . following symlink-to-dir, dir, symlink-to-nondir, nondir
> >- More .. than path depth
> >- Null argument
> >- Empty string argument
> - Argument consisting of a single ., ..
> >- Empty string link contents (testable only with seccomp hack)
> >- Argument valid abs path exact length PATH_MAX-1
> >- Argument valid rel path exact length PATH_MAX-1 to short abs path
> - Argument with PATH_MAX length (ENAMETOOLONG)
> - A relative symlink in the argument such that the length of the
> result is PATH_MAX-1 (valid path), PATH_MAX (ENAMETOOLONG)
> - An absolute symlink in the argument similar to the above
> - A relative argument with the current directory similar to the above
> - An argument consisting of a single (relative, absolute) symlink
> with the target having length PATH_MAX-1
> - An argument ending with a relative symlink with the target having
> length PATH_MAX-1 (ENAMETOOLONG)
> - An argument ending with an absolute symlink with the target having
> length PATH_MAX-1 (valid path)

OK, here are my WIP testcases. They only cover static cases, not
anything that depends on implementation-defined value of PATH_MAX or
how the working directory combines with tests to reach PATH_MAX.

The way you run the tests is by running ../setup.sh then ../checker.sh
from a scratch subdir of the dir containing the test files (realpath.c
also needs to be compiled and linked against the realpath impl you
want to test)

Format of the testcase file is one test per line, input and expected
output separated by whitespace. . and .. in expected output are
translated based on working dir. Error names are treated as output
since they don't fall in same namespace (no initial /).

All tests are currently passing for me.

If you have ideas for additional static tests that fit the form and
are easy to add, please suggest.

Rich

[-- Attachment #2: setup.sh --]
[-- Type: application/x-sh, Size: 434 bytes --]

[-- Attachment #3: checker.sh --]
[-- Type: application/x-sh, Size: 335 bytes --]

[-- Attachment #4: realpath.c --]
[-- Type: text/plain, Size: 510 bytes --]

#define _XOPEN_SOURCE 700
#include <stdlib.h>
#include <stdio.h>
#include <limits.h>
#include <errno.h>

#define E(x) { x, #x }

static const struct {
	int ec;
	char *name;
} errs[] = { E(ENOENT), E(ENOTDIR), E(ENAMETOOLONG), E(ELOOP), E(EACCES), E(0) };

int main(int argc, char *argv[])
{
	while (*++argv) {
		char *s = realpath(*argv, (char[PATH_MAX]){""});
		//char *s = realpath(*argv, 0);
		if (!s) {
			int i;
			for (i=0; errs[i].ec && errs[i].ec!=errno; i++);
			s = errs[i].name;
		}
		puts(s);
	}
}

[-- Attachment #5: testcases.txt --]
[-- Type: text/plain, Size: 1158 bytes --]

/	/
//	//
///	/
/dev	/dev
/..	/
//..	//
///..	/
/dev/..	/
/dev/../..	/
/dev/../../..	/
dir	./dir
./dir	./dir
file	./file
./file	./file
noent	ENOENT
./noent	ENOENT
dir/	./dir
file/	ENOTDIR
noent/	ENOENT
link_to_dir/	./dir
link_to_file/	ENOTDIR
link_to_noent/	ENOENT
dir/..	.
file/..	ENOTDIR
noent/..	ENOENT
link_to_dir/..	.
link_to_file/.. ENOTDIR
link_to_noent/..	ENOENT
dlink_to_dir	./dir
dlink_to_file	ENOTDIR
dlink_to_noent	ENOENT
dlink_to_link_to_dir	./dir
dlink_to_link_to_file	ENOTDIR
dlink_to_link_to_noent	ENOENT
dlink_to_dir/	./dir
dlink_to_file/	ENOTDIR
dlink_to_noent/	ENOENT
dlink_to_link_to_dir/	./dir
dlink_to_link_to_file/	ENOTDIR
dlink_to_link_to_noent/	ENOENT
dlink_to_dir/..	.
dlink_to_file/..	ENOTDIR
dlink_to_noent/..	ENOENT
dlink_to_link_to_dir/..	.
dlink_to_link_to_file/..	ENOTDIR
dlink_to_link_to_noent/..	ENOENT
abs1	/
abs1/	/
abs1/dev	/dev
abs1/dev/	/dev
abs2	/
abs2/	/
abs2/dev	/dev
abs2/dev/	/dev
abs3	/
abs3/	/
abs3/dev	/dev
abs3/dev/	/dev
abs4	/
abs4/	/
abs4/dev	/dev
abs4/dev/	/dev
alt1	//
alt1/	//
alt1/.	//
alt2	//
alt2/	//
alt2/.	//
a	./a
a/..	.
...	./...
.../	./...
.../..	.
..	..
../	..
../.	..
a/../..	..
../noent	ENOENT

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-25  5:40               ` Alexey Izbyshev
@ 2020-11-25 15:03                 ` Rich Felker
  0 siblings, 0 replies; 20+ messages in thread
From: Rich Felker @ 2020-11-25 15:03 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

On Wed, Nov 25, 2020 at 08:40:02AM +0300, Alexey Izbyshev wrote:
> I don't see why the size of stack has to be PATH_MAX+1 though. To
> address the issue with symlink targets of PATH_MAX-1 length, it
> seems sufficient to just do the following:
> 
> -               ssize_t k = readlink(output, stack, p);
> -               if (k==p) goto toolong;
> +               ssize_t k = readlink(output, stack, p+1);
> +               if (k==p+1) goto toolong;
> 
> Since p is never past the end of the stack, there is no harm in
> allowing k == p.

Indeed. I think I was concerned about it clobbering the first byte of
the live part of the stack, but on error the stack is discarded
anyway, so that seems to be a non-issue.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [musl] realpath without procfs -- should be ready for inclusion
  2020-11-25 15:02                   ` Rich Felker
@ 2020-11-25 19:40                     ` Alexey Izbyshev
  0 siblings, 0 replies; 20+ messages in thread
From: Alexey Izbyshev @ 2020-11-25 19:40 UTC (permalink / raw)
  To: musl

On 2020-11-25 18:02, Rich Felker wrote:
> On Tue, Nov 24, 2020 at 12:21:36PM +0300, Alexey Izbyshev wrote:
>> On 2020-11-24 09:30, Rich Felker wrote:
>> >I think before this goes upstream we should have a good set of
>> >testcases that can be contributed to libc-test. Do you have ideas for
>> >coverage? Some that come to mind:
>> >
>> Added some more ideas.
>> 
>> >- Absolute argument starting with /, //, and ///
>> - Absolute argument equal to one of /, //, and ///
>> >- Absolute symlink target starting with /, //, and ///
>> - Absolute symlink target equal to one of /, //, and ///, with the
>> link separated from the following component with /, //
>> >- Final / after symlink-to-dir, dir, symlink-to-nondir, nondir
>> - Intermediate / after symlink-to-nondir, nondir
>> >- Final / in link contents after [the above]
>> - Multiple / after ., .., normal component
>> >- Initial .. in argument, cwd root or non-root
>> >- Initial .. in symlink target, symlink in root or non-root
>> >- Initial ...
>> >- .. following symlink-to-dir, dir, symlink-to-nondir, nondir
>> - . following symlink-to-dir, dir, symlink-to-nondir, nondir
>> >- More .. than path depth
>> >- Null argument
>> >- Empty string argument
>> - Argument consisting of a single ., ..
>> >- Empty string link contents (testable only with seccomp hack)
>> >- Argument valid abs path exact length PATH_MAX-1
>> >- Argument valid rel path exact length PATH_MAX-1 to short abs path
>> - Argument with PATH_MAX length (ENAMETOOLONG)
>> - A relative symlink in the argument such that the length of the
>> result is PATH_MAX-1 (valid path), PATH_MAX (ENAMETOOLONG)
>> - An absolute symlink in the argument similar to the above
>> - A relative argument with the current directory similar to the above
>> - An argument consisting of a single (relative, absolute) symlink
>> with the target having length PATH_MAX-1
>> - An argument ending with a relative symlink with the target having
>> length PATH_MAX-1 (ENAMETOOLONG)
>> - An argument ending with an absolute symlink with the target having
>> length PATH_MAX-1 (valid path)
> 
> OK, here are my WIP testcases. They only cover static cases, not
> anything that depends on implementation-defined value of PATH_MAX or
> how the working directory combines with tests to reach PATH_MAX.
> 
> The way you run the tests is by running ../setup.sh then ../checker.sh
> from a scratch subdir of the dir containing the test files (realpath.c
> also needs to be compiled and linked against the realpath impl you
> want to test)
> 
> Format of the testcase file is one test per line, input and expected
> output separated by whitespace. . and .. in expected output are
> translated based on working dir. Error names are treated as output
> since they don't fall in same namespace (no initial /).
> 
> All tests are currently passing for me.
> 
Tests pass for me too. For the previous procfs-based impl and for glibc 
realpath(), the set of failing tests is exactly the same (all tests for 
//):

// -> / (expected //)
//.. -> / (expected //)
alt1 -> / (expected //)
alt1/ -> / (expected //)
alt1/. -> / (expected //)
alt2 -> / (expected //)
alt2/ -> / (expected //)
alt2/. -> / (expected //)

> If you have ideas for additional static tests that fit the form and
> are easy to add, please suggest.
> 
A test for ELOOP is easy to add.

Some of the skipped tests of the initially proposed list also fall 
within "static tests" limitations, e.g.:

* trailing . (could be added in the form of <normal, ENOTDIR, ENOENT> 
triples as existing tests for trailing / and ..)

* initial .. (or .) in a symlink target

* multiple / in the middle of the argument before a component that is 
not going to be removed

Alexey





^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-11-25 19:40 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-22 22:56 [musl] realpath without procfs -- should be ready for inclusion Rich Felker
2020-11-23  2:03 ` Alexey Izbyshev
2020-11-23  3:17   ` Érico Nogueira
2020-11-23  3:34     ` Rich Felker
2020-11-23  3:19   ` Rich Felker
2020-11-23 18:56     ` Rich Felker
2020-11-23 20:53       ` Rich Felker
2020-11-24  3:39         ` Alexey Izbyshev
2020-11-24  4:26           ` Rich Felker
2020-11-24  5:13             ` Alexey Izbyshev
2020-11-24  6:30               ` Rich Felker
2020-11-24  9:21                 ` Alexey Izbyshev
2020-11-24 14:35                   ` Rich Felker
2020-11-24 20:17                     ` Rich Felker
2020-11-25 15:02                   ` Rich Felker
2020-11-25 19:40                     ` Alexey Izbyshev
2020-11-24 20:31             ` Rich Felker
2020-11-25  5:40               ` Alexey Izbyshev
2020-11-25 15:03                 ` Rich Felker
2020-11-24  3:41     ` Alexey Izbyshev

mailing list of musl libc

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.vuxu.org/musl

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 musl musl/ http://inbox.vuxu.org/musl \
		musl@inbox.vuxu.org
	public-inbox-index musl

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.musl


code repositories for the project(s) associated with this inbox:

	https://git.vuxu.org/mirror/musl/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git