Poll the fifo once before reading to detect when a process never loads libc The fifo should be opened for writing from the traced process itself after the LD_PRELOAD injection. This however will not happen if the process does not dynamically link libc. And this happens to be the case in most Go programs. This change adds two tests to ensure fsatrace exits when run on a process that does not load libc and does not hang if the write end of the fifo exits before the read end consumes any bytes. Change-Id: If1a717614f42761c706724d1b6e560db055a5539
diff --git a/Makefile b/Makefile index 7247ed7..6556487 100644 --- a/Makefile +++ b/Makefile
@@ -19,11 +19,16 @@ PLAT=unix CPPFLAGS=-D_GNU_SOURCE -D_DEFAULT_SOURCE=1 LDFLAGS=-g +PLAT_TESTS=noop-test OS=$(shell uname -s) +MACHINE=$(shell uname -m) ifeq ($(OS),Linux) -LDLIBS=-ldl + LDLIBS=-ldl + ifeq ($(MACHINE),x86_64) + PLAT_TESTS:=$(PLAT_TESTS) nolibc-test + endif endif INSTALLDIR=$(HOME)/.local/bin @@ -58,7 +63,7 @@ rm -f fsatest$(EXE) fsatest32$(EXE) $(patsubst %.c,%.o,$(FSATEST_SRCS)) $(patsubst %.c,%.d,$(FSATEST_SRCS)) TEST_CC_CMD=$(CC) -c -D_GNU_SOURCE -D_DEFAULT_SOURCE=1 -std=c99 src/fsatrace.c -o /tmp/fsatrace.o -test: fsatrace$(EXE) +test: fsatrace$(EXE) $(PLAT_TESTS) ./fsatrace$(EXE) ewrmdqt - -- cp $(LS) /tmp/foo ./fsatrace$(EXE) ewrmdqt - -- mv -f /tmp/foo /tmp/bar ./fsatrace$(EXE) ewrmdqt - -- gzip -f /tmp/bar
diff --git a/src/fsatrace.c b/src/fsatrace.c index 3d36842..46a74a7 100644 --- a/src/fsatrace.c +++ b/src/fsatrace.c
@@ -77,6 +77,29 @@ if (verr) aerror(nargs, args, "waiting for command completion"); break; + case ERR_FIFO_TIMEOUT: + aerror(nargs, args, + "Timeout opening the FIFO for writing the traced operations.\n" + "\n" + "This is more than likely because the process being traced does\n" + "not dynamically link libc. The fsatrace program uses LD_PRELOAD\n" + "to inject hooks around libc calls, and this error should only\n" + "occur when those hooks are not injected into the process being\n" + "traced.\n" + "\n" + "A simple \"fix\" is to wrap the program being launched inside\n" + "another program that dynmically links libc, but that approach\n" + "is invalid because the process not dynamically linking libc\n" + "indicates the process will not run the hooks installed by\n" + "fsatrace to trace filesystem accesses. Thus, doing something\n" + "like `bash -c \"exec my_program\"` is unlikely to achieve your\n" + "reason for using fsatrace in the first place.\n" + "\n" + ); + break; + case ERR_FIFO_UNKNOWN: + aerror(nargs, args, "Error opening FIFO for writing the traced operations."); + break; case OK: if (rc) { if (verr)
diff --git a/src/proc.h b/src/proc.h index 8c60a42..e0967f8 100644 --- a/src/proc.h +++ b/src/proc.h
@@ -6,6 +6,8 @@ ERR_PROC_STOPPED, ERR_PROC_WAIT, ERR_PROC_UNKNOWN, + ERR_FIFO_TIMEOUT, + ERR_FIFO_UNKNOWN, }; void procPath(char *);
diff --git a/src/unix/proc.c b/src/unix/proc.c index e706d9b..5b95690 100644 --- a/src/unix/proc.c +++ b/src/unix/proc.c
@@ -3,6 +3,7 @@ #include <fcntl.h> #include <libgen.h> #include <limits.h> +#include <poll.h> #include <spawn.h> #include <stdarg.h> #include <stdbool.h> @@ -188,20 +189,59 @@ execvp(args[0], args); _exit(EXIT_FAILURE); break; - default: { + default: char *buf; size_t sz; char c; - const int fd = open(fifo, O_RDONLY); + int fd = open(fifo, O_RDONLY | O_NONBLOCK); if (fd < 0) { error("failed to open fifo %s for read"); - return ERR_PROC_UNKNOWN; + return ERR_FIFO_UNKNOWN; + } + + // Initial poll to wait for the write side to open. If this times out, it's likely the + // traced process never dynamically loads libc. + // + // fsatrace allows the forked process 2 seconds to load libc and run the installed hooks. + // Otherwise, it's assumed the forked process will not load libc. This 2 second timeout is + // designed to be long enough to allow even slow builders to avoid hitting the timeout, but + // short enough that a user would not give up and send SIGINT before the timeout expires. + // Sending SIGINT before the timeout expires would preempt printing a helpful error message. + { + struct pollfd pfd = { + .fd = fd, + .events = POLLIN, + .revents = 0, + }; + int poll_result = poll(&pfd, 1, 2000); + if (poll_result == -1) { + error("poll()"); + close(fd); + return ERR_FIFO_UNKNOWN; + } + if (poll_result == 0) { + close(fd); + return ERR_FIFO_TIMEOUT; + } + if (!(pfd.revents & POLLIN)) { + error("Initial poll of fifo did not receive POLLIN: %d", pfd.revents); + close(fd); + return ERR_FIFO_UNKNOWN; + } + } + + // Remove the O_NONBLOCK flag to make subsequent read operations blocking. + if (fcntl(fd, F_SETFL, O_RDONLY) == -1) { + error("fcntl F_SETFL"); + close(fd); + return ERR_FIFO_UNKNOWN; } FILE *memstream = open_memstream(&buf, &sz); while (read(fd, &c, 1) > 0) fprintf(memstream, "%c", c); + fclose(memstream); close(fd); unlink(fifo); @@ -209,6 +249,5 @@ return ERR_PROC_UNKNOWN; ret = waitchild(child, rc); } - } return ret; }
diff --git a/test/nolibc.c b/test/nolibc.c new file mode 100644 index 0000000..32817fb --- /dev/null +++ b/test/nolibc.c
@@ -0,0 +1,11 @@ +#include <sys/syscall.h> + +void _start() { + long _ret; + __asm__ volatile ( \ + "syscall" \ + : "=a" (_ret) \ + : "a" (__NR_exit), "D" (0) \ + : "rcx", "r11", "memory", "cc" \ + ); // exit(0); +} \ No newline at end of file
diff --git a/test/noop.c b/test/noop.c new file mode 100644 index 0000000..237c8ce --- /dev/null +++ b/test/noop.c
@@ -0,0 +1 @@ +int main() {}
diff --git a/unix.mk b/unix.mk index 239e309..163b6f2 100644 --- a/unix.mk +++ b/unix.mk
@@ -1,4 +1,5 @@ SOSRCS=src/unix/fsatraceso.c src/emit.c src/unix/proc.c +TESTSRCS=test/nolibc.c test/noop.c lib: fsatrace.so @@ -14,7 +15,22 @@ libinstall: fsatrace.so cp fsatrace.so $(INSTALLDIR) -cleanlib: - rm -f fsatrace.so $(patsubst %.c,%.d,$(SOSRCS)) $(patsubst %.c,%.os,$(SOSRCS)) +test/nolibc: test/nolibc.c + $(CC) -c $^ -o nolibc.o + ld -o $@ nolibc.o -e _start --include $(patsubst %.c,%.d,$(SOSRCS)) +nolibc-test: test/nolibc fsatrace + ./fsatrace ewrmdqt - -- ./test/nolibc; \ + test $$? -ne 0 || (echo "Error: fsatrace should fail when a program does not use libc." && false) + +test/noop: test/noop.o + $(CC) $(LDFLAGS) $(LDOBJS) $^ $(LDLIBS) -o $@ + +noop-test: test/noop fsatrace + ./fsatrace ewrmdqt - -- ./test/noop + +cleanlib: + rm -f fsatrace.so $(patsubst %.c,%.d,$(SOSRCS)) $(patsubst %.c,%.os,$(SOSRCS)) ./*.fifo + rm -f $(patsubst %.c,%,$(TESTSRCS)) $(patsubst %.c,%.d,$(TESTSRCS)) $(patsubst %.c,%.o,$(TESTSRCS)) + +-include $(patsubst %.c,%.d,$(SOSRCS) $(TESTSRCS))