[ruby-core:124688] [Ruby Bug#21865] Crash on signal raise

[ruby-core:124688] [Ruby Bug#21865] Crash on signal raise

[dodecadaniel (Daniel Colson) via ruby-core]

Issue #21865 has been reported by dodecadaniel (Daniel Colson).

----------------------------------------
Bug #21865: Crash on signal raise
https://bugs.ruby-lang.org/issues/21865

* Author: dodecadaniel (Daniel Colson)
* Status: Open
* ruby -v: 4.0.1
* Backport: 3.2: UNKNOWN, 3.3: UNKNOWN, 3.4: UNKNOWN, 4.0: UNKNOWN
----------------------------------------
This crashes on Ruby 4.0

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

[BUG] Segmentation fault at 0x000000000000001f
```

Note the `0x1f`, which is Ruby 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.



-- 
https://bugs.ruby-lang.org/
______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:124695] [Ruby Bug#21865] Crash on signal raise

[byroot (Jean Boussier) via ruby-core]

Issue #21865 has been updated by byroot (Jean Boussier).


Single script version of the repro:

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

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


----------------------------------------
Bug #21865: Crash on signal raise
https://bugs.ruby-lang.org/issues/21865#change-116289

* Author: dodecadaniel (Daniel Colson)
* Status: Open
* ruby -v: 4.0.1
* Backport: 3.2: UNKNOWN, 3.3: UNKNOWN, 3.4: UNKNOWN, 4.0: UNKNOWN
----------------------------------------
This crashes on Ruby 4.0

``` sh
$ 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.



-- 
https://bugs.ruby-lang.org/
______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:124697] [Ruby Bug#21865] Crash on signal raise

[Eregon (Benoit Daloze) via ruby-core]

Issue #21865 has been updated by Eregon (Benoit Daloze).


Interestingly enough I noticed I believe the same segfault yesterday when [trying to test the behavior](https://github.com/truffleruby/truffleruby/pull/4151#discussion_r2771020336) of `rb_scan_args_kw()` with [RB_PASS_KEYWORDS](https://github.com/ruby/ruby/blob/86dba8cfaeabb3b86df921da24b3243b9ce4ab2a/include/ruby/internal/scan_args.h#L72C9-L72C25) but not passing a Hash as last argument.
Given that the documentation states:
> Pass keywords, final argument should be a hash of keywords.

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

----------------------------------------
Bug #21865: Crash on signal raise
https://bugs.ruby-lang.org/issues/21865#change-116292

* Author: dodecadaniel (Daniel Colson)
* Status: Open
* ruby -v: 4.0.1
* Backport: 3.2: DONTNEED, 3.3: DONTNEED, 3.4: DONTNEED, 4.0: REQUIRED
----------------------------------------
This crashes on Ruby 4.0

``` sh
$ 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.



-- 
https://bugs.ruby-lang.org/
______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:124698] [Ruby Bug#21865] Crash on signal raise

[jeremyevans0 (Jeremy Evans) via ruby-core]

Issue #21865 has been updated by 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](https://github.com/truffleruby/truffleruby/pull/4151#discussion_r2771020336) of `rb_scan_args_kw()` with [RB_SCAN_ARGS_KEYWORDS](https://github.com/ruby/ruby/blob/86dba8cfaeabb3b86df921da24b3243b9ce4ab2a/include/ruby/internal/scan_args.h#L52-L53) 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.

----------------------------------------
Bug #21865: Crash on signal raise
https://bugs.ruby-lang.org/issues/21865#change-116293

* Author: dodecadaniel (Daniel Colson)
* Status: Open
* ruby -v: 4.0.1
* Backport: 3.2: DONTNEED, 3.3: DONTNEED, 3.4: DONTNEED, 4.0: REQUIRED
----------------------------------------
This crashes on Ruby 4.0

``` sh
$ 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.



-- 
https://bugs.ruby-lang.org/
______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:124743] [Ruby Bug#21865] Crash on signal raise

[k0kubun (Takashi Kokubun) via ruby-core]

Issue #21865 has been updated by 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 commit:c6d9ba58c50fd9c07023453d71cb55b4b9c36957.

----------------------------------------
Bug #21865: Crash on signal raise
https://bugs.ruby-lang.org/issues/21865#change-116345

* Author: dodecadaniel (Daniel Colson)
* Status: Closed
* ruby -v: 4.0.1
* Backport: 3.2: DONTNEED, 3.3: DONTNEED, 3.4: DONTNEED, 4.0: DONE
----------------------------------------
This crashes on Ruby 4.0

``` sh
$ 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.



-- 
https://bugs.ruby-lang.org/
______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:124756] [Ruby Bug#21865] Crash on signal raise

[Eregon (Benoit Daloze) via ruby-core]

Issue #21865 has been updated by 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?

----------------------------------------
Bug #21865: Crash on signal raise
https://bugs.ruby-lang.org/issues/21865#change-116362

* Author: dodecadaniel (Daniel Colson)
* Status: Closed
* ruby -v: 4.0.1
* Backport: 3.2: DONTNEED, 3.3: DONTNEED, 3.4: DONTNEED, 4.0: DONE
----------------------------------------
This crashes on Ruby 4.0

``` sh
$ 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.



-- 
https://bugs.ruby-lang.org/
______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:124762] [Ruby Bug#21865] Crash on signal raise

[jeremyevans0 (Jeremy Evans) via ruby-core]

Issue #21865 has been updated by 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

----------------------------------------
Bug #21865: Crash on signal raise
https://bugs.ruby-lang.org/issues/21865#change-116369

* Author: dodecadaniel (Daniel Colson)
* Status: Closed
* ruby -v: 4.0.1
* Backport: 3.2: DONTNEED, 3.3: DONTNEED, 3.4: DONTNEED, 4.0: DONE
----------------------------------------
This crashes on Ruby 4.0

``` sh
$ 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.



-- 
https://bugs.ruby-lang.org/
______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/