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

Fix Array#replace on shifted arrays #13256

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Mar 31, 2023

Array#replace is broken because it never accounts for the space available between an Array's root and real buffers, if #shift has been called on that Array. Here are two examples:

class Array(T)
  def dump
    String.build do |io|
      io << "["
      @capacity.times do |i|
        io << ", " if i > 0
        if 0 <= i - @offset_to_buffer < @size
          @buffer[i - @offset_to_buffer].inspect(io)
        else
          io << '_'
        end
      end
      io << ']'
    end
  end
end

x = [0, 1, 2, 3, 4]
x.shift
x.dump # => [_, 1, 2, 3, 4]

# element lost after reallocation
x.replace([0, 1, 2, 3, 4, 5, 6, 7])
x.dump # => [_, 0, 1, 2, 3, 4, 5, 6]
10000.times { x << 1 } # segfault

# element lost without reallocation
x = [0, 1, 2, 3, 4]
x.shift
x.replace([5, 6, 7, 8, 9])
x.dump # => [_, 5, 6, 7, 8]
10000.times { x << 1 } # segfault

Both #replace calls write into memory outside the array buffer. This PR ensures the above doesn't happen:

x = [0, 1, 2, 3, 4]
x.shift
x.replace([0, 1, 2, 3, 4, 5, 6, 7])
x.dump # => [0, 1, 2, 3, 4, 5, 6, 7, _, _]
10000.times { x << 1 } # okay

x = [0, 1, 2, 3, 4]
x.shift
x.replace([5, 6, 7, 8, 9])
x.dump # => [5, 6, 7, 8, 9]
10000.times { x << 1 } # okay

Additionally, it clears the unused region if the new array is smaller. This is important for arrays of reference types because previously the trailing elements would still be reachable after such a call to #replace and therefore cannot be garbage-collected.

src/array.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 1.8.0 milestone Apr 3, 2023
@straight-shoota straight-shoota merged commit 7f46589 into crystal-lang:master Apr 4, 2023
@HertzDevil HertzDevil deleted the bug/array-replace branch April 4, 2023 11:50
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants