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

Order of prepends and extends can break super #4531

Closed
headius opened this issue Mar 15, 2017 · 6 comments
Closed

Order of prepends and extends can break super #4531

headius opened this issue Mar 15, 2017 · 6 comments

Comments

@headius
Copy link
Member

@headius headius commented Mar 15, 2017

In debugging the behavior of CGI::Escape (a C ext added in Ruby 2.3) I discovered problems with the superclass logic surrounding prepended and extended modules.

Specifically, the following example will blow out the stack:

module X; end
class Foo; extend X; end
module Y; def blah; super; end; end
module X; prepend Y; end
class Foo; extend Y; end; Foo.blah

Small changes in order here will work properly, producing an error about "blah" not being available on Foo (for the super call).

For example, this works:

module Y; def blah; super; end; end
module X; prepend Y; end
class Foo; extend X; end
class Foo; extend Y; end; Foo.blah

For the moment I will modify the order in which these operations happen in the cgi library to get the related tests passing. Specifically, these fail:

test/ruby/cgi/test_cgi_util.rb

  Encoding.list.each do |enc|
    begin
      escaped = "'&"><".encode(enc)
      unescaped = "'&\"><".encode(enc)
    rescue Encoding::ConverterNotFoundError
      next
    else
      define_method("test_cgi_escapeHTML:#{enc.name}") do
        assert_equal(escaped, CGI::escapeHTML(unescaped))
      end
      define_method("test_cgi_unescapeHTML:#{enc.name}") do
        assert_equal(unescaped, CGI::unescapeHTML(escaped))
      end
    end
  end

Any non-ascii passed to these new optimized methods will hit the problem.

I believe this affects JRuby 9.1.8.0.

@headius headius added this to the JRuby 9.1.9.0 milestone Mar 15, 2017
headius added a commit that referenced this issue Mar 15, 2017
headius added a commit that referenced this issue Mar 15, 2017
@headius
Copy link
Member Author

@headius headius commented Mar 15, 2017

The related tests were added to MRI in ruby/ruby@8e46f40.

@headius
Copy link
Member Author

@headius headius commented Mar 15, 2017

The MRI commit in question does not appear to have been backported to the 2.3 branch, but I have cherry-picked this change there anyway.

@headius
Copy link
Member Author

@headius headius commented May 16, 2018

This is still a problem in 9.2, and likely needs my work on the super_fixes branch to work properly. However there are some edges to work out in that branch and it won't be ready for 9.2. Bumping.

@headius headius modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 16, 2018
@headius headius modified the milestones: JRuby 9.2.1.0, JRuby 9.2.2.0 Oct 26, 2018
@enebo enebo modified the milestones: JRuby 9.2.5.0, JRuby 9.2.6.0 Dec 6, 2018
headius added a commit to headius/jruby that referenced this issue Dec 10, 2018
headius added a commit to headius/jruby that referenced this issue Dec 12, 2018
ahorek pushed a commit to ahorek/ruby that referenced this issue Dec 17, 2018
@enebo enebo removed this from the JRuby 9.2.6.0 milestone Dec 19, 2018
@enebo enebo added this to the JRuby 9.2.7.0 milestone Dec 19, 2018
@enebo enebo modified the milestones: JRuby 9.2.7.0, JRuby 9.2.8.0 Jan 9, 2019
headius added a commit to jruby/ruby that referenced this issue Feb 12, 2019
@headius
Copy link
Member Author

@headius headius commented Mar 13, 2019

The first example in the description also fails to call the method on MRI, so I'm not sure when that was valid:

$ cat blah.rb
module X; end
class Foo; extend X; end
module Y; def blah; super; end; end
module X; prepend Y; end
class Foo; extend Y; end; Foo.blah

$ rvm ruby-2.5.3 do ruby blah.rb
Traceback (most recent call last):
	1: from blah.rb:5:in `<main>'
blah.rb:3:in `blah': super: no superclass method `blah' for Foo:Class (NoMethodError)

The changes I made to cgi/util.rb also do not appear to be necessary to pass all the cgi/util tests. It appears that work in #5627 has made those tweaks unnecessary.

@headius
Copy link
Member Author

@headius headius commented Jul 29, 2020

This was never actually fixed, and my change in 3d8847e just disabled the CGI extension altogether.

@headius headius removed this from the JRuby 9.2.7.0 milestone Jul 29, 2020
@headius
Copy link
Member Author

@headius headius commented Jul 29, 2020

Actually I will open a new bug for this.

The primary example case presented above is expected to produce a NoMethodError. This bug was about the fact that we instead overflowed the call stack. That issue has indeed been fixed (perhaps by #5627 or perhaps not).

Showing old behavior and new correct error:

[] ~/projects/jruby $ rvm jruby-9.2.1.0 do ruby blah.rb
Error: Your application used more stack memory than the safety cap of 2048K.
Specify -J-Xss####k to increase it (#### = cap size in KB).
Specify -w for full java.lang.StackOverflowError stack trace

[] ~/projects/jruby $ ruby blah.rb
NoMethodError: super: no superclass method `blah' for Foo:Class
  method_missing at org/jruby/RubyBasicObject.java:1715
            blah at blah.rb:3
            blah at blah.rb:3
          <main> at blah.rb:5

There is, however, some additional problem with the CGI module that is not covered by this bug's reduced example or the fixes that resolved it. Restoring the CGI logic that combines prepend and extend_object similar to CGI causes the errors shown in the original description. Since this bug's example case was fixed in 9.2.7.0, I will file a new issue for the continuing problems with CGI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants