Skip to content

Commit 29eb7e6

Browse files
jartjart
committed
Fix fork() regression on Windows
Recent optimizations to fork() introduced a regression, that could cause the subprocess to fail unexpectedly, when TlsAlloc() returns a different index. This is because we were burning the indexes into the displacement of x86 opcodes. So when fork() happened and the executable memory copied it would use the old index. Right now the way this is being solved is to not copy the executable on fork() and then re-apply code changes. If you need to be able to preserve self-modified code on fork, reach out and we can implement a better solution for you. This gets us unblocked quickly.
1 parent f71f61c commit 29eb7e6

File tree

11 files changed

+66
-34
lines changed

11 files changed

+66
-34
lines changed

‎libc/calls/winexec.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,8 @@ textwindows int IsWindowsExecutable(int64_t handle, const char16_t *path) {
8080
uint32_t got;
8181
BLOCK_SIGNALS;
8282
struct NtOverlapped overlap = {.hEvent = CreateEvent(0, 0, 0, 0)};
83-
ok = (ReadFile(handle, buf, 2, 0, &overlap) ||
83+
ok = overlap.hEvent &&
84+
(ReadFile(handle, buf, 2, 0, &overlap) ||
8485
GetLastError() == kNtErrorIoPending) &&
8586
GetOverlappedResult(handle, &overlap, &got, true);
8687
CloseHandle(overlap.hEvent);

‎libc/proc/describefds.c

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
#include "libc/fmt/itoa.h"
2323
#include "libc/intrin/fds.h"
2424
#include "libc/intrin/maps.h"
25-
#include "libc/intrin/strace.h"
2625
#include "libc/mem/mem.h"
2726
#include "libc/nt/files.h"
2827
#include "libc/nt/runtime.h"
@@ -68,7 +67,6 @@ textwindows void __undescribe_fds(int64_t hCreatorProcess,
6867
uint32_t dwExplicitHandleCount) {
6968
if (lpExplicitHandles) {
7069
for (uint32_t i = 0; i < dwExplicitHandleCount; ++i) {
71-
STRACE("close handle %lx %lx", hCreatorProcess, lpExplicitHandles[i]);
7270
DuplicateHandle(hCreatorProcess, lpExplicitHandles[i], 0, 0, 0, false,
7371
kNtDuplicateCloseSource);
7472
}
@@ -127,7 +125,6 @@ textwindows char *__describe_fds(const struct Fd *fds, size_t fdslen,
127125
for (uint32_t i = 0; i < 3; ++i)
128126
if (lpStartupInfo->stdiofds[i] == f->handle)
129127
lpStartupInfo->stdiofds[i] = handle;
130-
STRACE("added handle %lx", handle);
131128
handles[hi++] = handle;
132129

133130
// get shared memory handle for the file offset pointer
@@ -144,7 +141,6 @@ textwindows char *__describe_fds(const struct Fd *fds, size_t fdslen,
144141
__winerr();
145142
goto OnFailure;
146143
}
147-
STRACE("added handle %lx", shand);
148144
handles[hi++] = shand;
149145
}
150146

‎libc/proc/execve-nt.greg.c

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,8 @@ textwindows int sys_execve_nt(const char *program, char *const argv[],
7070

7171
// execve() needs to be @asyncsignalsafe
7272
sigset_t sigmask = __sig_block();
73-
STRACE("execve step #1");
7473
_pthread_mutex_lock(&__sig_worker_lock); // order matters
75-
STRACE("execve step #2");
76-
_pthread_lock(); // order matters
77-
STRACE("execve step #3");
74+
_pthread_lock(); // order matters
7875

7976
// new process should be a child of our parent
8077
int64_t hParentProcess =
@@ -83,8 +80,6 @@ textwindows int sys_execve_nt(const char *program, char *const argv[],
8380
sys_getppid_nt_win32)
8481
: 0;
8582

86-
STRACE("execve step #4");
87-
8883
// inherit pid
8984
char pidvar[11 + 21];
9085
FormatUint64(stpcpy(pidvar, "_COSMO_PID="), __pid);
@@ -103,8 +98,6 @@ textwindows int sys_execve_nt(const char *program, char *const argv[],
10398
setenv("_COSMO_PPID", ppidvar, true);
10499
}
105100

106-
STRACE("execve step #5");
107-
108101
// define stdio handles for the spawned subprocess
109102
struct NtStartupInfo si = {
110103
.cb = sizeof(struct NtStartupInfo),
@@ -118,8 +111,6 @@ textwindows int sys_execve_nt(const char *program, char *const argv[],
118111
}
119112
}
120113

121-
STRACE("execve step #6");
122-
123114
// which process is responsible for spawning the child?
124115
int64_t hCreatorProcess;
125116
if (hParentProcess) {
@@ -140,25 +131,19 @@ textwindows int sys_execve_nt(const char *program, char *const argv[],
140131
return -1;
141132
}
142133

143-
STRACE("execve step #7");
144-
145134
// inherit pending signals
146135
atomic_fetch_or_explicit(
147136
__sig.process,
148137
atomic_load_explicit(&__get_tls()->tib_sigpending, memory_order_acquire),
149138
memory_order_release);
150139

151-
STRACE("execve step #8");
152-
153140
// launch the process
154141
struct NtProcessInformation pi;
155142
int rc = ntspawn(&(struct NtSpawnArgs){
156143
AT_FDCWD, program, argv, envp,
157144
(char *[]){fdspec, maskvar, pidvar, ppidvar, 0}, 0, 0, hCreatorProcess,
158145
lpExplicitHandles, dwExplicitHandleCount, &si, &pi});
159-
STRACE("execve step #9");
160146
__undescribe_fds(hCreatorProcess, lpExplicitHandles, dwExplicitHandleCount);
161-
STRACE("execve step #10");
162147
if (rc == -1) {
163148
free(fdspec);
164149
if (hParentProcess)
@@ -177,11 +162,9 @@ textwindows int sys_execve_nt(const char *program, char *const argv[],
177162
int64_t handle;
178163
if (DuplicateHandle(GetCurrentProcess(), pi.hProcess, hParentProcess,
179164
&handle, 0, false, kNtDuplicateSameAccess)) {
180-
STRACE("execve step #11");
181165
unassert(!(handle & 0xFFFFFFFFFF000000));
182166
__imp_TerminateProcess(-1, 0x23000000u | handle);
183167
} else {
184-
STRACE("execve step #12");
185168
// TODO(jart): Why does `make loc` print this?
186169
// kprintf("DuplicateHandle failed w/ %d\n", GetLastError());
187170
__imp_TerminateProcess(-1, ECHILD);

‎libc/proc/fork-nt.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ textwindows static void sys_fork_nt_child(void) {
9090
// setup runtime
9191
__klog_handle = 0;
9292
__tls_index = __imp_TlsAlloc();
93+
__morph_tls();
9394
__set_tls_win32(__winmain_tib);
9495
__tls_enabled = true;
9596

@@ -241,6 +242,9 @@ textwindows static int sys_fork_nt_parent(uint32_t dwCreationFlags) {
241242
}
242243
if ((map->flags & MAP_NOFORK) && (map->flags & MAP_TYPE) == MAP_FILE) {
243244
// portable executable segment
245+
if (map->prot & PROT_EXEC)
246+
// TODO(jart): write a __remorph_tls() function
247+
continue;
244248
if (!(map->prot & PROT_WRITE)) {
245249
uint32_t child_old_protect;
246250
ok = ok && !!VirtualProtectEx(procinfo.hProcess, map->addr, allocsize,

‎libc/proc/fork.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,11 @@ int _fork(uint32_t dwCreationFlags) {
298298
}
299299
}
300300

301+
// reactivate ftrace
302+
/* if (ftrace_stackdigs) */
303+
/* if (_weaken(ftrace_install)) */
304+
/* _weaken(ftrace_install)(); */
305+
301306
STRACE("fork() → 0 (child of %d)", ppid_cosmo);
302307
} else {
303308
// this is the parent process

‎libc/proc/wait4-nt.c

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -131,14 +131,18 @@ textwindows static int __proc_wait(int pid, int *wstatus, int options,
131131
// perform blocking operation
132132
uint32_t wi;
133133
uintptr_t event;
134-
struct PosixThread *pt = _pthread_self();
135-
pt->pt_blkmask = waitmask;
136-
pt->pt_event = event = CreateEvent(0, 0, 0, 0);
137-
atomic_store_explicit(&pt->pt_blocker, PT_BLOCKER_EVENT,
138-
memory_order_release);
139-
wi = WaitForMultipleObjects(2, (intptr_t[2]){hWaitObject, event}, 0, -1u);
140-
atomic_store_explicit(&pt->pt_blocker, 0, memory_order_release);
141-
CloseHandle(event);
134+
if ((event = CreateEvent(0, 0, 0, 0))) {
135+
struct PosixThread *pt = _pthread_self();
136+
pt->pt_event = event;
137+
pt->pt_blkmask = waitmask;
138+
atomic_store_explicit(&pt->pt_blocker, PT_BLOCKER_EVENT,
139+
memory_order_release);
140+
wi = WaitForMultipleObjects(2, (intptr_t[2]){hWaitObject, event}, 0, -1u);
141+
atomic_store_explicit(&pt->pt_blocker, 0, memory_order_release);
142+
CloseHandle(event);
143+
} else {
144+
wi = -1u;
145+
}
142146

143147
// log warning if handle unexpectedly closed
144148
if (wi & kNtWaitAbandoned) {

‎libc/sock/sendfile.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,10 @@ textwindows dontinline static ssize_t sys_sendfile_nt(
9292
}
9393
struct NtOverlapped ov = {.hEvent = WSACreateEvent(), .Pointer = offset};
9494
cosmo_once(&g_transmitfile.once, transmitfile_init);
95-
if (g_transmitfile.lpTransmitFile(oh, ih, uptobytes, 0, &ov, 0, 0) ||
96-
WSAGetLastError() == kNtErrorIoPending ||
97-
WSAGetLastError() == WSAEINPROGRESS) {
95+
if (ov.hEvent &&
96+
(g_transmitfile.lpTransmitFile(oh, ih, uptobytes, 0, &ov, 0, 0) ||
97+
WSAGetLastError() == kNtErrorIoPending ||
98+
WSAGetLastError() == WSAEINPROGRESS)) {
9899
if (WSAGetOverlappedResult(oh, &ov, &uptobytes, true, &flags)) {
99100
rc = uptobytes;
100101
if (opt_in_out_inoffset) {

‎test/libc/proc/BUILD.mk

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ TEST_LIBC_PROC_DIRECTDEPS = \
3131
LIBC_NT_KERNEL32 \
3232
LIBC_PROC \
3333
LIBC_RUNTIME \
34+
LIBC_LOG \
3435
LIBC_STDIO \
3536
LIBC_STR \
3637
LIBC_SYSV \
@@ -90,6 +91,7 @@ o/$(MODE)/test/libc/proc/execve_test.dbg: \
9091
o/$(MODE)/test/libc/proc/execve_test.o \
9192
o/$(MODE)/test/libc/calls/life-nomod.zip.o \
9293
o/$(MODE)/test/libc/proc/execve_test_prog1.zip.o \
94+
o/$(MODE)/test/libc/proc/execve_test_prog2.zip.o \
9395
o/$(MODE)/test/libc/mem/prog/life.elf.zip.o \
9496
o/$(MODE)/test/libc/mem/prog/sock.elf.zip.o \
9597
o/$(MODE)/test/libc/proc/proc.pkg \
@@ -119,6 +121,7 @@ o/$(MODE)/test/libc/proc/life.dbg: \
119121

120122
o/$(MODE)/test/libc/proc/life.zip.o \
121123
o/$(MODE)/test/libc/proc/execve_test_prog1.zip.o \
124+
o/$(MODE)/test/libc/proc/execve_test_prog2.zip.o \
122125
o/$(MODE)/test/libc/proc/life-pe.zip.o: private \
123126
ZIPOBJ_FLAGS += \
124127
-B

‎test/libc/proc/execve_test.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ TEST(execve, testArgPassing) {
5353
char ibuf[12], buf[8];
5454
const char *prog = "./execve_test_prog1";
5555
testlib_extract("/zip/execve_test_prog1", prog, 0755);
56+
testlib_extract("/zip/execve_test_prog2", "execve_test_prog2", 0755);
5657
for (i = 0; i < N; ++i) {
5758
FormatInt32(ibuf, i);
5859
GenBuf(buf, i);

‎test/libc/proc/execve_test_prog1.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
╚─────────────────────────────────────────────────────────────────────────────*/
1919
#include "libc/calls/calls.h"
2020
#include "libc/fmt/conv.h"
21+
#include "libc/runtime/runtime.h"
2122
#include "libc/str/str.h"
2223

2324
void GenBuf(char buf[8], int x) {
@@ -40,5 +41,15 @@ int main(int argc, char *argv[]) {
4041
tinyprint(2, "error: buf check failed\n", NULL);
4142
return 10;
4243
}
44+
const char *prog = "./execve_test_prog2";
45+
if (!fork()) {
46+
execl(prog, prog, NULL);
47+
_Exit(127);
48+
}
49+
int ws;
50+
if (wait(&ws) == -1)
51+
return 30;
52+
if (ws)
53+
return 31;
4354
return 0;
4455
}

0 commit comments

Comments
 (0)