Bug #21865: Crash on signal raise

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: 4.0.1
• Backport: 3.2: DONTNEED, 3.3: DONTNEED, 3.4: DONTNEED, 4.0: DONE

[dodecadaniel (Daniel Colson)]

This crashes on Ruby 4.0

$ ruby -e 'loop { File.open("./test.rb", kwarg: true) {} }'&
[1] 93542
$ kill -TERM 93542

[BUG] Segmentation fault at 0x000000000000001f

Note the 0x1f (i.e. 15, which is Signal.list["TERM"]).

While preparing the exception we pass argc=2, with argv containing the signal class and the fixnum for the TERM. https://github.com/ruby/ruby/blob/86dba8cfaeabb3b86df921da24b3243b9ce4ab2a/thread.c#L2759-L2766

That eventually gets passed through to https://github.com/ruby/ruby/blob/86dba8cfaeabb3b86df921da24b3243b9ce4ab2a/class.c#L3162-L3165, where we set the signal fixnum as last and treat it like a hash. But 0x1f is a fixnum, not a hash, so that crashes.

rb_scan_args_keyword_p returns true in this case even though last is not a hash because kw_flag is RB_SCAN_ARGS_PASS_CALLED_KEYWORDS (which I think causes it to look at the last frame to see if keywords were passed, rather than checking whether last is a hash. I don't think we push any frames in this case, so it's looking at whatever the last frame before the signal happened to be). Should we still check that it's a hash regardless?

I believe the bug was introduced in https://github.com/ruby/ruby/commit/64f508ade8c8535b7d3ecdd217886aa52fddd43c.

[dodecadaniel (Daniel Colson)]

• Description updated (diff)

[byroot (Jean Boussier)]

Single script version of the repro:

t = Thread.new do
  sleep 1
  Process.kill("TERM", $$)
end

loop do
  File.open(__FILE__, kwarg: true) {}
end

[byroot (Jean Boussier)]

• Backport changed from 3.2: UNKNOWN, 3.3: UNKNOWN, 3.4: UNKNOWN, 4.0: UNKNOWN to 3.2: DONTNEED, 3.3: DONTNEED, 3.4: DONTNEED, 4.0: REQUIRED

[Eregon (Benoit Daloze)]

Interestingly enough I noticed I believe the same (or very similar) segfault yesterday when trying to test the behavior of rb_scan_args_kw() with RB_SCAN_ARGS_KEYWORDS but not passing a Hash as last argument.
Given that the documentation states:

The final argument should be a hash treated as keywords.

"should" seems like it should not segfault if the should doesn't hold.

Happy to split this to a different issue if desired/if the fix is quite different.

[jeremyevans0 (Jeremy Evans)]

Eregon (Benoit Daloze) wrote in #note-4:

Interestingly enough I noticed I believe the same (or very similar) segfault yesterday when trying to test the behavior of rb_scan_args_kw() with RB_SCAN_ARGS_KEYWORDS but not passing a Hash as last argument.
Given that the documentation states:

The final argument should be a hash treated as keywords.

"should" seems like it should not segfault if the should doesn't hold.

I wrote the rb_scan_args_kw support, the should here should be must. If you want behavior where the last argument may not be a hash, you should use RB_SCAN_ARGS_LAST_HASH_KEYWORDS, not RB_SCAN_ARGS_KEYWORDS. However, I don't think that's the issue here.

My understanding of this issue is the cfunc is operating on an unexpected VM stack frame, which seems to be a more serious problem. We need to ensure the cfunc is operating on the VM stack frame it expects to be operating on, and not on a stack frame related to a signal.

[jhawthorn (John Hawthorn)]

• Status changed from Open to Closed

Applied in changeset git|3e02e99e425ee1c2e4a7cee625eaea6ed1faf1c4.

---

Fix signal crash during keyword argument call

64f508ade8 changed rb_threadptr_raise to call rb_exception_setup,
which uses rb_scan_args with RB_SCAN_ARGS_PASS_CALLED_KEYWORDS.
This checked rb_keyword_given_p(), which read the interrupted
frame's keyword state rather than the signal raise arguments,
causing a crash when a signal arrived during a keyword call.

Revert rb_threadptr_raise to use rb_make_exception directly, and
have thread_raise_m call rb_exception_setup where
rb_keyword_given_p() reflects the correct frame.

[Bug #21865]

[k0kubun (Takashi Kokubun)]

• Backport changed from 3.2: DONTNEED, 3.3: DONTNEED, 3.4: DONTNEED, 4.0: REQUIRED to 3.2: DONTNEED, 3.3: DONTNEED, 3.4: DONTNEED, 4.0: DONE

ruby_4_0 c6d9ba58c50fd9c07023453d71cb55b4b9c36957.

[Eregon (Benoit Daloze)]

jeremyevans0 (Jeremy Evans) wrote in #note-5:

I wrote the rb_scan_args_kw support, the should here should be must. If you want behavior where the last argument may not be a hash, you should use RB_SCAN_ARGS_LAST_HASH_KEYWORDS, not RB_SCAN_ARGS_KEYWORDS. However, I don't think that's the issue here.

Could you fix the docs then?

[jeremyevans0 (Jeremy Evans)]

Eregon (Benoit Daloze) wrote in #note-8:

jeremyevans0 (Jeremy Evans) wrote in #note-5:

I wrote the rb_scan_args_kw support, the should here should be must. If you want behavior where the last argument may not be a hash, you should use RB_SCAN_ARGS_LAST_HASH_KEYWORDS, not RB_SCAN_ARGS_KEYWORDS. However, I don't think that's the issue here.

Could you fix the docs then?

Sure: https://github.com/ruby/ruby/pull/16133