Issue #22335 has been updated by eapache_opslevel (Evan Huus). Thank you for the prompt fix. Not an area of ruby I'm very familiar with, but I think perhaps we should also be extending sp in `vm_callee_setup_arg` and `vm_call_iseq_setup_kwparm_kwarg`? Getting rid of the method dispatch fixes the symptom, but any future change in these paths which might trigger a dispatch would cause the same issue again. `setup_parameters_complex` already extends `sp` in roughly the same case. ---------------------------------------- Bug #22335: Stack corruption with >31 keyword args and refined Integer#== https://bugs.ruby-lang.org/issues/22335#change-119135 * Author: eapache_opslevel (Evan Huus) * Status: Closed * ruby -v: 4.0.7 * Backport: 3.3: REQUIRED, 3.4: REQUIRED, 4.0: REQUIRED ---------------------------------------- This is a messy one that originally surfaced as a flaky test in our Rails app, and required a lot of AI-assisted debugging to track down. This bug appears in the latest 4.0.7, though all of the original investigation was done against 3.3.11. If you have: - a method that takes only keyword args - has >31 keyword args - has two call-time-evaluated default values (e.g. `{}`) at argument index >=31 - running in a Ruby process that has refined or redefined `Integer#==` at any point Then you get inconsistent stack corruption depending on your process's hash salt. The mechanism is nasty: - In a method with only keyword args, `vm_callee_setup_arg()` and `vm_call_iseq_setup_kwparm_kwarg()` call `args_setup_kw_parameters()` with `klocals = argv + kw_param->bits_start - kw_param->num`, i.e. **above `cfp->sp`**, without first extending `cfp->sp` over those locals - `args_setup_kw_parameters()` uses a full hash, keyed by integer, for arguments beyond index 31 - When `args_setup_kw_parameters()` sets a key in that hash, and that key collides with another in the ar_hint that small hashes use for a linear scan (not uncommon for a tiny hash, but still salt-dependent), it triggers an integer comparison - The refinement on `Integer#==` prevents that comparison from taking the fast path and turns it into a real C frame - Which gets pushed onto the stack, when the stack pointer hasn't been extended - Thus overwriting the other args and producing garbage The net effect is that the called method sees garbage values for the overwritten arguments. I've attached a reproducing script, which is unfortunately complex, as it needs to do some work to intentionally hit the correct hash collision depending on the process's salt. A simpler static version doesn't reproduce reliably enough to be useful. Even this version will occasionally get a hash salt with no collisions and fail to reproduce, though that seems pretty rare on my machine. ---Files-------------------------------- repro.rb (1.51 KB) -- https://bugs.ruby-lang.org/

Issue #22335 has been updated by alanwu (Alan Wu).

Not an area of ruby I'm very familiar with, but I think perhaps we should also be extending sp in vm_callee_setup_arg and vm_call_iseq_setup_kwparm_kwarg?

These don't extend SP because they're fast paths that try to do minimal work. Now, for correctness, they rely on implicit constraints such as "no ruby method calls possible within", as found here, but that is not a sign that we should change their design. `setup_parameters_complex` is the generic case that handles all possible situations, so it needs to extend SP because of that. You can think of the fast paths as sectioned off slices of `setup_parameters_complex`. ---------------------------------------- Bug #22335: Stack corruption with >31 keyword args and refined Integer#== https://bugs.ruby-lang.org/issues/22335#change-119195 * Author: eapache_opslevel (Evan Huus) * Status: Closed * ruby -v: 4.0.7 * Backport: 3.3: REQUIRED, 3.4: DONE, 4.0: REQUIRED ---------------------------------------- This is a messy one that originally surfaced as a flaky test in our Rails app, and required a lot of AI-assisted debugging to track down. This bug appears in the latest 4.0.7, though all of the original investigation was done against 3.3.11. If you have: - a method that takes only keyword args - has >31 keyword args - has two call-time-evaluated default values (e.g. `{}`) at argument index >=31 - running in a Ruby process that has refined or redefined `Integer#==` at any point Then you get inconsistent stack corruption depending on your process's hash salt. The mechanism is nasty: - In a method with only keyword args, `vm_callee_setup_arg()` and `vm_call_iseq_setup_kwparm_kwarg()` call `args_setup_kw_parameters()` with `klocals = argv + kw_param->bits_start - kw_param->num`, i.e. **above `cfp->sp`**, without first extending `cfp->sp` over those locals - `args_setup_kw_parameters()` uses a full hash, keyed by integer, for arguments beyond index 31 - When `args_setup_kw_parameters()` sets a key in that hash, and that key collides with another in the ar_hint that small hashes use for a linear scan (not uncommon for a tiny hash, but still salt-dependent), it triggers an integer comparison - The refinement on `Integer#==` prevents that comparison from taking the fast path and turns it into a real C frame - Which gets pushed onto the stack, when the stack pointer hasn't been extended - Thus overwriting the other args and producing garbage The net effect is that the called method sees garbage values for the overwritten arguments. I've attached a reproducing script, which is unfortunately complex, as it needs to do some work to intentionally hit the correct hash collision depending on the process's salt. A simpler static version doesn't reproduce reliably enough to be useful. Even this version will occasionally get a hash salt with no collisions and fail to reproduce, though that seems pretty rare on my machine. ---Files-------------------------------- repro.rb (1.51 KB) -- https://bugs.ruby-lang.org/