Skip to content

Conversation

jhawthorn
Copy link
Member

Previously in threads which weren't created to running Ruby (which includes the timer thread), calling rb_postponed_job_register_one as well as probably similar hooks would cause a segfault due to the now-thread-local EC being NULL.

This was particularly a problem for profiling tools (like stackprof), which rely on signal handlers, which may be sent to any thread in the process including those not managed by Ruby.

@jhawthorn jhawthorn force-pushed the stackprof_null_ec_crash branch from 69b4671 to 5cff4b9 Compare January 23, 2021 00:11
Previously in threads which weren't created to running Ruby (which
includes the timer thread), calling rb_postponed_job_register_one as
well as probably similar hooks would cause a segfault due to the
now-thread-local EC being NULL.

This was particularly a problem for profiling tools (like stackprof),
which rely on signal handlers, which may be sent to any thread in the
process including those not managed by Ruby.

[Fixes #17573]

Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
@jhawthorn jhawthorn force-pushed the stackprof_null_ec_crash branch from 5cff4b9 to ffd0431 Compare January 24, 2021 05:00
ko1 added a commit to ko1/ruby that referenced this pull request Nov 8, 2021
Postponed job can be registered from non-Ruby thread, which means
`ec` in TLS can be NULL. In this case, use main thread's `ec` instead.

See ruby#4108
and ruby#4336
ko1 added a commit that referenced this pull request Nov 9, 2021
Postponed job can be registered from non-Ruby thread, which means
`ec` in TLS can be NULL. In this case, use main thread's `ec` instead.

See #4108
and #4336
XrXr added a commit that referenced this pull request Nov 23, 2021
After 5680c38, postponed job APIs now
expect to be called on native threads not managed by Ruby and handles
getting a NULL execution context. However, in debug builds the change
runs into an assertion failure with GET_EC() which asserts that EC is
non-NULL. Avoid the assertion failure by passing `false` for `expect_ec`
instead as the intention is to handle when there is no EC.

Add a test from John Crepezzi and John Hawthorn to exercise this
situation.

See GH-4108
See GH-5094

[Bug #17573]

Co-authored-by: John Hawthorn <john@hawthorn.email>
Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
matzbot pushed a commit that referenced this pull request Nov 23, 2021
…Backport #17573]

	Use valid `ec` for postponed job.

	Postponed job can be registered from non-Ruby thread, which means
	`ec` in TLS can be NULL. In this case, use main thread's `ec` instead.

	See #4108
	and #4336
	---
	 vm_trace.c | 16 ++++++++++++----
	 1 file changed, 12 insertions(+), 4 deletions(-)

	Avoid assert failure when NULL EC is expected

	After 5680c38, postponed job APIs now
	expect to be called on native threads not managed by Ruby and handles
	getting a NULL execution context. However, in debug builds the change
	runs into an assertion failure with GET_EC() which asserts that EC is
	non-NULL. Avoid the assertion failure by passing `false` for `expect_ec`
	instead as the intention is to handle when there is no EC.

	Add a test from John Crepezzi and John Hawthorn to exercise this
	situation.

	See GH-4108
	See GH-5094

	[Bug #17573]

	Co-authored-by: John Hawthorn <john@hawthorn.email>
	Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
	---
	 ext/-test-/postponed_job/postponed_job.c       | 31 ++++++++++++++++++++++++++
	 test/-ext-/postponed_job/test_postponed_job.rb |  7 ++++++
	 vm_trace.c                                     |  2 +-
	 3 files changed, 39 insertions(+), 1 deletion(-)
@XrXr
Copy link
Member

XrXr commented Nov 27, 2021

Closing as test was merged in f5d2041 and fix superseded by 5680c38.

@XrXr XrXr closed this Nov 27, 2021
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.

3 participants