Skip to content

Commit

Permalink
Fix race condition in pthread_create handle initialization (#15043)
Browse files Browse the repository at this point in the history
The thread routine may be started before `GC.pthread_create` returns; `Thread#start` then calls `Crystal::System::Thread#stack_address`, which may call `LibC.pthread_getattr_np` before `GC.pthread_create` initializes the `@system_handle` instance variable, hence the segmentation fault. (On musl-libc, [the underlying non-atomic store occurs right before `LibC.pthread_create` returns](https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_create.c?id=dd1e63c3638d5f9afb857fccf6ce1415ca5f1b8b#n389).) Thus there needs to be some kind of synchronization.
  • Loading branch information
HertzDevil authored Sep 29, 2024
1 parent 8692740 commit 2821fbb
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 28 deletions.
8 changes: 0 additions & 8 deletions spec/std/thread/condition_variable_spec.cr
Original file line number Diff line number Diff line change
@@ -1,11 +1,3 @@
{% if flag?(:musl) %}
# FIXME: These thread specs occasionally fail on musl/alpine based ci, so
# they're disabled for now to reduce noise.
# See https://github.com/crystal-lang/crystal/issues/8738
pending Thread::ConditionVariable
{% skip_file %}
{% end %}

require "../spec_helper"

# interpreter doesn't support threads yet (#14287)
Expand Down
8 changes: 0 additions & 8 deletions spec/std/thread/mutex_spec.cr
Original file line number Diff line number Diff line change
@@ -1,11 +1,3 @@
{% if flag?(:musl) %}
# FIXME: These thread specs occasionally fail on musl/alpine based ci, so
# they're disabled for now to reduce noise.
# See https://github.com/crystal-lang/crystal/issues/8738
pending Thread::Mutex
{% skip_file %}
{% end %}

require "../spec_helper"

# interpreter doesn't support threads yet (#14287)
Expand Down
8 changes: 0 additions & 8 deletions spec/std/thread_spec.cr
Original file line number Diff line number Diff line change
@@ -1,13 +1,5 @@
require "./spec_helper"

{% if flag?(:musl) %}
# FIXME: These thread specs occasionally fail on musl/alpine based ci, so
# they're disabled for now to reduce noise.
# See https://github.com/crystal-lang/crystal/issues/8738
pending Thread
{% skip_file %}
{% end %}

# interpreter doesn't support threads yet (#14287)
pending_interpreted describe: Thread do
it "allows passing an argumentless fun to execute" do
Expand Down
23 changes: 19 additions & 4 deletions src/crystal/system/unix/pthread.cr
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,35 @@ module Crystal::System::Thread
@system_handle
end

protected setter system_handle

private def init_handle
# NOTE: the thread may start before `pthread_create` returns, so
# `@system_handle` must be set as soon as possible; we cannot use a separate
# handle and assign it to `@system_handle`, which would have been too late
# NOTE: `@system_handle` needs to be set here too, not just in
# `.thread_proc`, since the current thread might progress first; the value
# of `LibC.pthread_self` inside the new thread must be equal to this
# `@system_handle` after `pthread_create` returns
ret = GC.pthread_create(
thread: pointerof(@system_handle),
attr: Pointer(LibC::PthreadAttrT).null,
start: ->(data : Void*) { data.as(::Thread).start; Pointer(Void).null },
start: ->Thread.thread_proc(Void*),
arg: self.as(Void*),
)

raise RuntimeError.from_os_error("pthread_create", Errno.new(ret)) unless ret == 0
end

def self.thread_proc(data : Void*) : Void*
th = data.as(::Thread)

# `#start` calls `#stack_address`, which might read `@system_handle` before
# `GC.pthread_create` updates it in the original thread that spawned the
# current one, so we also assign to it here
th.system_handle = current_handle

th.start
Pointer(Void).null
end

def self.current_handle : Handle
LibC.pthread_self
end
Expand Down

0 comments on commit 2821fbb

Please sign in to comment.