Project

General

Profile

Actions

Backport #7046

closed

ERB#run and ERB#result are not safe for concurrent use

Added by headius (Charles Nutter) over 11 years ago. Updated over 11 years ago.

Status:
Rejected
[ruby-core:47638]

Description

ERB#run and ERB#result both accept an optional binding under which to execute the template. However, if none is given, they both use TOPLEVEL_BINDING by default. Given that by default, the _erbout variable is used for the String into which ERB output gets appended, this causes concurrent template execution on the same thread or separate threads to modify the same buffer. On JRuby, this led to overflow errors when in-progress writes saw their buffers suddenly altered.

This also causes any variables or values evaluated at TOPLEVEL to remain referenced.

I have provided a patch (https://gist.github.com/3764377) that is still very close to the toplevel binding, but instead uses the following logic each call to get a new, isolated binding in which to run the template:

eval "proc{binding}.call", TOPLEVEL_BINDING

This provides visibility to all values at TOPLEVEL, isolates runs to reduce concurrency issues, and guarantees any values stored in the binding will be thrown away after execution.

This fix should be backported to 1.9.3 at minimum.

Updated by headius (Charles Nutter) over 11 years ago

Redmine did not parse the patch URL correctly. It is https://gist.github.com/3764377

Updated by headius (Charles Nutter) over 11 years ago

Oh, I also want to make it clear that this is broken on MRI too, for both the nested, same-thread case and the separate, concurrent thread cases. It needs to be patched.

Updated by nobu (Nobuyoshi Nakada) over 11 years ago

Hi,

At Sat, 22 Sep 2012 07:59:51 +0900,
headius (Charles Nutter) wrote in [ruby-core:47638]:

I have provided a patch (https://gist.github.com/3764377) that is still very close to the toplevel binding, but instead uses the following logic each call to get a new, isolated binding in which to run the template:

eval "proc{binding}.call", TOPLEVEL_BINDING

`TOPLEVEL_BINDING.dup' would work too.

--
Nobu Nakada

Updated by headius (Charles Nutter) over 11 years ago

In JRuby it does not appear that dup'ing a binding copies all structures over, so we'd need to fix that as well to use TOPLEVEL_BINDING.dup.

It appears we match 1.8.7 behavior still, for Binding#dup:

system ~/projects/jruby $ ruby-1.8.7-p358 -e "eval 'a = 1', TOPLEVEL_BINDING.dup; eval 'puts a', TOPLEVEL_BINDING.dup"
1

system ~/projects/jruby $ jruby -e "eval 'a = 1', TOPLEVEL_BINDING.dup; eval 'puts a', TOPLEVEL_BINDING.dup"
1

system ~/projects/jruby $ ruby-1.9.3 -e "eval 'a = 1', TOPLEVEL_BINDING.dup; eval 'puts a', TOPLEVEL_BINDING.dup"

:in `': undefined local variable or method `a' for main:Object (NameError) from -e:1:in `eval' from -e:1:in `'

Given that we would not be releasing patched ERB in any release other than one with this fixed, I think TOPLEVEL_BINDING.dup is probably the simplest way.

Updated by nobu (Nobuyoshi Nakada) over 11 years ago

Hi,

At Tue, 25 Sep 2012 02:58:32 +0900,
headius (Charles Nutter) wrote in [ruby-core:47676]:

It appears we match 1.8.7 behavior still, for Binding#dup:

1.8.7 is dying.

--
Nobu Nakada

Updated by headius (Charles Nutter) over 11 years ago

Indeed, and we will fix our 1.9 mode to work like 1.9.3.

Updated by headius (Charles Nutter) over 11 years ago

We are shipping the proc-based fix in JRuby 1.7.0.RC1 today. There's a good chance we'll fix the Binding#dup behavior for 1.7.0, in which case we'd use the TOPLEVEL_BINDING.dup fix.

Functionally, I think they should both appear to be the same to any user.

Updated by headius (Charles Nutter) over 11 years ago

Ping! I am wondering if a decision has been made about how to fix this. As mentioned, JRuby is shipping the proc-based fix.

Updated by mame (Yusuke Endoh) over 11 years ago

  • Status changed from Open to Assigned
  • Assignee set to nobu (Nobuyoshi Nakada)
  • Target version set to 2.0.0

Looks good to me. Nobu, do you have any concern?

--
Yusuke Endoh

Actions #10

Updated by nobu (Nobuyoshi Nakada) over 11 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r37594.
Charles, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


erb.rb: safe concurrent use

  • lib/erb.rb (ERB#run, ERB#result): eval under isolated bindings for
    safe concurrent use. [ruby-core:47638] [Bug #7046]
Actions #11

Updated by nobu (Nobuyoshi Nakada) over 11 years ago

  • Tracker changed from Bug to Backport
  • Project changed from Ruby master to Backport193
  • Category deleted (lib)
  • Status changed from Closed to Assigned
  • Assignee changed from nobu (Nobuyoshi Nakada) to usa (Usaku NAKAMURA)
  • Target version deleted (2.0.0)
Actions #12

Updated by usa (Usaku NAKAMURA) over 11 years ago

  • Status changed from Assigned to Closed

This issue was solved with changeset r38318.
Charles, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


merge revision(s) 37594: [Backport #7046]

* lib/erb.rb (ERB#run, ERB#result): eval under isolated bindings for
  safe concurrent use.  [ruby-core:47638] [Bug #7046]

Updated by usa (Usaku NAKAMURA) over 11 years ago

  • Status changed from Closed to Rejected

I once backported r37594 at r38318, but it broke rubyspec.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0