Skip to content

Commit

Permalink
Mark asan fake stacks during machine stack marking
Browse files Browse the repository at this point in the history 
ASAN leaves a pointer to the fake frame on the stack; we can use the
__asan_addr_is_in_fake_stack API to work out the extent of the fake
stack and thus mark any VALUEs contained therein.

[Bug #20001]
  • Loading branch information
@KJTsanaktsidis
KJTsanaktsidis committed Jan 18, 2024
1 parent 3cfcb45 commit 61da90c
Show file tree
Hide file tree
Showing 8 changed files with 167 additions and 2 deletions.
43 changes: 43 additions & 0 deletions common.mk
View file

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions ext/objspace/depend
View file
Expand Up @@ -182,6 +182,7 @@ object_tracing.o: $(top_srcdir)/internal/basic_operators.h
object_tracing.o: $(top_srcdir)/internal/compilers.h
object_tracing.o: $(top_srcdir)/internal/gc.h
object_tracing.o: $(top_srcdir)/internal/imemo.h
object_tracing.o: $(top_srcdir)/internal/sanitizers.h
object_tracing.o: $(top_srcdir)/internal/serial.h
object_tracing.o: $(top_srcdir)/internal/static_assert.h
object_tracing.o: $(top_srcdir)/internal/vm.h
Expand Down
1 change: 1 addition & 0 deletions ext/ripper/depend
View file
Expand Up @@ -593,6 +593,7 @@ ripper.o: $(top_srcdir)/internal/parse.h
ripper.o: $(top_srcdir)/internal/rational.h
ripper.o: $(top_srcdir)/internal/re.h
ripper.o: $(top_srcdir)/internal/ruby_parser.h
ripper.o: $(top_srcdir)/internal/sanitizers.h
ripper.o: $(top_srcdir)/internal/serial.h
ripper.o: $(top_srcdir)/internal/static_assert.h
ripper.o: $(top_srcdir)/internal/string.h
Expand Down
15 changes: 15 additions & 0 deletions ext/socket/depend
View file
Expand Up @@ -199,6 +199,7 @@ ancdata.o: $(top_srcdir)/internal/error.h
ancdata.o: $(top_srcdir)/internal/gc.h
ancdata.o: $(top_srcdir)/internal/imemo.h
ancdata.o: $(top_srcdir)/internal/io.h
ancdata.o: $(top_srcdir)/internal/sanitizers.h
ancdata.o: $(top_srcdir)/internal/serial.h
ancdata.o: $(top_srcdir)/internal/static_assert.h
ancdata.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -408,6 +409,7 @@ basicsocket.o: $(top_srcdir)/internal/error.h
basicsocket.o: $(top_srcdir)/internal/gc.h
basicsocket.o: $(top_srcdir)/internal/imemo.h
basicsocket.o: $(top_srcdir)/internal/io.h
basicsocket.o: $(top_srcdir)/internal/sanitizers.h
basicsocket.o: $(top_srcdir)/internal/serial.h
basicsocket.o: $(top_srcdir)/internal/static_assert.h
basicsocket.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -617,6 +619,7 @@ constants.o: $(top_srcdir)/internal/error.h
constants.o: $(top_srcdir)/internal/gc.h
constants.o: $(top_srcdir)/internal/imemo.h
constants.o: $(top_srcdir)/internal/io.h
constants.o: $(top_srcdir)/internal/sanitizers.h
constants.o: $(top_srcdir)/internal/serial.h
constants.o: $(top_srcdir)/internal/static_assert.h
constants.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -827,6 +830,7 @@ ifaddr.o: $(top_srcdir)/internal/error.h
ifaddr.o: $(top_srcdir)/internal/gc.h
ifaddr.o: $(top_srcdir)/internal/imemo.h
ifaddr.o: $(top_srcdir)/internal/io.h
ifaddr.o: $(top_srcdir)/internal/sanitizers.h
ifaddr.o: $(top_srcdir)/internal/serial.h
ifaddr.o: $(top_srcdir)/internal/static_assert.h
ifaddr.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -1036,6 +1040,7 @@ init.o: $(top_srcdir)/internal/error.h
init.o: $(top_srcdir)/internal/gc.h
init.o: $(top_srcdir)/internal/imemo.h
init.o: $(top_srcdir)/internal/io.h
init.o: $(top_srcdir)/internal/sanitizers.h
init.o: $(top_srcdir)/internal/serial.h
init.o: $(top_srcdir)/internal/static_assert.h
init.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -1245,6 +1250,7 @@ ipsocket.o: $(top_srcdir)/internal/error.h
ipsocket.o: $(top_srcdir)/internal/gc.h
ipsocket.o: $(top_srcdir)/internal/imemo.h
ipsocket.o: $(top_srcdir)/internal/io.h
ipsocket.o: $(top_srcdir)/internal/sanitizers.h
ipsocket.o: $(top_srcdir)/internal/serial.h
ipsocket.o: $(top_srcdir)/internal/static_assert.h
ipsocket.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -1454,6 +1460,7 @@ option.o: $(top_srcdir)/internal/error.h
option.o: $(top_srcdir)/internal/gc.h
option.o: $(top_srcdir)/internal/imemo.h
option.o: $(top_srcdir)/internal/io.h
option.o: $(top_srcdir)/internal/sanitizers.h
option.o: $(top_srcdir)/internal/serial.h
option.o: $(top_srcdir)/internal/static_assert.h
option.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -1663,6 +1670,7 @@ raddrinfo.o: $(top_srcdir)/internal/error.h
raddrinfo.o: $(top_srcdir)/internal/gc.h
raddrinfo.o: $(top_srcdir)/internal/imemo.h
raddrinfo.o: $(top_srcdir)/internal/io.h
raddrinfo.o: $(top_srcdir)/internal/sanitizers.h
raddrinfo.o: $(top_srcdir)/internal/serial.h
raddrinfo.o: $(top_srcdir)/internal/static_assert.h
raddrinfo.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -1872,6 +1880,7 @@ socket.o: $(top_srcdir)/internal/error.h
socket.o: $(top_srcdir)/internal/gc.h
socket.o: $(top_srcdir)/internal/imemo.h
socket.o: $(top_srcdir)/internal/io.h
socket.o: $(top_srcdir)/internal/sanitizers.h
socket.o: $(top_srcdir)/internal/serial.h
socket.o: $(top_srcdir)/internal/static_assert.h
socket.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -2081,6 +2090,7 @@ sockssocket.o: $(top_srcdir)/internal/error.h
sockssocket.o: $(top_srcdir)/internal/gc.h
sockssocket.o: $(top_srcdir)/internal/imemo.h
sockssocket.o: $(top_srcdir)/internal/io.h
sockssocket.o: $(top_srcdir)/internal/sanitizers.h
sockssocket.o: $(top_srcdir)/internal/serial.h
sockssocket.o: $(top_srcdir)/internal/static_assert.h
sockssocket.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -2290,6 +2300,7 @@ tcpserver.o: $(top_srcdir)/internal/error.h
tcpserver.o: $(top_srcdir)/internal/gc.h
tcpserver.o: $(top_srcdir)/internal/imemo.h
tcpserver.o: $(top_srcdir)/internal/io.h
tcpserver.o: $(top_srcdir)/internal/sanitizers.h
tcpserver.o: $(top_srcdir)/internal/serial.h
tcpserver.o: $(top_srcdir)/internal/static_assert.h
tcpserver.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -2499,6 +2510,7 @@ tcpsocket.o: $(top_srcdir)/internal/error.h
tcpsocket.o: $(top_srcdir)/internal/gc.h
tcpsocket.o: $(top_srcdir)/internal/imemo.h
tcpsocket.o: $(top_srcdir)/internal/io.h
tcpsocket.o: $(top_srcdir)/internal/sanitizers.h
tcpsocket.o: $(top_srcdir)/internal/serial.h
tcpsocket.o: $(top_srcdir)/internal/static_assert.h
tcpsocket.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -2708,6 +2720,7 @@ udpsocket.o: $(top_srcdir)/internal/error.h
udpsocket.o: $(top_srcdir)/internal/gc.h
udpsocket.o: $(top_srcdir)/internal/imemo.h
udpsocket.o: $(top_srcdir)/internal/io.h
udpsocket.o: $(top_srcdir)/internal/sanitizers.h
udpsocket.o: $(top_srcdir)/internal/serial.h
udpsocket.o: $(top_srcdir)/internal/static_assert.h
udpsocket.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -2917,6 +2930,7 @@ unixserver.o: $(top_srcdir)/internal/error.h
unixserver.o: $(top_srcdir)/internal/gc.h
unixserver.o: $(top_srcdir)/internal/imemo.h
unixserver.o: $(top_srcdir)/internal/io.h
unixserver.o: $(top_srcdir)/internal/sanitizers.h
unixserver.o: $(top_srcdir)/internal/serial.h
unixserver.o: $(top_srcdir)/internal/static_assert.h
unixserver.o: $(top_srcdir)/internal/string.h
Expand Down Expand Up @@ -3126,6 +3140,7 @@ unixsocket.o: $(top_srcdir)/internal/error.h
unixsocket.o: $(top_srcdir)/internal/gc.h
unixsocket.o: $(top_srcdir)/internal/imemo.h
unixsocket.o: $(top_srcdir)/internal/io.h
unixsocket.o: $(top_srcdir)/internal/sanitizers.h
unixsocket.o: $(top_srcdir)/internal/serial.h
unixsocket.o: $(top_srcdir)/internal/static_assert.h
unixsocket.o: $(top_srcdir)/internal/string.h
Expand Down
36 changes: 34 additions & 2 deletions gc.c
View file
Expand Up @@ -953,6 +953,11 @@ typedef struct rb_objspace {

rb_darray(VALUE *) weak_references;
rb_postponed_job_handle_t finalize_deferred_pjob;

#ifdef RUBY_ASAN_ENABLED
rb_execution_context_t *marking_machine_context_ec;
#endif

} rb_objspace_t;


Expand Down Expand Up @@ -6821,6 +6826,26 @@ mark_const_tbl(rb_objspace_t *objspace, struct rb_id_table *tbl)
static void each_stack_location(rb_objspace_t *objspace, const rb_execution_context_t *ec,
const VALUE *stack_start, const VALUE *stack_end, void (*cb)(rb_objspace_t *, VALUE));

static void
gc_mark_machine_stack_location_maybe(rb_objspace_t *objspace, VALUE obj)
{
gc_mark_maybe(objspace, obj);

#ifdef RUBY_ASAN_ENABLED
rb_execution_context_t *ec = objspace->marking_machine_context_ec;
void *fake_frame_start;
void *fake_frame_end;
bool is_fake_frame = asan_get_fake_stack_extents(
ec->thread_ptr->asan_fake_stack_handle, obj,
ec->machine.stack_start, ec->machine.stack_end,
&fake_frame_start, &fake_frame_end
);
if (is_fake_frame) {
each_stack_location(objspace, ec, fake_frame_start, fake_frame_end, gc_mark_maybe);
}
#endif
}

#if defined(__wasm__)


Expand Down Expand Up @@ -6882,9 +6907,16 @@ mark_current_machine_context(rb_objspace_t *objspace, rb_execution_context_t *ec
SET_STACK_END;
GET_STACK_BOUNDS(stack_start, stack_end, 1);

each_location(objspace, save_regs_gc_mark.v, numberof(save_regs_gc_mark.v), gc_mark_maybe);
#ifdef RUBY_ASAN_ENABLED
objspace->marking_machine_context_ec = ec;
#endif

each_stack_location(objspace, ec, stack_start, stack_end, gc_mark_maybe);
each_location(objspace, save_regs_gc_mark.v, numberof(save_regs_gc_mark.v), gc_mark_machine_stack_location_maybe);
each_stack_location(objspace, ec, stack_start, stack_end, gc_mark_machine_stack_location_maybe);

#ifdef RUBY_ASAN_ENABLED
objspace->marking_machine_context_ec = NULL;
#endif
}
#endif

Expand Down
64 changes: 64 additions & 0 deletions internal/sanitizers.h
View file
Expand Up @@ -213,4 +213,68 @@ asan_get_real_stack_addr(void* slot)
return addr ? addr : slot;
}

/*!
* Gets the current thread's fake stack handle, which can be passed into get_fake_stack_extents
*
* \retval An opaque value which can be passed to asan_get_fake_stack_extents
*/
static inline void *
asan_get_thread_fake_stack_handle(void)
{
return __asan_get_current_fake_stack();
}

/*!
* Checks if the given VALUE _actually_ represents a pointer to an ASAN fake stack.
*
* If the given slot _is_ actually a reference to an ASAN fake stack, and that fake stack
* contains the real values for the passed-in range of machine stack addresses, returns true
* and the range of the fake stack through the outparams.
*
* Otherwise, returns false, and sets the outparams to NULL.
*
* Note that this function expects "start" to be > "end" on downward-growing stack architectures;
*
* \param[in] thread_fake_stack_handle The asan fake stack reference for the thread we're scanning
* \param[in] slot The value on the machine stack we want to inspect
* \param[in] machine_stack_start The extents of the real machine stack on which slot lives
* \param[in] machine_stack_end The extents of the real machine stack on which slot lives
* \param[out] fake_stack_start_out The extents of the fake stack which contains real VALUEs
* \param[out] fake_stack_end_out The extents of the fake stack which contains real VALUEs
* \return Whether slot is a pointer to a fake stack for the given machine stack range
*/

static inline bool
asan_get_fake_stack_extents(void *thread_fake_stack_handle, VALUE slot,
void *machine_stack_start, void *machine_stack_end,
void **fake_stack_start_out, void **fake_stack_end_out)
{
/* the ifdef is needed here to suppress a warning about fake_frame_{start/end} being
uninitialized if __asan_addr_is_in_fake_stack is an empty macro */
#ifdef RUBY_ASAN_ENABLED
void *fake_frame_start;
void *fake_frame_end;
void *real_stack_frame = __asan_addr_is_in_fake_stack(
thread_fake_stack_handle, (void *)slot, &fake_frame_start, &fake_frame_end
);
if (real_stack_frame) {
bool in_range;
#if STACK_GROW_DIRECTION < 0
in_range = machine_stack_start >= real_stack_frame && real_stack_frame >= machine_stack_end;
#else
in_range = machine_stack_start <= real_stack_frame && real_stack_frame <= machine_stack_end;
#endif
if (in_range) {
*fake_stack_start_out = fake_frame_start;
*fake_stack_end_out = fake_frame_end;
return true;
}
}
#endif
*fake_stack_start_out = 0;
*fake_stack_end_out = 0;
return false;
}


#endif /* INTERNAL_SANITIZERS_H */
3 changes: 3 additions & 0 deletions thread.c
View file
Expand Up @@ -525,6 +525,9 @@ void
ruby_thread_init_stack(rb_thread_t *th, void *local_in_parent_frame)
{
native_thread_init_stack(th, local_in_parent_frame);
#ifdef RUBY_ASAN_ENABLED
th->asan_fake_stack_handle = asan_get_thread_fake_stack_handle();
#endif
}

const VALUE *
Expand Down
6 changes: 6 additions & 0 deletions vm_core.h
View file
Expand Up @@ -94,6 +94,7 @@ extern int ruby_assert_critical_section_entered;
#include "internal.h"
#include "internal/array.h"
#include "internal/basic_operators.h"
#include "internal/sanitizers.h"
#include "internal/serial.h"
#include "internal/vm.h"
#include "method.h"
Expand Down Expand Up @@ -1153,6 +1154,11 @@ typedef struct rb_thread_struct {
void **specific_storage;

struct rb_ext_config ext_config;

#ifdef RUBY_ASAN_ENABLED
void *asan_fake_stack_handle;
#endif

} rb_thread_t;

static inline unsigned int
Expand Down

5 comments on commit 61da90c

@JasonLunn
Copy link

Choose a reason for hiding this comment

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

@KJTsanaktsidis - can this be back ported to 3.3?

@KJTsanaktsidis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JasonLunn I don’t think so. This (and #10122) are probably too invasive to be considered for backporting, and it doesn’t really fix a real bug (Asan support beforehand was so broken I can’t imagine anybody was actually using it).

next week I plan to merge ☝️ and a few other fixes, with which I have the entire test suite actually passing with ASAN. At that point, I’m happy to have a stab at cutting a branch from v3.3.0 and trying to backport all of the ASAN work I’ve done into it, if this will help you. However it won’t be considered for backport in the official ruby 3.3 branch.

would this be helpful for you?

@JasonLunn
Copy link

Choose a reason for hiding this comment

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

I can pause until you're finished landing fixes in main branch. I was hitting issues trying to get our own test suite passing with ASAN on a nightly snapshot of the master branch, and I was attributing that to it being 3.4.x, so I tried 3.3 but this fix isn't there. Probably I need to wait for other fixes to land first.

What's the best way for me to stay coordinated with the overall effort?

@JasonLunn
Copy link

Choose a reason for hiding this comment

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

I can pause until you're finished landing fixes in main branch. I was hitting issues trying to get our own test suite passing with ASAN on a nightly snapshot of the master branch, and I was attributing that to it being 3.4.x, so I tried 3.3 but this fix isn't there. Probably I need to wait for other fixes to land first.

What's the best way for me to stay coordinated with the overall effort?

@KJTsanaktsidis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JasonLunn I opened a ticket https://bugs.ruby-lang.org/issues/20387 where I summarised the current state of things and I'll keep it updated as I work on this. It's all very close to working though!

Please sign in to comment.