Rework API for inline hooking

This both simplifies and complicates things, but probably hopefully
maybe simplifies things overall. Certainly in cases like the L4D1 demo
thing where there's 3 inline hooks at once, it seems simpler to be able
to batch the fallible stuff to avoid rollbacks. In cases where you only
need one hook, it's a bit more verbose, but what can you do.

Thanks bill for discussing this with me pretty exhaustively and giving a
lot of good input.

I think both of us still kind of hate it actually.

12 files changed, 232 insertions, 133 deletions

diff --git a/src/ac.c b/src/ac.c
index 32309c5..ea3c08a 100644
--- a/src/ac.c
+++ b/src/ac.c
@@ -380,12 +380,9 @@ HANDLE_EVENT(PluginUnloaded) {
 
 INIT {
 	if_cold (!find_Key_Event()) return FEAT_INCOMPAT;
-	orig_Key_Event = (Key_Event_func)hook_inline((void *)orig_Key_Event,
-			(void *)&hook_Key_Event);
-	if_cold (!orig_Key_Event) {
-		errmsg_errorsys("couldn't hook Key_Event function");
-		return FEAT_FAIL;
-	}
+	struct hook_inline_featsetup_ret h = hook_inline_featsetup(
+			(void *)orig_Key_Event, (void **)&orig_Key_Event, "Key_Event");
+	if_cold (h.err) return h.err;
 
 #ifdef _WIN32
 	keybox = VirtualAlloc(0, 4096, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE);
@@ -423,6 +420,7 @@ INIT {
 		// run of bytes
 		memcpy(keybox->lbpub, lbpubkeys[LBPK_L4D], 32);
 	}
+	hook_inline_commit(h.prologue, (void *)hook_Key_Event);
 	return FEAT_OK;
 
 #ifdef _WIN32
@@ -430,7 +428,6 @@ e:	VirtualFree(keybox, 4096, MEM_RELEASE);
 #else
 e:	munmap(keybox, 4096);
 #endif
-	unhook_inline((void *)orig_Key_Event);
 	return FEAT_FAIL;
 }
 
diff --git a/src/fastfwd.c b/src/fastfwd.c
index 73db41e..1b7588d 100644
--- a/src/fastfwd.c
+++ b/src/fastfwd.c
@@ -232,19 +232,18 @@ INIT {
 		return FEAT_INCOMPAT;
 	}
 	if_cold (!(func = find_floatcall(func, 1, "_Host_RunFrame"))) {
-		errmsg_errorx("couldn't find _Host_RunFrame");
+		errmsg_errorx("couldn't find _Host_RunFrame function");
 		return FEAT_INCOMPAT;
 	}
 	if_cold (!find_Host_AccumulateTime(func)) {
-		errmsg_errorx("couldn't find Host_AccumulateTime");
+		errmsg_errorx("couldn't find Host_AccumulateTime function");
 		return FEAT_INCOMPAT;
 	}
-	orig_Host_AccumulateTime = (Host_AccumulateTime_func)hook_inline(
-			(void *)orig_Host_AccumulateTime, (void *)hook_Host_AccumulateTime);
-	if_cold (!orig_Host_AccumulateTime) {
-		errmsg_errorsys("couldn't hook Host_AccumulateTime function");
-		return FEAT_FAIL;
-	}
+	struct hook_inline_featsetup_ret h = hook_inline_featsetup(
+			(void *)orig_Host_AccumulateTime, (void **)&orig_Host_AccumulateTime,
+			"Host_AccumulateTime");
+	if_cold (h.err) return h.err;
+	hook_inline_commit(h.prologue, (void *)&hook_Host_AccumulateTime);
 	return FEAT_OK;
 }
 
diff --git a/src/fov.c b/src/fov.c
index 967d783..bfef858 100644
--- a/src/fov.c
+++ b/src/fov.c
@@ -100,12 +100,12 @@ INIT {
 		errmsg_errorx("couldn't find SetDefaultFOV function");
 		return FEAT_INCOMPAT;
 	}
