New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make rb_scan_args handle keywords more similar to Ruby methods #2460
Merged
jeremyevans
merged 5 commits into
ruby:master
from
jeremyevans:rb_scan_args-keyword-argument-separation
Sep 25, 2019
Merged
Make rb_scan_args handle keywords more similar to Ruby methods #2460
jeremyevans
merged 5 commits into
ruby:master
from
jeremyevans:rb_scan_args-keyword-argument-separation
Sep 25, 2019
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
jeremyevans
force-pushed
the
rb_scan_args-keyword-argument-separation
branch
2 times, most recently
from
September 15, 2019 05:18
e8f4687
to
aa95774
Compare
Cfuncs that use rb_scan_args with the : entry suffer similar keyword argument separation issues that Ruby methods suffer if the cfuncs accept optional or variable arguments. This makes the following changes to : handling. * If optional or variable arguments are also requested, treat the option handling similar to Ruby keyword option handling (keyword mode). Otherwise, if only mandatory arguments are requested other than this option hash, treat it similarly to an optional argument (hash mode). * In keyword mode, do not look for an option hash if empty keywords are provided. For backwards compatibility, in both modes, treat an empty keyword splat as a empty mandatory positional hash argument, but emit a a warning, as this behavior should be removed in Ruby 3. The argument number check needs to be moved lower so it can correctly handle an empty positional argument being added. * In both modes, if the last argument is nil and it is necessary to treat it as an option hash in order to make sure all arguments are processed, continue to treat the last argument as the option hash. Emit a warning in this case, as this behavior should be removed in Ruby 3. * In both modes, if splitting the option hash into two hashes, issue a warning, as we will not be splitting hashes in Ruby 3. * In keyword mode, if keyword arguments are not provided and the last argument is a hash, emit a warning, as in Ruby 3 it will be treated as a positional argument instead of the option hash. * In keyword mode, if the keyword argument is required to fill a mandatory positional argument, continue to do so, but emit a warning as this behavior will be going away in Ruby 3. * In keyword mode, if keyword arguments are provided an the last argument is not a hash, that indicates something wrong. This can happen if a cfunc is calling rb_scan_args multiple times, and providing arguments that were not passed to it from Ruby. Such functions will need to be updated, and there is a new feature to handle this. Instead of calling rb_keyword_given_p() and rb_empty_keyword_given_p(), allow an 'k', 'e', or 'n' prefix to the rb_scan_args string: * k: Treat as if the keyword given flag is set (last argument should be a hash. * e: Treat as if the empty keyword given flag is set (in keyword mode, do not look for a last positional hash). * n: In keyword mode, assume the call did not set the keyword or empty keyword flags, but do not issue a warning if the last argument is a hash that is treated as keywords. For external Cfuncs, you can be backwards compatible with Ruby <2.7 by using a macro. On 2.7+, the macro can be defined to "k", "e", or "n", and on <2.7, it can be defined to ""; This commit fixes all warnings caused by the changes above. It switches some function calls to *_kw versions with appropriate kw_splat flags. If delegating arguments, RB_PASS_CALLED_KEYWORDS is used. If creating new arguments, RB_PASS_KEYWORDS is used if the last argument is a hash to be treated as keywords. In open_key_args in io.c, use the 'n' prefix to rb_scan_args. In this case, the arguments provided come from another C function, not Ruby. The last argument may or may not be a hash, so we can't set keyword argument mode. However, if it is a hash, we don't want to warn when treating it as keywords. In Ruby files, make sure to appropriately use keyword splats or literal keywords when calling Cfuncs that now issue keyword argument separation warnings through rb_scan_args. Also, make sure not to pass nil in place of an option hash. Work around Kernel#warn warnings due to problems in the Rubygems override of the method. There is an open pull request to fix these issues in Rubygems, but part of the Rubygems tests for their override fail on ruby-head due to rb_scan_args not recognizing empty keyword splats, which this commit fixes.
Remove the "k", "e", and "n" flags support in rb_scan_args. Instead, rb_scan_args_kw takes a kw_flag first argument, which can be used where the "k", "e", and "n" flags could be used previously. Implementation wise, this is kind of a pain, because rb_scan_args takes a variable number of arguments. In order to not duplicate all the code, the function internals need to be split into two functions taking a va_list, and to avoid passing in a ton of arguments, a single struct argument is used to handle the variables previously shared.
jeremyevans
force-pushed
the
rb_scan_args-keyword-argument-separation
branch
from
September 22, 2019 06:48
aa95774
to
ac27ee9
Compare
Fix various issues in the core, ext, lib, and tests to deal with warnings caused by this.
flori/json#424 claims that this PR fixed [CVE-2020-10663]. True? Was that intent part of this PR? Or was the fix serendipitous? |
I might be mistaken, but I think it's the change to |
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.
Cfuncs that use rb_scan_args with the : entry suffer similar keyword
argument separation issues that Ruby methods suffer if the cfuncs
accept optional or variable arguments.
This makes the following changes to : handling.
If optional or variable arguments are also requested, treat
the option handling similar to Ruby keyword option handling
(keyword mode). Otherwise, if only mandatory arguments are
requested other than this option hash, treat it similarly to
an optional argument (hash mode).
In keyword mode, do not look for an option hash if empty
keywords are provided. For backwards compatibility, in both modes,
treat an empty keyword splat as a empty mandatory positional hash
argument, but emit a a warning, as this behavior should be removed
in Ruby 3. The argument number check needs to be moved lower so it
can correctly handle an empty positional argument being added.
In both modes, if the last argument is nil and it is necessary to
treat it as an option hash in order to make sure all arguments are
processed, continue to treat the last argument as the option hash.
Emit a warning in this case, as this behavior should be removed
in Ruby 3.
In both modes, if splitting the option hash into two hashes, issue
a warning, as we will not be splitting hashes in Ruby 3.
In keyword mode, if keyword arguments are not provided and the
last argument is a hash, emit a warning, as in Ruby 3 it will be
treated as a positional argument instead of the option hash.
In keyword mode, if the keyword argument is required to fill a
mandatory positional argument, continue to do so, but emit a
warning as this behavior will be going away in Ruby 3.
In keyword mode, if keyword arguments are provided an the last
argument is not a hash, that indicates something wrong. This can
happen if a cfunc is calling rb_scan_args multiple times, and
providing arguments that were not passed to it from Ruby. Such
functions will need to be updated, and there is a new feature
to handle this. Instead of calling rb_keyword_given_p() and
rb_empty_keyword_given_p(), allow an 'k', 'e', or 'n' prefix
to the rb_scan_args string:
k: Treat as if the keyword given flag is set (last argument
should be a hash.
e: Treat as if the empty keyword given flag is set (in
keyword mode, do not look for a last positional hash).
n: In keyword mode, assume the call did not set the keyword
or empty keyword flags, but do not issue a warning if the
last argument is a hash that is treated as keywords.
For external Cfuncs, you can be backwards compatible with Ruby
<2.7 by using a macro. On 2.7+, the macro can be defined to
"k", "e", or "n", and on <2.7, it can be defined to "";
This commit fixes all warnings caused by the changes above.
It switches some function calls to *_kw versions with appropriate
kw_splat flags. If delegating arguments, RB_PASS_CALLED_KEYWORDS
is used. If creating new arguments, RB_PASS_KEYWORDS is used if
the last argument is a hash to be treated as keywords.
In open_key_args in io.c, use the 'n' prefix to rb_scan_args.
In this case, the arguments provided come from another C
function, not Ruby. The last argument may or may not be a hash,
so we can't set keyword argument mode. However, if it is a
hash, we don't want to warn when treating it as keywords.
In Ruby files, make sure to appropriately use keyword splats
or literal keywords when calling Cfuncs that now issue keyword
argument separation warnings through rb_scan_args. Also, make
sure not to pass nil in place of an option hash.
Work around Kernel#warn warnings due to problems in the Rubygems
override of the method. There is an open pull request to fix
these issues in Rubygems, but part of the Rubygems tests for
their override fail on ruby-head due to rb_scan_args not
recognizing empty keyword splats, which this commit fixes.