Re: [hackers] Add realpath utility

From: Dimitris Papastamos <sin_AT_2f30.org>
Date: Sat, 21 Nov 2015 10:46:37 +0000

On Fri, Nov 20, 2015 at 02:29:25PM -0600, Brad Barden wrote:
> > I'm torn on what would be best.
>
> The more I think about it, the uglier I think readlink/realpath are
> altogether. I don't see why they were ever separate utilities.
>
> Attached is a patch for what I mentioned I tried, all flags implemented
> in readlink, and realpath is a link handled like [ -> test. See what you
> think. I still really don't like the way it screws up sbase-box's usage
> message.

I don't terribly mind this approach. The only thing I am worried
about is the actual implementation of realpathm() which looks quite
convoluted at this point.

> +char *
> +realpathm(const char * path, char * resolvedpath) {
> + const char *p = path;
> + char buf1[PATH_MAX] = {0}, buf2[PATH_MAX] = {0},
> + *a = buf1, *b = buf2, *ret = NULL;
> + int i = 0, m = 0;
> +
> + if (!p)
> + errno = EINVAL;
> + goto badpath;

Missing braces here.

> + if (!*p) {
> + errno = ENOENT;
> + goto badpath;
> + }
> +
> + if (strnlen(path, PATH_MAX) == PATH_MAX) {
> + errno = ENAMETOOLONG;
> + goto badpath;
> + }
> +
> + if (*p == '/') {
> + p++;
> + } else {
> + if (!realpath(".", a)) {
> + goto badpath;
> + }

Can we use getcwd() here?

> + }
> + strcat(a, "/");

I would prefer if we used strlcat/strlcpy() throughout.

> + while (*p) {
> + i = strlen(a);
> + while (*p && *p != '/') {
> + if (i >= PATH_MAX) {
> + errno = ENAMETOOLONG;
> + goto badpath;
> + }
> + a[i++] = *p++;
> + }
> + a[i] = '\0';
> + if (*p)
> + ++p;
> + if (i > 2 && !strcmp(&a[i-3], "/..")) {
> + if (m)
> + --m;
> + if (i == 3) {
> + strcpy(a, "/");
> + i = 1;
> + }
> + else {
> + i -= 3;
> + while (i > 0 && a[--i] != '/') ;
> + a[i] = '\0';
> + }
> + } else if (i > 1 && !strcmp(&a[i-2], "/.")) {
> + if (i == 2) {
> + strcpy(a, "/");
> + i = 1;
> + } else {
> + i -= 2;
> + a[i] = '\0';
> + }
> + } else if (a[i] == '/') {
> + if (i > 1)
> + a[--i] = '\0';
> + } else if (m) {
> + ++m;
> + } else if (!realpath(a, b)) {
> + if (errno == ENOENT) {
> + m = 1;
> + } else {
> + goto badpath;
> + }
> + } else if (a == buf1) {
> + a = buf2;
> + b = buf1;
> + } else {
> + a = buf1;
> + b = buf2;
> + }
> + if (*p && a[i-1] != '/' && strlcat(a, "/", PATH_MAX) >= PATH_MAX) {
> + errno = ENAMETOOLONG;
> + goto badpath;
> + }
> + }
> + i = strlen(a)-1;
> + if (a[i] == '/')
> + a[i] = '\0';
> + if (!(ret = resolvedpath))
> + ret = emalloc(PATH_MAX);
> + estrlcpy(ret, a, PATH_MAX);
> +
> +badpath:
> + return ret;

This bit looks like black magic to me. Can we break this into some
helper functions? For example one function that picks out the
individual components of the path and another function that actually
resolves that component? Then iteratively we construct the resolved
path by appending. The helper functions will be static and only
realpathm() will be visible. Or some other scheme that makes the
intent of the blocks more clear (comments would also help).

> >From 05ef3f271774b2989273c177ce83d1a320fa8dc5 Mon Sep 17 00:00:00 2001
> From: Brad Barden <brad_AT_13os.net>
> Date: Fri, 20 Nov 2015 14:12:23 -0600
> Subject: [PATCH 2/2] restore readlink -e and -m flags, add realpath
>
> -e and -m are added back to the readlink utility, now working as
> intended
>
> when called as 'realpath', readlink's default behavior is now
> readlink -f
>
> realpath is implemented with a link to readlink
>
> man page for readlink is updated and man page for realpath is added

BTW, the realpath manpage is not installed when doing make install.

> - return fshut(stdout, "<stdout>");

Don't remove the fshut calls, we need them.
Received on Sat Nov 21 2015 - 11:46:37 CET