Skip to content
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

Throw a runtime error if #binding is called from a C extensions module (non-ruby frame) #3436

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

manefz
Copy link
Contributor

@manefz manefz commented Feb 2, 2024

Part of #3039

Fixes [easy, java] Kernel#binding raises RuntimeError if called from a non-Ruby frame (such as a method defined in C). [[Bug #18487](https://bugs.ruby-lang.org/issues/18487)]

Following what was done in Cruby 3.2 PR here
Raises an error if the frame that is called from is not Ruby.

Co-authored with @rwstauner

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 2, 2024
C ext module (non-ruby frame)

Co-authored-by: Randy Stauner <[email protected]>
@@ -334,6 +334,12 @@ RubyBinding binding(Frame callerFrame, Object self, Object[] rubyArgs, RootCallT
@Cached(
value = "getAdoptedNode(this).getEncapsulatingSourceSection()",
allowUncached = true, neverDefault = false) SourceSection sourceSection) {
final InternalMethod method = RubyArguments.tryGetMethod(callerFrame);
if (method == null || method.getDeclaringModule().toString().equals("Truffle::CExt")) {
Copy link
Collaborator

@rwstauner rwstauner Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference it seems tryGetMethod is looking up the ruby method that is the context of the frame calling binding.
So in the case of the test

-> { @b.get_binding }.should raise_error(RuntimeError)

is backed by
static VALUE binding_spec_get_binding(VALUE self) {
return rb_funcall(self, rb_intern("binding"), 0);

and the method of the caller frame then points to CExt#rb_funcallv
def rb_funcallv(recv, meth, argv)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure in what scenario tryGetMethod might return null

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably also a better check here than doing a string comparison on the method's module, but we hadn't found anything.
Any pointers on what else we might be able to use for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is org.truffleruby.core.CoreLibrary#truffleCExtModule and then it can be compared with ==.

But comparing on the module is hacky, because it means every Truffle::CExt method can no longer access binding.
Which seems a problem notably for:

def rb_gv_set(name, value)
binding.eval("#{name} = value")
end

That can be worked around, but it shows it's rather hacky.

I'm not sure what's a good solution for detecting C ext methods, maybe/probably we should define Truffle::CExt#rb_define_method in Java eventually. Or find another way to mark those.

But I think for now a more useful check would be to check if sourceSection == CoreLibrary.JAVA_CORE_SOURCE_SECTION, which means a method defined in Java.
Getting a binding from there will always be empty, which seems not useful, so that seems the corresponding case to CRuby having methods defined in C and for which no meaningful binding can be given.

An example test for that is:

$ ruby -e 'p method(:binding).call'
-e:1:in `binding': Cannot create Binding object for non-Ruby caller (RuntimeError)

Currently on TruffleRuby it's:

$ ruby -e 'p method(:binding).call'
#<Binding:0xc8>
$ ruby -e 'p method(:binding).call.local_variables'
[]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW for the existing spec in spec/ruby/optional/capi/binding_spec.rb I would think we already raise an error for that case, because the callerFrame should be null in that case since it is a call from LLVM/Sulong.
But maybe there is something in between which means there is a frame somehow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the master branch this spec fails with

CApiBindingSpecs Kernel#binding raises when called from C FAILED
Expected RuntimeError but no exception was raised (#<Binding:0x368> was returned)

I loaded a debugger to check the sourceSection and it is not the same as CoreLibrary.JAVA_CORE_SOURCE_SECTION.

This is what I see:
image

If I use a different ruby script that calls binding from ruby code the sourceSection here doesn't have any different properties from the above (other than the path) that I can see 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so the SourceSection there is

Primitive.send_argv_without_cext_lock(recv, meth, argv, nil)

So let's not try to pass this specific spec for now, because it would require changing rb_funcallv which is out of scope.

But instead let's add another spec, using method(:binding).call, and that should raise the RuntimeError, and that one we can use the check for JAVA_CORE_SOURCE_SECTION.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing worth trying to pass that C API spec would be to change SendWithoutCExtLockBaseNode to call callWithFrameAndBlock without a frame (so with null instead). But that may have other side effects which might be undesirable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ but the frame was added back in 6fbb9ee probably for 796df16.
And 9c0f7ce seems also related.
So I think best to not touch that for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3449 Is an alternative implementation that uses something similar to the commit you pointed to 9c0f7ce

@@ -334,6 +334,12 @@ RubyBinding binding(Frame callerFrame, Object self, Object[] rubyArgs, RootCallT
@Cached(
value = "getAdoptedNode(this).getEncapsulatingSourceSection()",
allowUncached = true, neverDefault = false) SourceSection sourceSection) {
final InternalMethod method = RubyArguments.tryGetMethod(callerFrame);
if (method == null || method.getDeclaringModule().toString().equals("Truffle::CExt")) {
throw new RaiseException(getContext(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use an InlinedBranchProfile errorProfile here, so this branch is only explored by the JIT if it ever happened? (that way it can also optimize better because it knows there is no throw)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement. shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants