leahneukirchen / extrace

trace exec() calls system-wide
Other
122 stars 9 forks source link

use openat() to reduce races #2

Closed FauxFaux closed 6 years ago

FauxFaux commented 6 years ago

Instead of repeatedly reopening /proc/%d/foo, open /proc/%d once, then open/read foo relative to that.

This should reduce the chance that incorrect data is presented.

I've left pid_depth alone. I did rewrite it, but the new implementation seemed to be slower (more syscalls) and more complex. I think it's probably worth capturing the cmdline before calculating the depth, as that's by far the most interesting piece of information. That, or capture the command name (not the full command-line) from the first stat invocation, so we have that to print.

The handling of errors in fdopen is intentionally sloppy. fdopen fails only if malloc fails, I believe. If this happens, we might leak a file descriptor. I don't think the extra code is worth it, but I'm happy to fix it if you do.

Open question: If we get a notification about a pid that's already died, why not print something? Line 274's TODO.

leahneukirchen commented 6 years ago

Good idea, but unfortunately, this will not work.

Example:

#define _XOPEN_SOURCE 700
#include <fcntl.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>

int main()
{
    pid_t pid;

    pid = fork();
    if (pid == 0) {
        pause();
        exit(0);
    }

    char buf[100];
    sprintf(buf, "/proc/%d", pid);
    int dirfd = open(buf, O_DIRECTORY);
    printf("%d\n", dirfd);

    int fd1 = openat(dirfd, "comm", O_RDONLY);
    printf("%d\n", fd1);

    kill(pid, SIGTERM);
    int x;
    wait(&x);

    int fd2 = openat(dirfd, "environ", O_RDONLY);
    printf("%d %m\n", fd2);

    exit(0);
}

After the process died, open fails with ESRCH.

FauxFaux commented 6 years ago

Yeah, I noticed that it doesn't allow you to actually view data for dead processes. It should be faster, however (and performance is everything when we're racing the process exit). It appears better at catching short-lived processes on my desktop (amd64 i7-3770k), but it's significantly limited by the depth check being before anything else, which consumes most of the process lifetime.

I believe it also helps prevent the (incredibly unlikely) case where pids are reused between the event being raised, and us reading the environment.

leahneukirchen commented 6 years ago

Committed with a few style fixes.

I'll move the depth lookup down later.