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 > > >#include > > >#include > > >#include > > >#include > > > > > >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