Bug #7383

Use stricter cache check in load.c

Added by Yura Sokolov over 2 years ago. Updated over 2 years ago.

[ruby-core:49518]
Status:Closed
Priority:Normal
Assignee:Hiroshi Shirosaki
ruby -v:ruby 2.0.0dev (2012-11-18 trunk 37708) [x86_64-linux] Backport:

Description

rb_ary_shared_with_p does not react when #shift or #pop is called on original array.

This patch introduce rb_ary_dup_of_p , which makes more adequate check for duplicate
of array.

https://github.com/ruby/ruby/pull/216
https://github.com/ruby/ruby/pull/216.patch

0001-Fix-cache-validity-check-of-require.patch Magnifier (2.57 KB) Hiroshi Shirosaki, 11/19/2012 10:55 PM

Associated revisions

Revision 37808
Added by shirosaki over 2 years ago

Fix cache validity check of require

  • array.c (rb_ary_shared_with_p): fix cache validity check.
    If #pop or #shift has been called against $: or $", the array will
    be still shared with the snapshot. We check array length for cache
    validity.
    [Bug #7383]

  • test/ruby/test_require.rb
    (TestRequire#test_require_with_array_pop,
    TestRequire#test_require_with_array_shift): add tests for above.

Revision 37808
Added by shirosaki over 2 years ago

Fix cache validity check of require

  • array.c (rb_ary_shared_with_p): fix cache validity check.
    If #pop or #shift has been called against $: or $", the array will
    be still shared with the snapshot. We check array length for cache
    validity.
    [Bug #7383]

  • test/ruby/test_require.rb
    (TestRequire#test_require_with_array_pop,
    TestRequire#test_require_with_array_shift): add tests for above.

History

#1 Updated by Hiroshi Shirosaki over 2 years ago

Yura, thank you.
Indeed rb_ary_shared_with_p does not react against #shift or #pop. Original array remains shared with the snapshot.
However, I think adding a check of array length would be sufficient for that. This fix has no performance impact, still O(1). I've attached a patch with tests.
Are there any reasons that such a strict check is needed?

#2 Updated by Yura Sokolov over 2 years ago

Hiroshi,

Adding size check is enough in this case.
But rb_ary_dup_of_p is more useful in the wild, I think.

With regards,
Yura

2012/11/19 h.shirosaki (Hiroshi Shirosaki) h.shirosaki@gmail.com

Issue #7383 has been updated by h.shirosaki (Hiroshi Shirosaki).

File 0001-Fix-cache-validity-check-of-require.patch added

Yura, thank you.
Indeed rb_ary_shared_with_p does not react against #shift or #pop.
Original array remains shared with the snapshot.
However, I think adding a check of array length would be sufficient for
that. This fix has no performance impact, still O(1). I've attached a patch
with tests.

Are there any reasons that such a strict check is needed?

Bug #7383: Use stricter cache check in load.c
https://bugs.ruby-lang.org/issues/7383#change-33097

Author: funny_falcon (Yura Sokolov)
Status: Open
Priority: High
Assignee: h.shirosaki (Hiroshi Shirosaki)
Category: core
Target version: 2.0.0
ruby -v: ruby 2.0.0dev (2012-11-18 trunk 37708) [x86_64-linux]

rb_ary_shared_with_p does not react when #shift or #pop is called on
original array.

This patch introduce rb_ary_dup_of_p , which makes more adequate check for
duplicate
of array.

https://github.com/ruby/ruby/pull/216
https://github.com/ruby/ruby/pull/216.patch

http://bugs.ruby-lang.org/

#3 Updated by Hiroshi Shirosaki over 2 years ago

On Thu, Nov 22, 2012 at 6:09 PM, Юрий Соколов funny.falcon@gmail.com wrote:

Adding size check is enough in this case.
But rb_ary_dup_of_p is more useful in the wild, I think.

OK. Then I'll apply my patch for now. If real issues were raised, I'll
consider your patch.

--
Hiroshi Shirosaki

#4 Updated by Anonymous over 2 years ago

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

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


Fix cache validity check of require

  • array.c (rb_ary_shared_with_p): fix cache validity check.
    If #pop or #shift has been called against $: or $", the array will
    be still shared with the snapshot. We check array length for cache
    validity.
    [Bug #7383]

  • test/ruby/test_require.rb
    (TestRequire#test_require_with_array_pop,
    TestRequire#test_require_with_array_shift): add tests for above.

Also available in: Atom PDF