Bug #22335: Stack corruption with >31 keyword args and refined Integer#==

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: 4.0.7
• Backport: 3.3: REQUIRED, 3.4: DONE, 4.0: REQUIRED

[eapache_opslevel (Evan Huus)]

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.

[eapache_opslevel (Evan Huus)]

• Description updated (diff)

[eapache_opslevel (Evan Huus)]

• Description updated (diff)

[eapache_opslevel (Evan Huus)]

• Description updated (diff)

[nobu (Nobuyoshi Nakada)]

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

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

Applied in changeset git|d87ffb271a80fb108de3465d12ada829f3c130af.

---

[Bug #22335] Use identity hashes for keyword argument tracking

With more than 31 keyword arguments, tracking unevaluated defaults
uses an integer-keyed Hash whose collisions can invoke methods when
Integer#== is refined or redefined, corrupting argument locals above
cfp->sp. Use identity hashes to prevent these calls during argument
setup and checkkeyword execution.

Making rb_any_cmp compare special constants directly would also
bypass redefined eql? methods in ordinary hashes. Limit identity
comparison to the internal keyword tracking hashes to preserve that
behavior for now.

[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.

[nagachika (Tomoyuki Chikanaga)]

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

ruby_3_4 41e214d7100100a43c469e2628403d6916d1f0f8 merged revision(s) d87ffb271a80fb108de3465d12ada829f3c130af.

[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.