Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

guminterceptor: add alt stack support #826

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ajwerner
Copy link

This commit adds a new public API to gum to configure the size of an alternate stack to use for hooking. If the configuration is set, then when an interceptor is triggered, it will switch to this stack after the trampoline but before executing the handler. This is needed to support go or programs which do not have large and mutable thread stacks.

One note is that only x86_64 support has initially been added, and that it only applies for entry invocation listeners. Exit invocation listeners are not supported because they still would not work with go out-of-the-box because go doesn't like junk being added to call stacks.

This commit adds a new public API to gum to configure the size of an
alternate stack to use for hooking. If the configuration is set, then
when an interceptor is triggered, it will switch to this stack after the
trampoline but before executing the handler. This is needed to support
go or programs which do not have large and mutable thread stacks.

One note is that only x86_64 support has initially been added, and that
it only applies for entry invocation listeners. Exit invocation
listeners are not supported because they still would not work with go
out-of-the-box because go doesn't like junk being added to call stacks.
@ajwerner
Copy link
Author

One question I have is how best to test this in CI. Locally I changed the default value the stack size to enable the alternate stack switching and I observed the tests passing. Ideally we could run the interceptor suite twice -- once on the local stack and once on the switched stack. Any pointers on how best to do this would be great.

Copy link
Member

@oleavr oleavr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a stab at this! This looks promising. (Apologies for the delay! 😅)

// Switch to the new stack and call the function with arguments
asm volatile (
"movq %%rsp, %%rbx\n" // Save the current stack pointer
"movq %0, %%rsp\n" // Set the new stack pointer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this may be too late in some cases, as we've already used quite some stack space for saving registers?

@@ -1782,7 +1897,7 @@ gum_function_context_fixup_cpu_context (GumFunctionContext * function_ctx,
}

static InterceptorThreadContext *
get_interceptor_thread_context (void)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect; in C (unlike C++), (void) means "no arguments", whereas () means "0 or more arguments".

@@ -84,6 +84,13 @@ GUM_API void gum_interceptor_with_lock_held (GumInterceptor * self,
GumInterceptorLockedFunc func, gpointer user_data);
GUM_API gboolean gum_interceptor_is_locked (GumInterceptor * self);

// Configure the alternate stack for the interceptor. The alternate stack is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nitpick: Only C-style comments should be used.

@@ -1939,9 +2054,16 @@ interceptor_thread_context_new (void)
context->stack = g_array_sized_new (FALSE, TRUE,
sizeof (GumInvocationStackEntry), GUM_MAX_CALL_DEPTH);


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nitpick: For consistency, two or more consecutive blank lines shouldn't be used.

@@ -1919,7 +2034,7 @@ gum_interceptor_replacement_invocation_backend =
};

static InterceptorThreadContext *
interceptor_thread_context_new (void)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(See earlier comment.)

@@ -27,6 +27,10 @@
# define GUM_INTERCEPTOR_CODE_SLICE_SIZE 256
#endif

#if defined(__x86_64__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if defined(__x86_64__)
#if defined (HAVE_I386) && GLIB_SIZEOF_VOID_P == 8

(For consistency.)

Should also check that GCC or Clang is used, as MSVC doesn't support the inline assembly syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants