segfault in "su" if realpath fails

[avih]

The applet (and function) realpath can fail with "Function not implemented" (GetLastError() == 1) for some ramdisks paths, e.g. using ImDisk or OSFMount.

E.g. realpath r:/ where r:/ is a ramdisk.

In suw32_main, this results in getcwd(NULL, 0) returning a correct path, e.g. R:\, but then xmalloc_realpath fails and returns NULL, which this function doesn't take into account as a possibility, and then segfaults.

Unrelated and regardless, I think it also leaks the result of getcwd, which the MS docs say should be deallocated with free (when the argument is NULL).

The failure is here where the handle h is valid but status becomes 0 (with GetLastError() == 1): https://github.com/rmyorston/busybox-w32/blob/1273a1ddcab67c8ccca61c7c7c52c6049be4c336/win32/mingw.c#L1558-L1560

Not sure how to address it, but as a workaround, if status is 0 and last error is 1 (and the path looks like <letter>:/... (or backslash), then return it as is, something like this:

diff --git a/win32/mingw.c b/win32/mingw.c
index 79f85ac61..ef980943f 100644
--- a/win32/mingw.c
+++ b/win32/mingw.c
@@ -1647,6 +1647,12 @@ static char *resolve_symlinks(char *path)
        if (status != 0 && status < MAX_PATH) {
            ptr = normalize_ntpathA(path);
            goto end;
+       } else if (GetLastError() == ERROR_INVALID_FUNCTION && path[0] && path[1] == ':') {
+           // GetFinalPathNameByHandleA can fail for some paths
+           // on virtual disks with last error 1. Use path anyway.
+           ptr = normalize_ntpathA(path);
+           goto end;
        }
    }

This makes realpath . and su work also when invoked in a ramdisk path.

[avih]

And this would be hardening of suw32_main:

diff --git a/loginutils/suw32.c b/loginutils/suw32.c
index 240a692bc..a65d03936 100644
--- a/loginutils/suw32.c
+++ b/loginutils/suw32.c
@@ -40,7 +40,7 @@ int suw32_main(int argc UNUSED_PARAM, char **argv)
        unsigned opt;
        char *opt_command = NULL;
        SHELLEXECUTEINFO info;
-       char *bb_path, *cwd, *q, *args;
+       char *bb_path, *cwd, *rcwd, *q, *args;
        DECLARE_PROC_ADDR(BOOL, ShellExecuteExA, SHELLEXECUTEINFOA *);

        opt = getopt32(argv, "c:NW", &opt_command);
@@ -74,11 +74,15 @@ int suw32_main(int argc UNUSED_PARAM, char **argv)
         * a network share it may not be available once we have elevated
         * privileges.
         */
-       cwd = xmalloc_realpath(getcwd(NULL, 0));
-       q = quote_arg(cwd);
+       cwd = getcwd(NULL, 0);
+       if (!cwd)
+               return 1;
+       rcwd = xmalloc_realpath(cwd);
+       q = quote_arg(rcwd ? rcwd : cwd);
        args = xasprintf("--busybox ash -d %s -t \"BusyBox ash (Admin)\"", q);
        free(q);
        free(cwd);
+       free(rcwd);

        if (opt & OPT_N)
                args = xappendword(args, "-N");

Unrelated and regardless, I think it also leaks the result of getcwd

While the patch above fixes that, I think bb_path is also leaked.

[rmyorston]

I've applied a fix to su. I'm still thinking about what to do about realpath(1).

All allocated memory is now referenced. It's also freed, if required.

[avih]

Thanks.

In su, there are still two return paths which don't free - if INIT_PROC_ADDR fails (unliekely) or if ShellExecuteExA fails.

I'm still thinking about what to do about realpath(1)

Yeah. I'm also not sure how to address it...

[avih]

By the way, do you want to consider a more generic approach to ENABLE_FEATURE_CLEAN_UP ? maybe something like this (untested)?

#if ENABLE_FEATURE_CLEAN_UP
    void oncleanup_free(void *mem);
    void oncleanup_fclose(FILE *file);
    void oncleanup_close(int fd);
    void oncleanup_call(void (*fn)(void *ctx), void *ctx);
    void do_cleanup(void);
#else
    #define oncleanup_free(mem)
    #define oncleanup_fclose(file)
    #define oncleanup_close(fd)
    #define oncleanup_call(fn, ctx)
    #define do_cleanup()
#endif

#if ENABLE_FEATURE_CLEAN_UP
enum cleanup_type {
    FREE, FCLOSE, CLOSE, CALL,
}

typedef struct cleanup {
    cleanup_type type;
    union u {
        void *mem;
        FILE *file;
        int fd;
        struct call {
            void *fn;
            void *ctx;
        }
    }
} cleanup;

static cleanup *cleanup_data;
static size_t cleanup_len, cleanup_cap

void do_cleanup(void)
{
    while (cleanup_len--) {
        cleanup *d = cleanup_data + cleanup_len;
        switch (d->type) {
            case FREE: free(d->u.mem); break;
            case FCLOSE: fclose(d->u.file); break;
            case CLOSE: close(d->u.fd); break;
            case CALL: d->u.call.fn(d->u.call.ctx); break;
        }
    }
    free(cleanup_data);
}

static void add_cleanup(cleanup *item)
{
    if (cleanup_len == cleanup_cap) {
        cleanup_cap = cleanup_cap ? cleanup_cap * 2 : 64;
        cleanup_data = xrealloc(cleanup_data, cleanup_cap * sizeof(cleanup));
    }
    cleanup_data[cleanup_len++] = *item;
}

void oncleanup_free(void *mem)
{
    add_cleanup(&(cleanup){.type = FREE, .u.mem = mem});
}

void oncleanup_fclose(FILE *file)
{
    add_cleanup(&(cleanup){.type = FCLOSE, .u.file = file});
}

void oncleanup_close(int fd)
{
    add_cleanup(&(cleanup){.type = CLOSE, .u.fd = fd});
}

void oncleanup_call(void (*fn)(void *ctx), void *ctx)
{
    add_cleanup(&(cleanup){.type = CALL, .u.call = {.fn = fn, .ctx = ctx}});
}
#endif /* ENABLE_FEATURE_CLEAN_UP */

So that any potential cleanup can be added right after it's assigned an allocation, e.g.

bb_path = xstrdup(...);
oncleanup_free(bb_path);
...
do_cleanup();

[rmyorston]

OK, here's the latest:

• I've rearranged the code in su so everything should be freed.
• On consideration I decided to let realpath(3) succeed without resolving symlinks when the filesystem doesn't support the operation. I took the view that the convenience of having realpath(1) and cd symlink work rather than fail is worth the loss of strict correctness.
• The proposal for enhancing FEATURE_CLEAN_UP is not something I'd entertain in busybox-w32. It's a matter for upstream.

[avih]

Thanks.

Do correct me if I'm wrong though, but the new fallback code path requires free, while previously it never required free, because it returned NULL or a pointer into the (possibly modified) input argument path.

As a result, the function realpath at mingw.c can now leak the return value of resolve_symlinks if it enters the fallback code.

The proposal for enhancing FEATURE_CLEAN_UP is not something I'd entertain in busybox-w32. It's a matter for upstream.

True, but it impacts mingw code as well, which requires "manual" handling of cleanup, which might be simplified with such setup.

Nevertheless, I'm not so convinced in its value myself, though I do like the "destructor" setup.

[rmyorston]

Oops, yes, there is a leak there.

It isn't necessary to strdup(path) in the fallback path of resolve_symlinks. This should do, I think:

        } else if (err_win_to_posix() == ENOSYS) {
            ptr = path;
            goto end;
        }

[avih]

This should do, I think:

Yes, or ptr = normalize_ntpathA(path); depending if you think it may be required.

[rmyorston]

Actually, we can do slightly better: if the path ends with a symlink it's resolved. So resolve_symlinks() can return that.

[avih]

This change gets rid of a handful of realpath-related test failures on ReactOS and Windows XP.

Nice.

Also, leak fixed ACAICT, and I can't spot new ones ;)

So why does the main return path apply normalize_ntpathA, while the fallback return path doesn't? (I saw what it does, but didn't try to understand what is it good for)

Do you maybe expect that it would always be no-op at the fallback path?

One case I can think of is when using the realpath applet with an argument which would be affected by normalize_ntpathA, however, I don't know if such paths exist which would end up at the fallback path.

[rmyorston]

I'm not aware of any reason why normalize_ntpathA would need to be applied to the fallback output.

[rmyorston]

This issue should be resolved in the latest release, FRP-5398.