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.

Sign up for GitHub

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

Jump to bottom

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
+7 −1
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ Compatibility:
* Do not autosplat a proc that accepts a single positional argument and keywords (#3039, @andrykonchin).
* Support passing anonymous * and ** parameters as method call arguments (#3039, @andrykonchin).
* Handle either positional or keywords arguments by default in `Struct.new` (#3039, @rwstauner).
* Make `Kernel#binding` raise a RuntimeError when called from a non-Ruby frame (such as a method defined in C) (#3039, @rwstauner, @manefz)

Performance:

Expand Down
1 change: 0 additions & 1 deletion spec/tags/optional/capi/binding_tags.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
fails:CApiBindingSpecs Kernel#binding gives the top-most Ruby binding when called from C
fails:CApiBindingSpecs Kernel#binding raises when called from C
6 changes: 6 additions & 0 deletions src/main/java/org/truffleruby/core/kernel/KernelNodes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

truffleruby/spec/ruby/optional/capi/binding_spec.rb

Line 14 in 0c3f184

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

is backed by

truffleruby/spec/ruby/optional/capi/ext/binding_spec.c

Lines 8 to 9 in 0c3f184

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

truffleruby/lib/truffle/truffle/cext.rb

Line 1059 in 0c3f184

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:

truffleruby/lib/truffle/truffle/cext.rb

Lines 2039 to 2041 in 610cea3

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

truffleruby/lib/truffle/truffle/cext.rb

Line 1060 in 677ac08

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

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)

coreExceptions().runtimeError("You cannot call Kernel#Binding from a non-ruby frame", this));

}
needCallerFrame(callerFrame, target);
return BindingNodes.createBinding(getContext(), getLanguage(), callerFrame.materialize(), sourceSection);
}
Expand Down
Loading