-	orig_SetDefaultFOV = (SetDefaultFOV_func)hook_inline(
-			(void *)orig_SetDefaultFOV, (void *)&hook_SetDefaultFOV);
-	if_cold (!orig_SetDefaultFOV) {
-		errmsg_errorsys("couldn't hook SetDefaultFOV function");
-		return FEAT_FAIL;
-	}
+
+	struct hook_inline_featsetup_ret h = hook_inline_featsetup(
+			(void *)orig_SetDefaultFOV, (void **)&orig_SetDefaultFOV,
+			"SetDefaultFov");
+	if_cold (h.err) return h.err;
+	hook_inline_commit(h.prologue, (void *)&hook_SetDefaultFOV);
 
 	// we might not be using our cvar but simpler to do this unconditionally
 	fov_desired->cb = &fovcb;
diff --git a/src/hook.c b/src/hook.c
index b6ca703..a1504da 100644
--- a/src/hook.c
+++ b/src/hook.c
@@ -17,7 +17,7 @@
 
 #include <string.h>
 
-#include "con_.h"
+#include "hook.h"
 #include "intdefs.h"
 #include "langext.h"
 #include "mem.h"
@@ -35,20 +35,13 @@ __declspec(dllimport) int __stdcall FlushInstructionCache(
 // Almost certainly breaks in some weird cases. Oh well! Most of the time,
 // vtable hooking is more reliable, this is only for, uh, emergencies.
 
-#if defined(__GNUC__) || defined(__clang__)
-__attribute__((aligned(4096)))
-#elif defined(_MSC_VER)
-__declspec(align(4096))
-#else
-#error no way to align stuff!
-#endif
-static uchar trampolines[4096];
-static uchar *nexttrampoline = trampolines;
+static _Alignas(4096) uchar trampolines[4096];
+static uchar *curtrampoline = trampolines;
 
 bool hook_init() {
 	// PE doesn't support rwx sections, not sure about ELF. Meh, just set it
 	// here instead.
-	return os_mprot(trampolines, sizeof(trampolines), PAGE_EXECUTE_READWRITE);
+	return os_mprot(trampolines, 4096, PAGE_EXECUTE_READWRITE);
 }
 
 static inline void iflush(void *p, int len) {
@@ -63,51 +56,59 @@ static inline void iflush(void *p, int len) {
 #endif
 }
 
-void *hook_inline(void *func_, void *target) {
-	uchar *func = func_;
+struct hook_inline_prep_ret hook_inline_prep(void *func, void **trampoline) {
+	uchar *p = func;
 	// dumb hack: if we hit some thunk that immediately jumps elsewhere (which
 	// seems common for win32 API functions), hook the underlying thing instead.
-	while (*func == X86_JMPIW) func += mem_loads32(func + 1) + 5;
-	if_cold (!os_mprot(func, 5, PAGE_EXECUTE_READWRITE)) return 0;
+	// later: that dumb hack has now ended up having implications in the
+	// redesign of the entire API. :-)
+	while (*p == X86_JMPIW) p += mem_loads32(p + 1) + 5;
+	void *prologue = p;
 	int len = 0;
 	for (;;) {
-		// FIXME: these cases may result in somewhat dodgy error messaging. They
-		// shouldn't happen anyway though. Maybe if we're confident we just
-		// compile 'em out of release builds some day, but that sounds a little
-		// scary. For now preferring confusing messages over crashes, I guess.
-		if (func[len] == X86_CALL) {
-			con_warn("hook_inline: can't trampoline call instructions\n");
-			return 0;
+		if_cold (p[len] == X86_CALL) {
+			return (struct hook_inline_prep_ret){
+				0, "can't trampoline call instructions"
+			};
 		}
-		int ilen = x86_len(func + len);
-		if (ilen == -1) {
-			con_warn("hook_inline: unknown or invalid instruction\n");
-			return 0;
+		int ilen = x86_len(p + len);
+		if_cold (ilen == -1) {
+			return (struct hook_inline_prep_ret){
+				0, "unknown or invalid instruction"
+			};
 		}
 		len += ilen;
-		if (len >= 5) break;
-		if (func[len] == X86_JMPIW) {
-			con_warn("hook_inline: can't trampoline jmp instructions\n");
-			return 0;
+		if (len >= 5) {
+			// we should have statically made trampoline buffer size big enough
+			assume(curtrampoline - trampolines < sizeof(trampolines) - len - 6);
+			*curtrampoline = len; // stuff length in there for quick unhooking
+			uchar *newtrampoline = curtrampoline + 1;
+			curtrampoline += len + 6;
+			memcpy(newtrampoline, p, len);
+			newtrampoline[len] = X86_JMPIW;
+			u32 diff = p - (newtrampoline + 5); // goto the continuation
+			memcpy(newtrampoline + len + 1, &diff, 4);
+			*trampoline = newtrampoline;
+			return (struct hook_inline_prep_ret){prologue, 0};
+		}
+		if_cold (p[len] == X86_JMPIW) {
+			return (struct hook_inline_prep_ret){
+				0, "can't trampoline jump instructions"
+			};
 		}
 	}
-	// for simplicity, just bump alloc the trampoline. no need to free anyway
-	if_cold (nexttrampoline - trampolines > sizeof(trampolines) - len - 6) {
-		con_warn("hook_inline: out of trampoline space\n");
-		return 0;
-	}
-	uchar *trampoline = nexttrampoline;
-	nexttrampoline += len + 6; // NOT thread-safe. we don't need that anyway!
-	*trampoline++ = len; // stick length in front for quicker unhooking
-	memcpy(trampoline, func, len);
-	trampoline[len] = X86_JMPIW;
-	uint diff = func - (trampoline + 5); // goto the continuation
-	memcpy(trampoline + len + 1, &diff, 4);
-	diff = (uchar *)target - (func + 5); // goto the hook target
-	func[0] = X86_JMPIW;
-	memcpy(func + 1, &diff, 4);
-	iflush(func, 5);
-	return trampoline;
+}
+
+bool hook_inline_mprot(void *prologue) {
+	return os_mprot(prologue, 5, PAGE_EXECUTE_READWRITE);
+}
+
+void hook_inline_commit(void *restrict prologue, void *restrict target) {
+	uchar *p = prologue;
+	u32 diff = (uchar *)target - (p + 5); // goto the hook target
+	p[0] = X86_JMPIW;
+	memcpy(p + 1, &diff, 4);
+	iflush(p, 5);
 }
 
 void unhook_inline(void *orig) {
diff --git a/src/hook.h b/src/hook.h
index 09af156..0aeae73 100644
--- a/src/hook.h
+++ b/src/hook.h
@@ -18,6 +18,9 @@
 #define INC_HOOK_H
 
 #include "intdefs.h"
+#include "errmsg.h"
+#include "feature.h"
+#include "langext.h"
 
 bool hook_init();
 
@@ -39,17 +42,110 @@ static inline void unhook_vtable(void **vtable, usize off, void *orig) {
 }
 
 /*
- * Returns a trampoline pointer, or null if hooking failed. Unlike hook_vtable,
- * handles memory protection for you.
+ * Finds the correct function prologue location to install an inline hook, and
+ * tries to initialise a trampoline with sufficient instructions and a jump back
+ * to enable calling the original function.
  *
- * This function is not remotely thread-safe, and should never be called from
- * any thread besides the main one nor be used to hook anything that gets called
- * from other threads.
+ * This is a low-level API and in most cases, if doing hooking from inside a
+ * plugin feature, the hook_inline_featsetup() function should be used instead.
+ * It automatically performs conventional error logging for both this step and
+ * the hook_inline_mprot() call below, and returns error codes that are
+ * convenient for use in a feature INIT function.
+ *
+ * When this function succeeds, the returned struct will have the prologue
+ * member set to the prologue or starting point of the hooked function (which is
+ * not always the same as the original function pointer). The trampoline
+ * parameter, being a pointer-to-pointer, is an output parameter to which a
+ * trampoline pointer will be written. The trampoline is a small run of
+ * instructions from the original function, followed by a jump back to it,
+ * allowing the original to be seamlessly called from a hook.
+ *
+ * In practically rare cases, this function will fail due to unsupported
+ * instructions in the function prologue. In such instances, the returned struct
+ * will have a null prologue, and the second member err, will point to a
+ * null-terminated string for error logging. In this case, the trampoline
+ * pointer will remain untouched.
+ */
+struct hook_inline_prep_ret {
+	void *prologue;
+	const char *err;
+} hook_inline_prep(void *func, void **trampoline);
+
+/*
+ * This is a small helper function to make the memory page containing a
+ * function's prologue writable, allowing an inline hook to be inserted with
+ * hook_inline_commit().
+ *
+ * This is a low-level API and in most cases, if doing hooking from inside a
+ * plugin feature, the hook_inline_featsetup() function should be used instead.
+ * It automatically performs conventional error logging for both this step and
+ * the prior hook_inline_prep() call documented above, and returns error codes
+ * that are convenient for use in a feature INIT function.
+ *
+ * After using hook_inline_prep() to obtain the prologue and an appropriate
+ * trampoline, call this to unlock the prologue, and then use
+ * hook_inline_commit() to finalise the hook. In the event that multiple
+ * functions need to be hooked at once, the commit calls can be batched up at
+ * the end, removing the need for rollbacks since commitment is guaranteed to
+ * succeed after all setup is complete.
+ *
+ * This function returns true on success, or false if a failure occurs at the
+ * level of the OS memory protection API. os_lasterror() or errmsg_*sys() can be
+ * used to report such an error.
  */
-void *hook_inline(void *func, void *target);
+bool hook_inline_mprot(void *func);
+
+/*
+ * Finalises an inline hook set up using the hook_inline_prep() and
+ * hook_inline_mprot() functions above (or the hook_inline_featsetup() helper
+ * function below). prologue must be the prologue obtained via the
+ * aforementioned functons and target must be the function that will be jumped
+ * to in place of the original. It is very important that these functions are
+ * ABI-compatible lest obvious bad things happen.
+ *
+ * The resulting hook can be removed later by calling unhook_inline().
+ */
+void hook_inline_commit(void *restrict prologue, void *restrict target);
+
+/*
+ * This is a helper specifically for use in feature INIT code. It doesn't make
+ * much sense to call it elsewhere.
+ *
+ * Combines the functionality of the hook_inline_prep() and hook_inline_mprot()
+ * functions above, logs to the console on error automatically in a conventional
+ * format, and returns an error status that can be propagated straight from a
+ * feature INIT function.
+ *
+ * func must point to the original function to be hooked, orig must point to
+ * your trampoline pointer (which can in turn be used to call the original
+ * function indirectly from within your hook or elsewhere), and fname should be
+ * the name of the function for error logging purposes.
+ *
+ * If the err member of the returned struct is nonzero, simply return it as-is.
+ * Otherwise, the prologue member will contain the prologue pointer to pass to
+ * hook_inline_commit() to finalise the hook.
+ */
+static inline struct hook_inline_featsetup_ret {
+	void *prologue;
+	int err;
+} hook_inline_featsetup(void *func, void **orig, const char *fname) {
+	void *trampoline;
+	struct hook_inline_prep_ret prep = hook_inline_prep(func, &trampoline);
+	if_cold (prep.err) {
+		errmsg_warnx("couldn't hook %s function: %s", fname, prep.err);
+		return (struct hook_inline_featsetup_ret){0, FEAT_INCOMPAT};
+	}
+	if_cold (!hook_inline_mprot(prep.prologue)) {
+		errmsg_errorsys("couldn't hook %s function: %s", fname,
+				"couldn't make prologue writable");
+		return (struct hook_inline_featsetup_ret){0, FEAT_FAIL};
+	}
+	*orig = trampoline;
+	return (struct hook_inline_featsetup_ret){prep.prologue, 0};
+}
 
 /*
- * Reverts the function to its original unhooked state. Takes the pointer to the
+ * Reverts a function to its original unhooked state. Takes the pointer to the
  * callable "original" function, i.e. the trampoline, NOT the initial function
  * pointer from before hooking.
  */
diff --git a/src/l4d1democompat.c b/src/l4d1democompat.c
index c4dab0c..6754d76 100644
--- a/src/l4d1democompat.c
+++ b/src/l4d1democompat.c
@@ -150,29 +150,22 @@ INIT {
 		return FEAT_INCOMPAT;
 	}
 	gameversion = orig_GetHostVersion();
-	orig_GetHostVersion = (GetHostVersion_func)hook_inline(
-			(void *)orig_GetHostVersion, (void *)&hook_GetHostVersion);
-	if (!orig_GetHostVersion) {
-		errmsg_errorsys("couldn't hook GetHostVersion");
-		return FEAT_FAIL;
-	}
-	orig_ReadDemoHeader = (ReadDemoHeader_func)hook_inline(
-			(void *)orig_ReadDemoHeader, (void *)&hook_ReadDemoHeader);
-	if (!orig_ReadDemoHeader) {
-		errmsg_errorsys("couldn't hook ReadDemoHeader");
-		goto e1;
-	}
-	ReadDemoHeader_midpoint = hook_inline(
-			(void *)ReadDemoHeader_midpoint, (void *)&hook_midpoint);
-	if (!ReadDemoHeader_midpoint) {
-		errmsg_errorsys("couldn't hook ReadDemoHeader midpoint");
-		goto e2;
-	}
+	struct hook_inline_featsetup_ret h1 = hook_inline_featsetup(
+			(void *)orig_GetHostVersion, (void **)&orig_GetHostVersion,
+			"GetHostVersion");
+	if_cold (h1.err) return h1.err;
+	struct hook_inline_featsetup_ret h2 = hook_inline_featsetup(
+			(void *)orig_ReadDemoHeader, (void **)&orig_ReadDemoHeader,
+			"ReadDemoHeader");
+	if_cold (h2.err) return h2.err;
+	struct hook_inline_featsetup_ret h3 = hook_inline_featsetup(
+			ReadDemoHeader_midpoint, (void **)&hook_midpoint,
+			"ReadDemoHeader midpoint");
+	if_cold (h3.err) return h3.err;
+	hook_inline_commit(h1.prologue, (void *)&hook_GetHostVersion);
+	hook_inline_commit(h2.prologue, (void *)&hook_ReadDemoHeader);
+	hook_inline_commit(h3.prologue, (void *)&hook_midpoint);
 	return FEAT_OK;
-
-e2:	unhook_inline((void *)orig_ReadDemoHeader);
-e1:	unhook_inline((void *)orig_GetHostVersion);
-	return FEAT_FAIL;
 }
 
 END {
diff --git a/src/l4daddon.c b/src/l4daddon.c
index 0609459..8e61e05 100644
--- a/src/l4daddon.c
+++ b/src/l4daddon.c
@@ -241,12 +241,11 @@ INIT {
 		return FEAT_INCOMPAT;
 	}
 	try_fix_broken_addon_check();
-	orig_FS_MAFAS = (FS_MAFAS_func)hook_inline((void *)orig_FS_MAFAS,
-			(void *)&hook_FS_MAFAS);
-	if_cold (!orig_FS_MAFAS) {
-		errmsg_errorsys("couldn't hook FileSystem_ManageAddonsForActiveSession");
-		return FEAT_FAIL;
-	}
+	struct hook_inline_featsetup_ret h = hook_inline_featsetup(
+			(void *)orig_FS_MAFAS, (void **)&orig_FS_MAFAS,
+			"FileSystem_ManageAddonsForActiveSession");
+	if_cold (h.err) return h.err;
+	hook_inline_commit(h.prologue, (void *)&hook_FS_MAFAS);
 	return FEAT_OK;
 }
 
diff --git a/src/l4dreset.c b/src/l4dreset.c
index 2de75cd..84daa65 100644
--- a/src/l4dreset.c
+++ b/src/l4dreset.c
@@ -549,8 +549,11 @@ INIT {
 			errmsg_errorx("couldn't find UnfreezeTeam function");
 			return FEAT_INCOMPAT;
 		}
-		orig_UnfreezeTeam = (UnfreezeTeam_func)hook_inline(
-				(void *)orig_UnfreezeTeam, (void *)&hook_UnfreezeTeam);
+		struct hook_inline_featsetup_ret h = hook_inline_featsetup(
+			(void *)orig_UnfreezeTeam, (void **)&orig_UnfreezeTeam,
+			"UnfreezeTeam");
+		if_cold (h.err) return h.err;
+		hook_inline_commit(h.prologue, (void *)&hook_UnfreezeTeam);
 	}
 #endif
 	// Only try cooldown stuff for L4D2, since L4D1 always had unlimited votes.
diff --git a/src/portalcolours.c b/src/portalcolours.c
index 7779cb3..24224c1 100644
--- a/src/portalcolours.c
+++ b/src/portalcolours.c
@@ -87,6 +87,9 @@ static bool find_UTIL_Portal_Color(void *base) {
 	orig_UTIL_Portal_Color = (UTIL_Portal_Color_func)mem_offset(base, 0x1AA810);
 	if (!memcmp((void *)orig_UTIL_Portal_Color, x, sizeof(x))) return true;
 	// SteamPipe (7197370) - almost sure to break in a later update!
+	// TODO(compat): this has indeed been broken for ages.
+	// TODO(compat): we also still don't have 4104. really need to do this
+	// properly some time soon, it seems.
 	static const uchar y[] = HEXBYTES(55, 8B, EC, 8B, 45, 0C, 83, E8, 00, 74,
 			24, 48, 74, 16, 48, 8B, 45, 08, 74, 08, C7, 00, FF, FF);
 	orig_UTIL_Portal_Color = (UTIL_Portal_Color_func)mem_offset(base, 0x234C00);
@@ -100,12 +103,11 @@ INIT {
 		errmsg_errorx("couldn't find UTIL_Portal_Color");
 		return FEAT_INCOMPAT;
 	}
-	orig_UTIL_Portal_Color = (UTIL_Portal_Color_func)hook_inline(
-			(void *)orig_UTIL_Portal_Color, (void *)&hook_UTIL_Portal_Color);
-	if_cold (!orig_UTIL_Portal_Color) {
-		errmsg_errorsys("couldn't hook UTIL_Portal_Color");
-		return FEAT_INCOMPAT;
-	}
+	struct hook_inline_featsetup_ret h = hook_inline_featsetup(
+			(void *)orig_UTIL_Portal_Color, (void **)&orig_UTIL_Portal_Color,
+			"UTIL_Portal_Color");
+	if_cold (h.err) return h.err;
+	hook_inline_commit(h.prologue, (void *)&hook_UTIL_Portal_Color);
 	sst_portal_colour0->cb = &colourcb;
 	sst_portal_colour1->cb = &colourcb;
 	sst_portal_colour2->cb = &colourcb;
diff --git a/src/rinput.c b/src/rinput.c
index 850efee..f4e498e 100644
--- a/src/rinput.c
+++ b/src/rinput.c
@@ -180,22 +180,17 @@ INIT {
 		goto ok;
 	}
 
-	orig_GetCursorPos = (GetCursorPos_func)hook_inline((void *)&GetCursorPos,
-			(void *)&hook_GetCursorPos);
-	if_cold (!orig_GetCursorPos) {
-		errmsg_errorsys("couldn't hook %s", "GetCursorPos");
-		goto e0;
-	}
-	orig_SetCursorPos = (SetCursorPos_func)hook_inline((void *)&SetCursorPos,
-			(void *)&hook_SetCursorPos);
-	if_cold (!orig_SetCursorPos) {
-		errmsg_errorsys("couldn't hook %s", "SetCursorPos");
-		goto e1;
-	}
+	int err;
+	struct hook_inline_featsetup_ret h1 = hook_inline_featsetup(
+			(void *)GetCursorPos, (void **)&orig_GetCursorPos, "GetCursorPos");
+	if_cold (err = h1.err) goto e0;
+	struct hook_inline_featsetup_ret h2 = hook_inline_featsetup(
+			(void *)SetCursorPos, (void **)&orig_SetCursorPos, "SetCursorPos");
+	if_cold (err = h2.err) goto e0;
 	inwin = CreateWindowExW(0, L"RInput", L"RInput", 0, 0, 0, 0, 0, 0, 0, 0, 0);
 	if_cold (!inwin) {
 		errmsg_errorsys("couldn't create input window");
-		goto e2;
+		goto e0;
 	}
 	RAWINPUTDEVICE rd = {
 		.hwndTarget = inwin,
@@ -204,18 +199,19 @@ INIT {
 	};
 	if_cold (!RegisterRawInputDevices(&rd, 1, sizeof(rd))) {
 		errmsg_errorsys("couldn't create raw mouse device");
-		goto e3;
+		err = FEAT_FAIL;
+		goto e1;
 	}
+	hook_inline_commit(h1.prologue, (void *)&hook_GetCursorPos);
+	hook_inline_commit(h2.prologue, (void *)&hook_SetCursorPos);
 
 ok:	m_rawinput->base.flags &= ~CON_HIDDEN;
 	sst_mouse_factor->base.flags &= ~CON_HIDDEN;
 	return FEAT_OK;
 
-e3:	DestroyWindow(inwin);
-e2:	unhook_inline((void *)orig_SetCursorPos);
-e1:	unhook_inline((void *)orig_GetCursorPos);
+e1:	DestroyWindow(inwin);
 e0:	UnregisterClassW(L"RInput", 0);
-	return FEAT_FAIL;
+	return err;
 }
 
 END {
diff --git a/src/sst.c b/src/sst.c
index a60ad36..bd73d44 100644
--- a/src/sst.c
+++ b/src/sst.c
@@ -283,6 +283,7 @@ static const char *updatenotes = "\
 * Removed multiplayer chat rate limit in L4D series and Portal 2\n\
 * Made L4D1 demo playback backwards-compatible for Steam version demos (1022+)\n\
 * plugin_unload now displays an error when used incorrectly (without a number)\n\
+* Improved error messages in the event of functions failing to hook\n\
 * Rewrote and optimised a whole bunch of internal stuff\n\
 ";
 
diff --git a/test/hook.test.c b/test/hook.test.c
index 9e7cfa9..625fdbf 100644
--- a/test/hook.test.c
+++ b/test/hook.test.c
@@ -30,16 +30,28 @@ __attribute__((noinline)) static int func2(int a, int b) { return a - b; }
 static int (*orig_func2)(int, int);
 static int hook2(int a, int b) { return orig_func2(a, b) + 5; }
 
+// basic reimplementation of old API to support existing test cases.
+// XXX: we could probably have tests at the boundaries of the new API too,
+// although the current tests are only testing for regressions in x86 jmp logic.
+static inline void *test_hook_inline(void *func, void *target) {
+	void *trampoline;
+	struct hook_inline_prep_ret prep = hook_inline_prep(func, &trampoline);
+	if (prep.err) return 0;
+	if (!hook_inline_mprot(prep.prologue)) return 0;
+	hook_inline_commit(prep.prologue, target);
+	return trampoline;
+}
+
 TEST("Inline hooks should be able to wrap the original function") {
 	if (!hook_init()) return false;
-	orig_func1 = (testfunc)hook_inline((void *)&func1, (void *)&hook1);
+	orig_func1 = (testfunc)test_hook_inline((void *)&func1, (void *)&hook1);
 	if (!orig_func1) return false;
 	return func1(5, 5) == 15;
 }
 
 TEST("Inline hooks should be removable again") {
 	if (!hook_init()) return false;
-	orig_func1 = (testfunc)hook_inline((void *)&func1, (void *)&hook1);
+	orig_func1 = (testfunc)test_hook_inline((void *)&func1, (void *)&hook1);
 	if (!orig_func1) return false;
 	unhook_inline((void *)orig_func1);
 	return func1(5, 5) == 10;
@@ -47,9 +59,9 @@ TEST("Inline hooks should be removable again") {
 
 TEST("Multiple functions should be able to be inline-hooked at once") {
 	if (!hook_init()) return false;
-	orig_func1 = (testfunc)hook_inline((void *)&func1, (void *)&hook1);
+	orig_func1 = (testfunc)test_hook_inline((void *)&func1, (void *)&hook1);
 	if (!orig_func1) return false;
-	orig_func2 = (testfunc)hook_inline((void *)&func2, (void *)&hook2);
+	orig_func2 = (testfunc)test_hook_inline((void *)&func2, (void *)&hook2);
 	if (!orig_func2) return false;
 	return func2(5, 5) == 5;
 }