Bug #11409: {instance,module}_eval(&:foo) segfaults since r51243.

• Status: Closed
• Assignee: ko1 (Koichi Sasada)
• Target version: -
• ruby -v: ruby 2.3.0dev (2015-08-02 trunk 51469) [x86_64-linux]

[0x0dea (D.E. Akers)]

The segfault only occurs when the argument is a #to_proc'd Symbol, and the receiver needn't actually respond to the named method.

This bug was introduced in a rather large patch, and should almost certainly be fixed in one of the files modified therein. That said, I've discovered that removing the call to rb_block_clear_env_self() in sym_to_proc() prevents the segfault, as does setting env->env[0] to Qfalse rather than Qnil in rb_block_clear_env_self(). Neither of those is a proper fix, of course, but I hope this information may be of use to somebody more intimately familiar with Ruby's internals.

[0x0dea (D.E. Akers)]

Upon further investigation, I've discovered why replacing Qnil with Qfalse prevents the crash.

static rb_cref_t *
check_cref(VALUE obj, int can_be_svar)
{
    if (obj == Qfalse) return NULL;

The prelude of check_cref() only checks whether obj is Qfalse; since rb_block_clear_env_self() uses Qnil, check_cref() continues by calling imemo_type() on Qnil, which eventually leads to Qnil being dereferenced and the interpreter crashing.

The fix is remarkably simple:

- if (obj == Qfalse) return NULL;
+ if (!RTEST(obj)) return NULL;

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Assigned
• Assignee set to ko1 (Koichi Sasada)

[ko1 (Koichi Sasada)]

Thank you for reporting a bug and your detailed analysis.
They help me very much.

In this case, I believe `obj' should be Qfalse or T_IMEMO objects. So that we clear by Qfalse intead of Qnil.

Reproducible code:

class Foo
end

Foo.instance_eval(&:p)

A patch:

Index: proc.c
===================================================================
--- proc.c	(revision 51472)
+++ proc.c	(working copy)
@@ -670,7 +670,7 @@ rb_block_clear_env_self(VALUE proc)
     rb_env_t *env;
     GetProcPtr(proc, po);
     GetEnvPtr(rb_vm_proc_envval(po), env);
-    env->env[0] = Qnil;
+    env->env[0] = Qfalse;
     return proc;
 }
 

[0x0dea (D.E. Akers)]

Koichi Sasada wrote:

Thank you for reporting a bug and your detailed analysis.
They help me very much.

I am happy to have been of some assistance.

I believe `obj' should be Qfalse...

It's true that changing that Qnil to Qfalse is the "cleaner" fix this time around, but I think using RTEST() to check for both is slightly more future-proof. I've submitted a pull request which takes the latter approach, in addition to adding a couple of tests to ensure something like this will be caught in future. I shall take no offense if your much greater familiarity with YARV tells you that using Qfalse there is the better solution.

[ko1 (Koichi Sasada)]

It's true that changing that Qnil to Qfalse is the "cleaner" fix this time around, but I think using RTEST() to check for both is slightly more future-proof. I've submitted a pull request which takes the latter approach, in addition to adding a couple of tests to ensure something like this will be caught in future. I shall take no offense if your much greater familiarity with YARV tells you that using Qfalse there is the better solution.

I changed data structures around these part to have NULL (== Qfalse). If not, then it should be a bug. Using RTEST() hides these bugs.

I will check your test and introduce to our tests.
I will also add some assertions in codes to check Qnil.

Thaks,
Koichi

[ko1 (Koichi Sasada)]

• Status changed from Assigned to Closed

Applied in changeset r51480.

---

• proc.c (rb_block_clear_env_self): clear by Qfalse intead of Qnil.
  [Bug #11409]
• test/ruby/test_eval.rb: add tests for this issue,
  written by @0x0dea.
  https://github.com/ruby/ruby/pull/988