Close stdin / stdout / stderr in child process after fork

[tsg]

We use go-daemon in the Elastic Beats init scripts on the platforms that don't support systemd. We occasionally get reports about the init scripts failing when executed over SSH, usually via some deployment tool.
#5 seems related although not quite exactly what we experience. The easiest way to reproduce the problem is to run something like this:

ssh host "god sleep 10"

SSH hangs for 10 seconds.

Closing FDs 0, 1, 2 on the first fork (the one for daemonizing the go-daemon process itself) seems necessary to detach from the pseudo tty when running in background. I did the minimal change that seems to fix our issue, but other changes might be useful as well here (setsid, redirect stdout/stderr to /dev/null), etc.

[fiorix]

I spent little time chasing this since #5, and considered your change before. The problem is that if you execute a file that does not exist, and stdout/stderr are closed, we don't get a chance to report errors.

For example, before your change:

$ ./god lalala
lalala: No such file or directory
$

After your change:

$ ./god lalala
$

So I guess there's more to look at.

[tsg]

Thanks for the quick answer, I see the issue. If I get the code correctly, execvp errors are the only ones that get reported this way, right?

Out of curiosity I checked how debian's start-stop-daemon handles this:

start-stop-daemon --start --background --exec lalala
start-stop-daemon: unable to stat lalala (No such file or directory)

So it seems (and I confirmed with strace) that it does a stat to check that the file exist, before forking.

This is not bullet proof, if I create a file without execution permission, start-stop-daemon fails to report an error. However, IMHO, this is a pretty good compromise because it catches the most common error case. We could probably also check that the file is executable.

WDYT? I can do the change if you like the idea.

The only other option that I can think of is to close the FDs after half a second or so, but that's a bit ugly, slows down SSH connections, and could lead to races.

[fiorix]

That's where I was going to, also. Do a stat and check for exec permission, should be good enough given start-stop-daemon does similar.

Yeah go ahead and update the PR and let's move fwd.

[fiorix]

The check for exec permission needs to check against the user and group that we're going to run with.. perhaps that's why start-stop-daemon doesn't do it. :)

edit: if we're restrictive based on that, we might fall into checking posix extended acls and all that jazz.

[tsg]

@fiorix I added another commit that does a stat on the file and checks if any of the executable flags are set. The check passes if any of the bits (user, group, other) is set. It still doesn't catch all possible permission errors, but I think the most common ones should be covered without false positives.

There is a downside to this, though: you can no longer execute anything from $PATH. In other words, god sleep 1 no longer works, you need god /bin/sleep 1.

[fiorix]

That sucks. We can scan through $PATH and look for the file in it. I'm a bit rusty in C but I think this would do:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/syslimits.h>

// executable_abspath checks whether the given file name exists in
// the current directory or in $PATH, and returns a pointer containing
// a string with the absolute path of the executable file. The returned
// pointer points to a static buffer that is reused on subsequent calls.
// Returns NULL if the given file name cannot be found.
char *executable_abspath(char *filename) {
    static char abspath[PATH_MAX];

    char *path = getenv("PATH");
    if (!path) return NULL;

    char *v[PATH_MAX/2], **paths = v;
    char *p = strdup(path);
    path = p;

    while (*p) {
        while (*p && *p == ':') *p++ = '\0';
        if (*p) *(paths++) = p;
        while (*p && *p != ':') p++;
    }
    *paths = NULL;

    int l, fnlen = strlen(filename);
    for (paths = v; *paths; paths++) {
        l = strlen(*paths) + fnlen + 2;
        if (l > sizeof(abspath)) continue;
        snprintf(abspath, l, "%s/%s", *paths, filename);
        if (0 /* stat(abspath) exists and is executable */) {
            free(path);
            return abspath;
        }
    }
    free(path);
    return NULL;
}

int main() {
    printf("abs=%s\n", executable_abspath("sleep"));
}

It needs an #ifdef to check for Linux vs OS X (which is what I used) and include the equivalent syslimits for PATH_MAX. You can probably put your stuff in the if (0 ...) block replacing it.

[fiorix]

Although the comments say it checks the current directory, it doesn't. Needs that too.

[tsg]

I wouldn't say it's that bad, since this is primarily meant to be used in init scripts, in which it's good practice to use absolute paths anyway. And it's easy to do god which sleep 10, which works great.

I would be afraid of corner cases when looking for the executable in $PATH + working directory, like differences in behavior between our implementation and execvp. Check the source code of execvp, it looks pretty complex.

[fiorix]

Print Permission denied instead.

[fiorix]

Up to you, I'll probably put that in later on anyway. It's not too different from what execvp is doing except that they handle : at the beginning, end, and double to search current directory and my code does not.

I've added a comment to your last commit to change the error message to the standard Permission denied when the file mode doesn't have executable bits set.

[tsg]

Thanks @fiorix, I pushed a fix for the err message.

[fiorix]

This patch caused problems with the logger thread. I've pushed another fix, which also addresses the security issues from other tickets.