Tracing multiple subprocesses doesn't work on Windows

[ndmitchell]

Tracing multiple subprocesses doesn't work on Windows

[ndmitchell]

Concretely:

diff --git a/src/win/hooks.c b/src/win/hooks.c
index eb1e66a..529612b 100644
--- a/src/win/hooks.c
+++ b/src/win/hooks.c
@@ -3,6 +3,8 @@
 #endif
 #include <winternl.h>
 #include <limits.h>
+#include <processthreadsapi.h>
+
 
 #undef ASSERT
 #include "dbg.h"
@@ -213,7 +215,7 @@ static NTSTATUS NTAPI hNtQueryFullAttributesFile(POBJECT_ATTRIBUTES oa, PFILE_NE
 static NTSTATUS NTAPI hNtResumeThread(HANDLE th, PULONG sc) {
 	NTSTATUS r;
 	D;
-	if (!patchInstalled())
+	if (!patchInstalled(GetProcessIdOfThread(th)))
 		injectThread(th);
 	r = oNtResumeThread(th, sc);
 	return r;
diff --git a/src/win/patch.c b/src/win/patch.c
index 6fdc26d..7b63f0a 100644
--- a/src/win/patch.c
+++ b/src/win/patch.c
@@ -95,12 +95,12 @@ void patchInstall(void *orig, void *hook, void **preal, const char *nm) {
 	dbg("modules patched\n");
 }
 
-int patchInstalled() {
+int patchInstalled(DWORD old) {
 	int ret;
 	ASSERT(s_hooked);
-	ret = (int)(intptr_t)TlsGetValue(s_hooked);
-	CHK(TlsSetValue(s_hooked, (void *)1));
-	return ret;
+	ret = (DWORD)(intptr_t)TlsGetValue(s_hooked);
+	CHK(TlsSetValue(s_hooked, (void *)old));
+	return ret == old;
 }
 
 void patchInit() {
diff --git a/src/win/patch.h b/src/win/patch.h
index a9628ec..64ba3d0 100644
--- a/src/win/patch.h
+++ b/src/win/patch.h
@@ -1,5 +1,4 @@
 void patchInit();
 void patchTerm();
 void patchInstall(void *orig, void *hook, void **preal, const char *nm);
-int patchInstalled();
-
+int patchInstalled(DWORD old);

[jacereda]

Looks certainly better than what we have now, but I guess it isn't bulletproof either. A different thread could call ResumeThread() and we could end up with 2 patches installed.
I guess a 4 KB bit mask indexed by the lower bits of (pid/4) could be quite robust.

[jacereda]

Here's what I thought could be a solution, but it didn't pass the tests.

7abb16b

Unfortunately I can't check with a windows box yet.

[ndmitchell]

I'll try tomorrow on a real windows box. Why do the pid byte masking and not just have an actual array with the values seen?

[jacereda]

OK, I think I found the bug. Please, check if the fix-23 branch works for you.

[jacereda]

Yeah, it's kind of silly to worry about that table size considering the amount of memory I reserve for the logs :)

[jacereda]

Oh, you mean doing a lookup in the array? Yes, that would probably be a better option. Feel free to change it.

[ndmitchell]

It worked for me, thanks a lot. Given it's currently working I'd probably just leave it at that.