Move arity-checking into variable-arity method bodies #7751
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These paths are automatically arity-checked but only along the DynamicMethod.call path. This leaves direct Java calls and indy binding without arity checking. Given the impossibility of automatically arity-checking the direct call paths, it seems best to always arity-check inside variable-arity native methods. We may want to audit all such places and remove the automatic arity- checking happening in DynamicMethod.call(..., IRbuyObject[]) so we are always checking at the point of use, where the stack trace will reflect the method name, and without double-checking in both generated wrappers and in the method itself.
headius
force-pushed
the
more_indy_fixes
branch
2 times, most recently
from
21613ba
to
c782f00
Compare
In order to ensure that arity checks always happen along variable- arity paths, it makes sense for us to move the check into the method bodies rather than relying on the generated wrapper or the caller to do the check. This has several advantages: * Direct calls from Java will now be checked. * Direct calls from invokedynamic call sites will be checked. * The generated invokers will have less data and do less arity- checking. Splitting methods by arity will still enable direct calls to bypass the arity check. The pattern I used here, which we may want to adopt as a recommended pattern, is as follows: * If the method accesses the contents of the array direct or * If the method passes the array on to other code that accesses its contents directly without a check then * A manual arity-check should be performed before any other operations.
headius
force-pushed
the
more_indy_fixes
branch
from
c782f00
to
5565751
Compare
headius
changed the title
headius
added a commit
to headius/stringio
that referenced
this pull request
Apr 12, 2023
As part of jruby/jruby#7751 we are recommending that all extension code manually check the arity of incoming arguments to variable- arity methods, as in CRuby. This ensures that all call paths will be checked, including direct paths from Java or invokedynamic, and avoids array indexing errors in these situations.
This will be fixed once ruby/stringio#48 is merged and released.
headius
added a commit
to ruby/stringio
that referenced
this pull request
Apr 12, 2023
As part of jruby/jruby#7751 we are recommending that all extension code manually check the arity of incoming arguments to variable- arity methods, as in CRuby. This ensures that all call paths will be checked, including direct paths from Java or invokedynamic, and avoids array indexing errors in these situations.
headius
added a commit
to headius/jruby
that referenced
this pull request
Apr 12, 2023
|
This PR introduced the behavior change in 9.4.3.0 observed in #7851. Given that this breaks 3rd party extensions that work correctly on 9.4.2.0 and below, I recommend rolling this back in 9.4.3.1 so that JRuby users using these extensions are not negatively impacted by an upgrade from an older version of JRuby. If there is a long term need for this breaking change, it should be rescheduled and introduced in a major version bump. |
headius
added a commit
to headius/jruby
that referenced
this pull request
Jul 17, 2023
These were all moved to manual arity-checking in jruby#7751, but that led to breakage when third-party extensions had varargs paths that did not check arity manually (see jruby#7851). Instead we restore the default to auto arity-check with this flag provided for opting out (jruby#7680).
headius
added a commit
to headius/jruby
that referenced
this pull request
Jul 17, 2023
These were all moved to manual arity-checking in jruby#7751, but that led to breakage when third-party extensions had varargs paths that did not check arity manually (see jruby#7851). Instead we restore the default to auto arity-check with this flag provided for opting out (jruby#7680).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During testing of indy-based compilation, it became obvious that we need to ensure variable-arity method bodies do their own arity-checking, so that direct calls from indy or Java do not cause array index errors. This PR adds such arity-checking to all such methods, following these rules:
Methods should do arity-checking manually if:
Methods do not need a manual check if they do not have minimums or maximums, or if they only pass their argument list to another variable-arity method that does a manual check.
This PR also removes arity-checking from the generated invokers, to avoid double-checking.
The main risk from this change lies in third-party extension code that has variable-arity methods but no manual checking. Passing incorrect numbers of arguments to these methods may trigger an array index error, as it would segfault in MRI (which also does not automatically check arity).