Skip to content

Commit 5b3f1c4

Browse files
luke-gruberjhawthorn
authored andcommitted
Take VM lock around manipulation of fiber pool for vacant stacks
When creating fibers in multiple ractors at the same time there were issues with the manipulation of this structure, causing segfaults. I didn't add any tests for this because I'm making a more general PR in the very near future to be able to run test methods (test-all suite) inside multiple ractors at the same time. This is how this bug was caught, running test/ruby/test_fiber.rb inside 10 ractors at once.
1 parent 38ecaca commit 5b3f1c4

1 file changed

Lines changed: 110 additions & 97 deletions

File tree

cont.c

Lines changed: 110 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -509,83 +509,87 @@ fiber_pool_allocate_memory(size_t * count, size_t stride)
509509
static struct fiber_pool_allocation *
510510
fiber_pool_expand(struct fiber_pool * fiber_pool, size_t count)
511511
{
512-
STACK_GROW_DIR_DETECTION;
512+
struct fiber_pool_allocation * allocation;
513+
RB_VM_LOCK_ENTER();
514+
{
515+
STACK_GROW_DIR_DETECTION;
513516

514-
size_t size = fiber_pool->size;
515-
size_t stride = size + RB_PAGE_SIZE;
517+
size_t size = fiber_pool->size;
518+
size_t stride = size + RB_PAGE_SIZE;
516519

517-
// Allocate the memory required for the stacks:
518-
void * base = fiber_pool_allocate_memory(&count, stride);
520+
// Allocate the memory required for the stacks:
521+
void * base = fiber_pool_allocate_memory(&count, stride);
519522

520-
if (base == NULL) {
521-
rb_raise(rb_eFiberError, "can't alloc machine stack to fiber (%"PRIuSIZE" x %"PRIuSIZE" bytes): %s", count, size, ERRNOMSG);
522-
}
523+
if (base == NULL) {
524+
rb_raise(rb_eFiberError, "can't alloc machine stack to fiber (%"PRIuSIZE" x %"PRIuSIZE" bytes): %s", count, size, ERRNOMSG);
525+
}
523526

524-
struct fiber_pool_vacancy * vacancies = fiber_pool->vacancies;
525-
struct fiber_pool_allocation * allocation = RB_ALLOC(struct fiber_pool_allocation);
527+
struct fiber_pool_vacancy * vacancies = fiber_pool->vacancies;
528+
allocation = RB_ALLOC(struct fiber_pool_allocation);
526529

527-
// Initialize fiber pool allocation:
528-
allocation->base = base;
529-
allocation->size = size;
530-
allocation->stride = stride;
531-
allocation->count = count;
530+
// Initialize fiber pool allocation:
531+
allocation->base = base;
532+
allocation->size = size;
533+
allocation->stride = stride;
534+
allocation->count = count;
532535
#ifdef FIBER_POOL_ALLOCATION_FREE
533-
allocation->used = 0;
536+
allocation->used = 0;
534537
#endif
535-
allocation->pool = fiber_pool;
536-
537-
if (DEBUG) {
538-
fprintf(stderr, "fiber_pool_expand(%"PRIuSIZE"): %p, %"PRIuSIZE"/%"PRIuSIZE" x [%"PRIuSIZE":%"PRIuSIZE"]\n",
539-
count, (void*)fiber_pool, fiber_pool->used, fiber_pool->count, size, fiber_pool->vm_stack_size);
540-
}
538+
allocation->pool = fiber_pool;
541539

542-
// Iterate over all stacks, initializing the vacancy list:
543-
for (size_t i = 0; i < count; i += 1) {
544-
void * base = (char*)allocation->base + (stride * i);
545-
void * page = (char*)base + STACK_DIR_UPPER(size, 0);
540+
if (DEBUG) {
541+
fprintf(stderr, "fiber_pool_expand(%"PRIuSIZE"): %p, %"PRIuSIZE"/%"PRIuSIZE" x [%"PRIuSIZE":%"PRIuSIZE"]\n",
542+
count, (void*)fiber_pool, fiber_pool->used, fiber_pool->count, size, fiber_pool->vm_stack_size);
543+
}
546544

545+
// Iterate over all stacks, initializing the vacancy list:
546+
for (size_t i = 0; i < count; i += 1) {
547+
void * base = (char*)allocation->base + (stride * i);
548+
void * page = (char*)base + STACK_DIR_UPPER(size, 0);
547549
#if defined(_WIN32)
548-
DWORD old_protect;
550+
DWORD old_protect;
549551

550-
if (!VirtualProtect(page, RB_PAGE_SIZE, PAGE_READWRITE | PAGE_GUARD, &old_protect)) {
551-
VirtualFree(allocation->base, 0, MEM_RELEASE);
552-
rb_raise(rb_eFiberError, "can't set a guard page: %s", ERRNOMSG);
553-
}
552+
if (!VirtualProtect(page, RB_PAGE_SIZE, PAGE_READWRITE | PAGE_GUARD, &old_protect)) {
553+
VirtualFree(allocation->base, 0, MEM_RELEASE);
554+
rb_raise(rb_eFiberError, "can't set a guard page: %s", ERRNOMSG);
555+
}
554556
#elif defined(__wasi__)
555-
// wasi-libc's mprotect emulation doesn't support PROT_NONE.
556-
(void)page;
557+
// wasi-libc's mprotect emulation doesn't support PROT_NONE.
558+
(void)page;
557559
#else
558-
if (mprotect(page, RB_PAGE_SIZE, PROT_NONE) < 0) {
559-
munmap(allocation->base, count*stride);
560-
rb_raise(rb_eFiberError, "can't set a guard page: %s", ERRNOMSG);
561-
}
560+
if (mprotect(page, RB_PAGE_SIZE, PROT_NONE) < 0) {
561+
munmap(allocation->base, count*stride);
562+
rb_raise(rb_eFiberError, "can't set a guard page: %s", ERRNOMSG);
563+
}
562564
#endif
563565

564-
vacancies = fiber_pool_vacancy_initialize(
565-
fiber_pool, vacancies,
566-
(char*)base + STACK_DIR_UPPER(0, RB_PAGE_SIZE),
567-
size
568-
);
566+
vacancies = fiber_pool_vacancy_initialize(
567+
fiber_pool, vacancies,
568+
(char*)base + STACK_DIR_UPPER(0, RB_PAGE_SIZE),
569+
size
570+
);
569571

570572
#ifdef FIBER_POOL_ALLOCATION_FREE
571-
vacancies->stack.allocation = allocation;
573+
vacancies->stack.allocation = allocation;
572574
#endif
573-
}
575+
}
574576

575-
// Insert the allocation into the head of the pool:
576-
allocation->next = fiber_pool->allocations;
577+
// Insert the allocation into the head of the pool:
578+
allocation->next = fiber_pool->allocations;
577579

578580
#ifdef FIBER_POOL_ALLOCATION_FREE
579-
if (allocation->next) {
580-
allocation->next->previous = allocation;
581-
}
581+
if (allocation->next) {
582+
allocation->next->previous = allocation;
583+
}
582584

583-
allocation->previous = NULL;
585+
allocation->previous = NULL;
584586
#endif
585587

586-
fiber_pool->allocations = allocation;
587-
fiber_pool->vacancies = vacancies;
588-
fiber_pool->count += count;
588+
fiber_pool->allocations = allocation;
589+
fiber_pool->vacancies = vacancies;
590+
fiber_pool->count += count;
591+
}
592+
RB_VM_LOCK_LEAVE();
589593

590594
return allocation;
591595
}
@@ -659,41 +663,46 @@ fiber_pool_allocation_free(struct fiber_pool_allocation * allocation)
659663
static struct fiber_pool_stack
660664
fiber_pool_stack_acquire(struct fiber_pool * fiber_pool)
661665
{
662-
struct fiber_pool_vacancy * vacancy = fiber_pool_vacancy_pop(fiber_pool);
666+
struct fiber_pool_vacancy * vacancy ;
667+
RB_VM_LOCK_ENTER();
668+
{
669+
vacancy = fiber_pool_vacancy_pop(fiber_pool);
663670

664-
if (DEBUG) fprintf(stderr, "fiber_pool_stack_acquire: %p used=%"PRIuSIZE"\n", (void*)fiber_pool->vacancies, fiber_pool->used);
671+
if (DEBUG) fprintf(stderr, "fiber_pool_stack_acquire: %p used=%"PRIuSIZE"\n", (void*)fiber_pool->vacancies, fiber_pool->used);
665672

666-
if (!vacancy) {
667-
const size_t maximum = FIBER_POOL_ALLOCATION_MAXIMUM_SIZE;
668-
const size_t minimum = fiber_pool->initial_count;
673+
if (!vacancy) {
674+
const size_t maximum = FIBER_POOL_ALLOCATION_MAXIMUM_SIZE;
675+
const size_t minimum = fiber_pool->initial_count;
669676

670-
size_t count = fiber_pool->count;
671-
if (count > maximum) count = maximum;
672-
if (count < minimum) count = minimum;
677+
size_t count = fiber_pool->count;
678+
if (count > maximum) count = maximum;
679+
if (count < minimum) count = minimum;
673680

674-
fiber_pool_expand(fiber_pool, count);
681+
fiber_pool_expand(fiber_pool, count);
675682

676-
// The free list should now contain some stacks:
677-
VM_ASSERT(fiber_pool->vacancies);
683+
// The free list should now contain some stacks:
684+
VM_ASSERT(fiber_pool->vacancies);
678685

679-
vacancy = fiber_pool_vacancy_pop(fiber_pool);
680-
}
686+
vacancy = fiber_pool_vacancy_pop(fiber_pool);
687+
}
681688

682-
VM_ASSERT(vacancy);
683-
VM_ASSERT(vacancy->stack.base);
689+
VM_ASSERT(vacancy);
690+
VM_ASSERT(vacancy->stack.base);
684691

685692
#if defined(COROUTINE_SANITIZE_ADDRESS)
686-
__asan_unpoison_memory_region(fiber_pool_stack_poison_base(&vacancy->stack), fiber_pool_stack_poison_size(&vacancy->stack));
693+
__asan_unpoison_memory_region(fiber_pool_stack_poison_base(&vacancy->stack), fiber_pool_stack_poison_size(&vacancy->stack));
687694
#endif
688695

689-
// Take the top item from the free list:
690-
fiber_pool->used += 1;
696+
// Take the top item from the free list:
697+
fiber_pool->used += 1;
691698

692699
#ifdef FIBER_POOL_ALLOCATION_FREE
693-
vacancy->stack.allocation->used += 1;
700+
vacancy->stack.allocation->used += 1;
694701
#endif
695702

696-
fiber_pool_stack_reset(&vacancy->stack);
703+
fiber_pool_stack_reset(&vacancy->stack);
704+
}
705+
RB_VM_LOCK_LEAVE();
697706

698707
return vacancy->stack;
699708
}
@@ -764,40 +773,44 @@ static void
764773
fiber_pool_stack_release(struct fiber_pool_stack * stack)
765774
{
766775
struct fiber_pool * pool = stack->pool;
767-
struct fiber_pool_vacancy * vacancy = fiber_pool_vacancy_pointer(stack->base, stack->size);
776+
RB_VM_LOCK_ENTER();
777+
{
778+
struct fiber_pool_vacancy * vacancy = fiber_pool_vacancy_pointer(stack->base, stack->size);
768779

769-
if (DEBUG) fprintf(stderr, "fiber_pool_stack_release: %p used=%"PRIuSIZE"\n", stack->base, stack->pool->used);
780+
if (DEBUG) fprintf(stderr, "fiber_pool_stack_release: %p used=%"PRIuSIZE"\n", stack->base, stack->pool->used);
770781

771-
// Copy the stack details into the vacancy area:
772-
vacancy->stack = *stack;
773-
// After this point, be careful about updating/using state in stack, since it's copied to the vacancy area.
782+
// Copy the stack details into the vacancy area:
783+
vacancy->stack = *stack;
784+
// After this point, be careful about updating/using state in stack, since it's copied to the vacancy area.
774785

775-
// Reset the stack pointers and reserve space for the vacancy data:
776-
fiber_pool_vacancy_reset(vacancy);
786+
// Reset the stack pointers and reserve space for the vacancy data:
787+
fiber_pool_vacancy_reset(vacancy);
777788

778-
// Push the vacancy into the vancancies list:
779-
pool->vacancies = fiber_pool_vacancy_push(vacancy, pool->vacancies);
780-
pool->used -= 1;
789+
// Push the vacancy into the vancancies list:
790+
pool->vacancies = fiber_pool_vacancy_push(vacancy, pool->vacancies);
791+
pool->used -= 1;
781792

782793
#ifdef FIBER_POOL_ALLOCATION_FREE
783-
struct fiber_pool_allocation * allocation = stack->allocation;
794+
struct fiber_pool_allocation * allocation = stack->allocation;
784795

785-
allocation->used -= 1;
796+
allocation->used -= 1;
786797

787-
// Release address space and/or dirty memory:
788-
if (allocation->used == 0) {
789-
fiber_pool_allocation_free(allocation);
790-
}
791-
else if (stack->pool->free_stacks) {
792-
fiber_pool_stack_free(&vacancy->stack);
793-
}
798+
// Release address space and/or dirty memory:
799+
if (allocation->used == 0) {
800+
fiber_pool_allocation_free(allocation);
801+
}
802+
else if (stack->pool->free_stacks) {
803+
fiber_pool_stack_free(&vacancy->stack);
804+
}
794805
#else
795-
// This is entirely optional, but clears the dirty flag from the stack
796-
// memory, so it won't get swapped to disk when there is memory pressure:
797-
if (stack->pool->free_stacks) {
798-
fiber_pool_stack_free(&vacancy->stack);
799-
}
806+
// This is entirely optional, but clears the dirty flag from the stack
807+
// memory, so it won't get swapped to disk when there is memory pressure:
808+
if (stack->pool->free_stacks) {
809+
fiber_pool_stack_free(&vacancy->stack);
810+
}
800811
#endif
812+
}
813+
RB_VM_LOCK_LEAVE();
801814
}
802815

803816
static inline void

0 commit comments

Comments
 (0)