https://redmine.ruby-lang.org/https://redmine.ruby-lang.org/favicon.ico?17113305112020-07-24T16:19:22ZRuby Issue Tracking SystemRuby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867042020-07-24T16:19:22Zalanwu (Alan Wu)
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/86704/diff?detail_id=57557">diff</a>)</li></ul> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867052020-07-24T16:24:48Zalanwu (Alan Wu)
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/86705/diff?detail_id=57558">diff</a>)</li></ul> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867062020-07-24T16:40:01Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>Removing <code>Module#initialize_copy</code> would probably require changes to <code>Module#{dup,clone}</code>. Maybe we can set a flag in <code>Module#initialize_copy</code> such that calling the method raises if called again on the same module (not sure if that is what you meant)?</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867072020-07-24T17:23:59Zalanwu (Alan Wu)
<ul></ul><p>I'm not proposing that we remove <code>initialize_copy</code>, but to make it raise when the receiver has children. So this is what it would look like:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">module</span> <span class="nn">A</span>
<span class="k">end</span>
<span class="no">A</span><span class="p">.</span><span class="nf">send</span><span class="p">(</span><span class="ss">:initialize_copy</span><span class="p">,</span> <span class="no">Module</span><span class="p">.</span><span class="nf">new</span><span class="p">)</span> <span class="c1"># fine, no one inherits from A</span>
<span class="k">class</span> <span class="nc">B</span>
<span class="kp">include</span> <span class="no">A</span>
<span class="no">A</span><span class="p">.</span><span class="nf">send</span><span class="p">(</span><span class="ss">:initialize_copy</span><span class="p">,</span> <span class="no">Module</span><span class="p">.</span><span class="nf">new</span><span class="p">)</span> <span class="c1"># raise, since A now has one child</span>
<span class="k">end</span>
</code></pre>
<p>Admittedly this proposed behavior is designed to sidestep the problem that the current implementation has.<br>
Though maybe it's fine since this is a seldom used private method and other Ruby implementations don't have to follow this behavior?</p>
<p>Now that you've mentioned it, removing the method and moving the logic into <code>Module#{dup,clone}</code> feels like the cleanest solution. It's more likely to be a breaking change though.</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867082020-07-24T17:52:49Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>I would recommend to have <code>initialize_copy</code> always raise instead of only raising if the module has children. It's more consistent, and there is no reason anyone should be calling <code>initialize_copy</code> more than once. This shouldn't be considered a breaking change, as <code>initialize_copy</code> is a private method. I think it is better than moving the logic to <code>dup</code>/<code>clone</code>, just in case a Module subclass is overridding <code>initialize_copy</code> and calling super.</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867122020-07-24T22:29:46Zalanwu (Alan Wu)
<ul></ul><p>In principle I agree that allowing initialization only once is a good way to go, but the way <code>Module.allocate</code> is currently setup makes implementing this a bit complicated. At the moment there is not really a pre-init state for modules and the result from <code>Module.allocate</code> is the same as <code>Module.new</code>. If we want to do this we would have to implement a pre-init state for modules, and make sure that all the operations on modules (adding methods, constants, etc) are aware of this state so they can do the initialization in case they receive a pre-init module.</p>
<p>It's doable, but I don't know if the extra consistency is worth the added complexity. It would slow things down a small bit for operations like method addition and has the potential to introduce crashes if we miss places where we need the initialization check.</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867152020-07-25T02:35:19Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul></ul><p>I agree with <a class="user active user-mention" href="https://redmine.ruby-lang.org/users/16806">@alanwu (Alan Wu)</a>, that it won't be worth.</p>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gh">diff --git c/class.c i/class.c
index 6835d2d7289..f7a56601634 100644
</span><span class="gd">--- c/class.c
</span><span class="gi">+++ i/class.c
</span><span class="p">@@ -354,6 +354,13 @@</span> static void ensure_origin(VALUE klass);
VALUE
rb_mod_init_copy(VALUE clone, VALUE orig)
{
<span class="gi">+ if (RCLASS_EXT(clone)->subclasses ||
+ RCLASS_EXT(clone)->parent_subclasses ||
+ RCLASS_EXT(clone)->module_subclasses) {
+ rb_raise(rb_eTypeError, "cannot replace %s in use",
+ (RB_TYPE_P(clone, T_MODULE) ? "module" : "class"));
+ }
+
</span> /* cloned flag is refer at constant inline cache
* see vm_get_const_key_cref() in vm_insnhelper.c
*/
<span class="gh">diff --git c/test/ruby/test_module.rb i/test/ruby/test_module.rb
index d2da384cbd1..8d986f13413 100644
</span><span class="gd">--- c/test/ruby/test_module.rb
</span><span class="gi">+++ i/test/ruby/test_module.rb
</span><span class="p">@@ -435,6 +435,12 @@</span>
assert_empty(m.constants, bug9813)
end
<span class="gi">+ def test_initialize_copy_in_use
+ m = Module.new
+ Class.new {include m}
+ assert_raise(TypeError) {m.send(:initialize_copy, Module.new)}
+ end
+
</span> def test_dup
OtherSetup.call
</code></pre> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867262020-07-25T10:32:51ZEregon (Benoit Daloze)
<ul></ul><p>Should we rather design a good way to allow copying but yet not have to deal with uninitialized state?</p>
<p>Right now the only well-defined protocols for copying are</p>
<ul>
<li><code>dup = allocate, copy @ivars, initialize_dup (which calls initialize_copy)</code></li>
<li><code>clone = allocate, copy @ivars, initialize_clone (which calls initialize_copy), clone also copies extra state like frozen and the singleton class</code></li>
</ul>
<p>This means some classes have to support an "unintialized state" when otherwise they would not need to.<br>
And in some cases it even means instances have to be mutable when they would otherwise not need to (e.g., MatchData, <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Make MatchData frozen and forbid MatchData.allocate (Closed)" href="https://redmine.ruby-lang.org/issues/16294">#16294</a>).</p>
<p>So maybe we should make Module.allocate and #initialize_copy always raise, and override <code>dup</code> and <code>clone</code> for Module?<br>
It's still unfortunate that this would mean duplicating the logic for dup/clone there.<br>
So I think a better copying protocol is warranted here as it's not just an issue for Module.</p>
<p>Re @nobu's patch I don't like this ad-hoc condition which leaks implementation details into semantics.<br>
How about having an <code>initialized</code> flag that's set by <code>#initialize</code> and <code>#initialize_copy</code> and checked in both of these methods if we want a quick fix?</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867312020-07-25T18:33:52Zalanwu (Alan Wu)
<ul></ul><blockquote>
<p>How about having an initialized flag that's set by #initialize and #initialize_copy and checked in both of these methods if we want a quick fix?</p>
</blockquote>
<p>That doesn't work because you can trigger the bug without ever calling <code>initialize</code> on the module:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="n">m</span> <span class="o">=</span> <span class="no">Module</span><span class="p">.</span><span class="nf">allocate</span>
<span class="n">m</span><span class="p">.</span><span class="nf">prepend</span><span class="p">(</span><span class="no">Module</span><span class="p">.</span><span class="nf">allocate</span><span class="p">)</span>
<span class="n">m</span><span class="p">.</span><span class="nf">define_method</span><span class="p">(</span><span class="ss">:hello</span><span class="p">)</span> <span class="p">{}</span>
<span class="n">klass</span> <span class="o">=</span> <span class="no">Class</span><span class="p">.</span><span class="nf">new</span> <span class="p">{</span> <span class="kp">include</span> <span class="n">m</span> <span class="p">}</span>
<span class="n">m</span><span class="p">.</span><span class="nf">send</span><span class="p">(</span><span class="ss">:initialize_copy</span><span class="p">,</span> <span class="no">Module</span><span class="p">.</span><span class="nf">allocate</span><span class="p">)</span>
<span class="no">GC</span><span class="p">.</span><span class="nf">start</span>
<span class="n">klass</span><span class="p">.</span><span class="nf">new</span><span class="p">.</span><span class="nf">hello</span> <span class="k">rescue</span> <span class="kp">nil</span>
<span class="c1"># you may need to run this multiple times to get it to crash</span>
</code></pre>
<p>If we want something like that we would have to implement an uninitialized state.</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867372020-07-26T06:21:31Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul></ul><p>Tried it.<br>
<a href="https://github.com/nobu/ruby/tree/uninitialized-module" class="external">https://github.com/nobu/ruby/tree/uninitialized-module</a></p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867432020-07-26T16:23:31ZEregon (Benoit Daloze)
<ul></ul><p>nobu (Nobuyoshi Nakada) wrote in <a href="#note-10">#note-10</a>:</p>
<blockquote>
<p>Tried it.<br>
<a href="https://github.com/nobu/ruby/tree/uninitialized-module" class="external">https://github.com/nobu/ruby/tree/uninitialized-module</a></p>
</blockquote>
<p>Right, removing <code>Module.allocate</code> is one way since <code>dup</code>/<code>clone</code> uses the alloc function directly (and does not call <code>allocate</code>).</p>
<p>I think calling <code>Module.allocate</code> in user code makes no sense, so that approach seems a good way to me.</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=867842020-07-28T23:45:18Zalanwu (Alan Wu)
<ul></ul><p>Thank you for the code, <a class="user active user-mention" href="https://redmine.ruby-lang.org/users/4">@nobu (Nobuyoshi Nakada)</a> . I think with your branch we could even keep <code>.allocate</code>, though people wouldn't be able to do much with it.<br>
As long as no one is able to call <code>initialize_copy</code> after children (iclasses) exist, it's fine.<br>
I think I was wrong about the number of places we would have to plug to implement an uninitialized state that resolves the issue.<br>
Only the places that make new iclasses need to check for the uninitilaized state, so jsut <code>prepend</code>, <code>include</code> and maybe refinements.</p>
<p>Side note about the branch (57c7f9b), it's possible to get access to an uninitialized module in Ruby land by subclassing from <code>Module</code>:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">class</span> <span class="nc">Sub</span> <span class="o"><</span> <span class="no">Module</span>
<span class="k">def</span> <span class="nf">initialize_copy</span><span class="p">(</span><span class="n">other</span><span class="p">)</span>
<span class="nb">p</span> <span class="nb">ancestors</span>
<span class="k">end</span>
<span class="k">end</span>
<span class="no">Sub</span><span class="p">.</span><span class="nf">new</span><span class="p">.</span><span class="nf">dup</span> <span class="c1"># [#<Sub:0x00007fa4ec015b10>, BasicObject]</span>
</code></pre>
<p>It doesn't cause anything bad to happen AFAICT. I just found it interesting that the branch adds a normally impossible-to-construct module.<br>
Maybe it's a positive because it makes Ruby more weird :D</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=870332020-08-12T07:30:31Zko1 (Koichi Sasada)
<ul></ul><p>sorry I didn't check all threads, but nobu's patch can close it?</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=872052020-08-26T20:27:30Zalanwu (Alan Wu)
<ul></ul><p>Yes, nobu's patch fixes the crash. It is technically a breaking change though, so maybe it needs approval from Matz?<br>
Side note, the bug still exists on master as of <a href="https://wandbox.org/permlink/afSmo1UbL9wnIBmq" class="external">today</a>.</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=927352021-07-02T17:11:38Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>This example in the original post has been fixed recently in the master branch. I bisected the fixing commit to <a class="changeset" title="Use Module#ancestors order in recursive constant lookup Before this commit, const_get with inher..." href="https://redmine.ruby-lang.org/projects/ruby-master/repository/git/revisions/3dd3ea092acead6179033f2c95525ffc5b8bb6ff">3dd3ea092acead6179033f2c95525ffc5b8bb6ff</a>. However, as that changes constant lookup order, I'm guessing we aren't going to want to backport it.</p>
<p>Ruby still allows calling <code>Module.allocate</code> and calling <code>Module#initialize_copy</code> on modules with subclasses. And the example in <a href="#note-9">#note-9</a> still can cause an occasional hang in my environment. So I'm going to leave this open.</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=936982021-09-16T07:18:25Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul></ul><p>As far as I tested it with <code>master</code> slightly, it didn't reproduce.<br>
Does it still happen, and how occational?</p> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=937252021-09-16T16:03:04Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>nobu (Nobuyoshi Nakada) wrote in <a href="#note-16">#note-16</a>:</p>
<blockquote>
<p>As far as I tested it with <code>master</code> slightly, it didn't reproduce.<br>
Does it still happen, and how occational?</p>
</blockquote>
<p>It still happens in my environment. I tested with 1000 iterations, and it hung on the 14th iteration:</p>
<pre><code>$ for x in `jot 1000`; do echo -n .; ./ruby 17048-note9.rb; done
..............
</code></pre> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=937272021-09-16T21:00:31Zalanwu (Alan Wu)
<ul></ul><p>Here a repro that that always crashes for me on cbbda3e:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="n">mod</span> <span class="o">=</span> <span class="no">Module</span><span class="p">.</span><span class="nf">new</span> <span class="p">{</span> <span class="n">define_method</span><span class="p">(</span><span class="ss">:foo</span><span class="p">)</span> <span class="p">{</span><span class="ss">:first</span><span class="p">}</span> <span class="p">}</span>
<span class="n">klass</span> <span class="o">=</span> <span class="no">Class</span><span class="p">.</span><span class="nf">new</span> <span class="p">{</span> <span class="kp">include</span> <span class="n">mod</span> <span class="p">}</span>
<span class="n">instance</span> <span class="o">=</span> <span class="n">klass</span><span class="p">.</span><span class="nf">new</span>
<span class="nb">p</span> <span class="n">instance</span><span class="p">.</span><span class="nf">foo</span>
<span class="n">new_mod</span> <span class="o">=</span> <span class="no">Module</span><span class="p">.</span><span class="nf">new</span> <span class="p">{</span> <span class="n">define_method</span><span class="p">(</span><span class="ss">:foo</span><span class="p">)</span> <span class="p">{</span> <span class="ss">:second</span> <span class="p">}</span> <span class="p">}</span>
<span class="n">mod</span><span class="p">.</span><span class="nf">send</span><span class="p">(</span><span class="ss">:initialize_copy</span><span class="p">,</span> <span class="n">new_mod</span><span class="p">)</span>
<span class="mi">4</span><span class="p">.</span><span class="nf">times</span> <span class="p">{</span> <span class="no">GC</span><span class="p">.</span><span class="nf">start</span> <span class="p">}</span>
<span class="nb">p</span> <span class="n">instance</span><span class="p">.</span><span class="nf">foo</span> <span class="c1"># [BUG] unreachable</span>
</code></pre> Ruby master - Bug #17048: Calling initialize_copy on live modules leads to crasheshttps://redmine.ruby-lang.org/issues/17048?journal_id=937412021-09-17T10:07:46Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li></ul><p>Applied in changeset <a class="changeset" title="Already initialized modules cannot be replaced [Bug #17048]" href="https://redmine.ruby-lang.org/projects/ruby-master/repository/git/revisions/178ee1e801acb33d13b3e8a630f6ca4926c68fbc">git|178ee1e801acb33d13b3e8a630f6ca4926c68fbc</a>.</p>
<hr>
<p>Already initialized modules cannot be replaced [Bug <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Calling initialize_copy on live modules leads to crashes (Closed)" href="https://redmine.ruby-lang.org/issues/17048">#17048</a>]</p>