Re: [musl] realpath without procfs -- should be ready for inclusion

[Rich Felker <dalias@libc.org>]

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