Fix CALL_KEYWORD_EMPTY leaking from (...) forwards to native methods#9501
Conversation
…tive method
A literal keyword argument is silently dropped (passed as a trailing
positional Hash) when the immediately-preceding statement is a `(...)`
argument-forwarding call to a native method that does not reset callInfo.
This raises a bogus `ArgumentError: wrong number of arguments (given 2,
expected 1)`.
The new test in test/jruby/test_call_info.rb fails on current HEAD and
documents the bug; the fix is described in the follow-up commit.
== Symptom (real world) ==
Found via sigstore-ruby on jruby 10.x HEAD. Sigstore::SBundle is a
DelegateClass; its #expected_tlog_entry(hashed_input, verifier_pem:) was
called as `bundle.expected_tlog_entry(hashed_input, verifier_pem:)` from
inside Verifier#find_rekor_entry, where verifier_pem was computed as
`signing_key&.public_to_pem || bundle.leaf_certificate&.to_pem`. The
keyword was dropped and the call raised ArgumentError. Switching the call
site from keyword to positional worked around it.
== Root cause ==
Two interacting pieces in the IR runtime:
1. org.jruby.runtime.Helpers#argsPush (Helpers.java:2334) and
IRRuntimeHelpers#isHashEmpty / #irSplat set ThreadContext CALL_KEYWORD_EMPTY
(1<<3) when a call is built with an *empty* keyword-rest. A Ruby >= 2.7
`(...)` forward (which is what Forwardable#def_delegator generates, and
what Certificate#to_pem -> OpenSSL is) builds the forwarded call with an
empty `**` rest, so it sets CALL_KEYWORD_EMPTY.
2. IRRuntimeHelpers#setCallInfo (IRRuntimeHelpers.java:938-940):
public static void setCallInfo(ThreadContext context, int flags) {
// FIXME: This may propagate empty more than the current call? ...
context.callInfo = (context.callInfo & CALL_KEYWORD_EMPTY) | flags;
}
It *preserves* a leftover CALL_KEYWORD_EMPTY bit into the next call's
callInfo (there is already a FIXME about exactly this). callInfo is meant
to be transient between a caller and its callee: the callee clears it in
IRRuntimeHelpers#receiveKeywords via ThreadContext.resetCallInfo. But a
*native* forward target (e.g. jruby-openssl's
OpenSSL::X509::Certificate#to_pem, a generated $INVOKER$i$0$0$ method) has
no IR recv_kw, so it never resets callInfo. The CALL_KEYWORD_EMPTY bit
therefore survives the forward call.
When the very next call is a keyword call (CALL_KEYWORD, 1<<1), setCallInfo
ORs in CALL_KEYWORD while keeping the stale CALL_KEYWORD_EMPTY. In
receiveKeywords -> shouldHandleKwargs, `!keywordsEmpty(callInfo)` is then
false (the EMPTY bit is set), so keyword processing is skipped and the hash
is left as a trailing positional argument -> arity error.
== Diagnosis trail (verified by running, not guessed) ==
- Reproduces under `-X-C` (pure interpreter): an IR-runtime bug, not JIT.
- Caller IR is identical to passing cases: `call_2o(recv, arg, %hash, flags:2)`
(flags 2 = CALL_KEYWORD). Callee IR is correct: recv_kw(acceptsKeywords:true),
check_arity(required:1). So neither side is individually wrong.
- It is *not* receiver/class specific. A plain class with the same keyword
method also fails when the call site is poisoned. The discriminator is the
preceding value computation: a literal value passes; a value obtained from a
`(...)` forward to a native method fails.
- Confirmed instrumenting Helpers#argsPush, IRRuntimeHelpers#{isHashEmpty,
irSplat,setCallInfo} with prints: for the failing path argsPush sets EMPTY,
then setCallInfo logs "preserving CALL_KEYWORD_EMPTY into CALL_KEYWORD call
(prev=8 flags=...)" for the next keyword call. A passing path (e.g. forward
to StringIO#string) shows callInfo already reset (prev=0) before that call.
- Only "first call" through a given `(...)` forward call site leaks; once the
call site caches a non-array fast path it stops building the empty kwrest.
- jruby-openssl native methods (to_pem/to_der/version) trigger it; the core
natives tried (String#upcase, Integer#even?, Array#length, StringIO#string,
Digest#digest_length, BigDecimal#sign, ...) do not, because their dispatch
leaves callInfo clean. So a fully gem-free repro needs `openssl` (bundled
with JRuby); ~30 core/extension natives were tried and none reproduce.
== Minimal reproduction (gem-free; openssl ships with JRuby) ==
require "openssl"
cert = OpenSSL::X509::Certificate.new # any signed cert
def forward(o, ...) = o.to_pem(...) # `(...)` forward to a native method
def kwm(a, k: nil) = [a, k]
v = forward(cert)
kwm("HI", k: v) # => ArgumentError: wrong number of arguments (given 2, expected 1)
Fixes the bug captured by the failing test in the parent commit
(test/jruby/test_call_info.rb#test_forward_dots_to_native_method_does_not_leak):
a literal keyword argument was silently dropped (passed as a trailing
positional Hash) when the immediately-preceding statement was a `(...)`
argument-forwarding call to a native method, raising a bogus
`ArgumentError: wrong number of arguments (given N, expected N-1)`.
== Root cause ==
ThreadContext#callInfo is transient state passed from a caller's argument
build to its callee. CALL_KEYWORD_EMPTY (1<<3) is set dynamically while
building a call's arguments (Helpers#argsPush, IRRuntimeHelpers#isHashEmpty
and #irSplat) when a keyword-rest or splat turns out to be empty; it must
survive into the call so the callee treats the kwargs as explicitly empty.
IRRuntimeHelpers#setCallInfo carried that bit forward unconditionally:
context.callInfo = (context.callInfo & CALL_KEYWORD_EMPTY) | flags;
callInfo is normally cleared on the callee side in #receiveKeywords via
ThreadContext.resetCallInfo, but a native callee (a generated $INVOKER with
no recv_kw step) does not clear it. A Ruby >= 2.7 `(...)` forward to such a
native method (e.g. jruby-openssl OpenSSL::X509::Certificate#to_pem, whose
precompiled invoker predates and bypasses the InvocationMethodFactory wipe)
builds an empty keyword-rest, sets CALL_KEYWORD_EMPTY, and leaves it set.
The next keyword call then inherited the stale bit and skipped keyword
processing.
== Fix ==
A legitimate CALL_KEYWORD_EMPTY is only ever produced for a call that
itself passes a keyword-rest or splat. So carry it forward only when this
call's flags indicate CALL_KEYWORD_REST or CALL_SPLATS; otherwise a leftover
bit from a previous call is stale and is dropped. This preserves correct
empty-`**`/splat delegation (which approach of setting `callInfo = flags`
outright would regress) while stopping the leak into unrelated keyword calls.
== Verification ==
export JAVA_HOME=~/.asdf/installs/java/temurin-21.0.11+10.0.LTS
./mvnw -Pbootstrap clean install -DskipTests -q
./bin/jruby -Itest test/jruby/test_call_info.rb # 34/34, JIT
./bin/jruby -X-C -Itest test/jruby/test_call_info.rb # 34/34, interpreted
spec/ruby/language keyword/method/block specs show no regressions against a
rebuilt baseline.
|
@enebo If you get a chance to review I would appreciate extra eyes. It seems right to me. |
|
@segiddins I'm wondering if there's a more generic test in MRI we could enable/un-exclude or a test we could add to ruby/spec that doesn't require openssl specifically. |
|
There probably is! I just really struggled to get another native-implemented method to reproduce -- the reduction already took me (and my french friend) a while to narrow in on. I'm happy for either the implementation of the test to change, I just figured it would be more helpful to share both than just an issue that required running a whole other test suite |
|
This seems good to me. We went through and audited setCallInfo a while back due to us using that method incorrectly someplace. The documentation along in this PR is also good as it explains the odd nature of empty. @headius this probably should be cherry-picked back to 10.0. @segiddins If you do find a more generic (or we do) then we can eliminate this test and move another test into ruby/spec. |
think this should go towards jruby-10.0 branch as well, there were similar CALL_INFO leak fixes in 10.0.5.0 |
Problem
A literal keyword argument is silently dropped — passed as a trailing positional Hash — when the immediately-preceding statement is a
(...)argument-forwarding call to a native method, raising a bogusArgumentError: wrong number of arguments (given N, expected N-1).Found via sigstore-ruby on JRuby 10.x: a
bundle.expected_tlog_entry(hashed_input, verifier_pem:)call raisedArgumentErroronly because the preceding statement forwarded(...)to a native method (OpenSSL::X509::Certificate#to_pem).Minimal reproduction (gem-free;
opensslships with JRuby):Reproduces under
-X-Cas well, so it is an IR-runtime bug, not JIT-specific.Root cause
ThreadContext#callInfois transient state passed from a caller's argument build to its callee.CALL_KEYWORD_EMPTY(1<<3) is set dynamically while building a call's arguments (Helpers#argsPush,IRRuntimeHelpers#isHashEmpty/#irSplat) when a keyword-rest or splat turns out empty, and must survive into the call so the callee treats the kwargs as explicitly empty.IRRuntimeHelpers#setCallInfocarried that bit forward unconditionally:callInfois normally cleared on the callee side in#receiveKeywordsviaresetCallInfo, but a native callee (a generated$INVOKERwith norecv_kwstep) does not clear it — and the precompiled jruby-openssl invoker predates and bypasses theInvocationMethodFactorywipe. So the empty bit set by the(...)forward survives, and the next keyword call inherits it and skips keyword processing.Fix
A legitimate
CALL_KEYWORD_EMPTYis only ever produced for a call that itself passes a keyword-rest or splat. So carry it forward only when this call's flags indicateCALL_KEYWORD_RESTorCALL_SPLATS; otherwise a leftover bit from a previous call is stale and is dropped. This preserves correct empty-**/splat delegation — which settingcallInfo = flagsoutright would regress — while stopping the leak into unrelated keyword calls.Commits
test_call_info.rb#test_forward_dots_to_native_method_does_not_leak, documenting the bug (fails on the base).setCallInfochange above.Verification
test/jruby/test_call_info.rb: 34/34 pass under both JIT and-X-C.spec/ruby/languagekeyword/method/block specs: no regressions vs. a rebuilt baseline (notably the empty-**delegation specs still pass).