String: remove to_unsafe_slice and keep to_slice, but make it ret…

…urn a read-only Slice. Making `String#to_slice` return a copy of the bytes is not good because it wastes memory. It's better to return the original bytes, as before. However, these bytes shouldn't be mutated. To solve this we could introduce `const` to the language, but that's a huge change and it's not clear how to do it. Instead, we let slices be optionally read-only and raise when trying to modify them. This solves the problem because there's no longer unsafe code by invoking `String#to_slice` and it's as efficient as before. We also don't need `Slice.unsafe_readonly` anymore.
crystal-lang · Feb 20, 2017 · bdc81db · bdc81db · Sija · Feb 23, 2017
1 parent d94bb7e
commit bdc81db
Show file tree

Hide file tree

Showing 19 changed files with 97 additions and 115 deletions.
diff --git a/spec/std/slice_spec.cr b/spec/std/slice_spec.cr
@@ -366,6 +366,18 @@ describe "Slice" do
     slice = Slice(Int32).empty
     slice.empty?.should be_true
   end
+
+  it "creates read-only slice" do
+    slice = Slice.new(3, 0, read_only: true)
+    expect_raises { slice[0] = 1 }
+    expect_raises { slice.copy_from(slice) }
+
+    subslice = slice[0, 1]
+    expect_raises { subslice[0] = 1 }
+
+    slice = Bytes[1, 2, 3, read_only: true]
+    expect_raises { slice[0] = 0_u8 }
+  end
 end
 
 private def itself(*args)

diff --git a/src/adler32/adler32.cr b/src/adler32/adler32.cr
@@ -10,7 +10,7 @@ module Adler32
   end
 
   def self.update(data, adler32 : UInt32) : UInt32
-    slice = Slice.unsafe_readonly(data)
+    slice = data.to_slice
     LibZ.adler32(adler32, slice, slice.size).to_u32
   end
 

diff --git a/src/base64.cr b/src/base64.cr
@@ -42,7 +42,7 @@ module Base64
   # Q3J5c3RhbA==
   # ```
   def encode(data) : String
-    slice = Slice.unsafe_readonly(data)
+    slice = data.to_slice
     String.new(encode_size(slice.size, new_lines: true)) do |buf|
       appender = buf.appender
       encode_with_new_lines(slice) { |byte| appender << byte }
@@ -60,7 +60,7 @@ module Base64
   # ```
   def encode(data, io : IO)
     count = 0
-    encode_with_new_lines(Slice.unsafe_readonly(data)) do |byte|
+    encode_with_new_lines(data.to_slice) do |byte|
       io.write_byte byte
       count += 1
     end
@@ -70,7 +70,7 @@ module Base64
 
   private def encode_with_new_lines(data)
     inc = 0
-    to_base64(Slice.unsafe_readonly(data), CHARS_STD, pad: true) do |byte|
+    to_base64(data.to_slice, CHARS_STD, pad: true) do |byte|
       yield byte
       inc += 1
       if inc >= LINE_SIZE
@@ -100,7 +100,7 @@ module Base64
   end
 
   private def strict_encode(data, alphabet, pad = false)
-    slice = Slice.unsafe_readonly(data)
+    slice = data.to_slice
     String.new(encode_size(slice.size)) do |buf|
       appender = buf.appender
       to_base64(slice, alphabet, pad: pad) { |byte| appender << byte }
@@ -121,7 +121,7 @@ module Base64
 
   private def strict_encode_to_io_internal(data, io, alphabet, pad)
     count = 0
-    to_base64(Slice.unsafe_readonly(data), alphabet, pad: pad) do |byte|
+    to_base64(data.to_slice, alphabet, pad: pad) do |byte|
       count += 1
       io.write_byte byte
     end
@@ -138,7 +138,7 @@ module Base64
   # The *padding* parameter defaults to `true`. When `false`, enough `=` characters
   # are not added to make the output divisible by 4.
   def urlsafe_encode(data, padding = true) : String
-    slice = Slice.unsafe_readonly(data)
+    slice = data.to_slice
     String.new(encode_size(slice.size)) do |buf|
       appender = buf.appender
       to_base64(slice, CHARS_SAFE, pad: padding) { |byte| appender << byte }
@@ -159,7 +159,7 @@ module Base64
   # Returns the base64-decoded version of *data* as a `Bytes`.
   # This will decode either the normal or urlsafe alphabets.
   def decode(data) : Bytes
-    slice = Slice.unsafe_readonly(data)
+    slice = data.to_slice
     buf = Pointer(UInt8).malloc(decode_size(slice.size))
     appender = buf.appender
     from_base64(slice) { |byte| appender << byte }
@@ -170,7 +170,7 @@ module Base64
   # This will decode either the normal or urlsafe alphabets.
   def decode(data, io : IO)
     count = 0
-    from_base64(Slice.unsafe_readonly(data)) do |byte|
+    from_base64(data.to_slice) do |byte|
       io.write_byte byte
       count += 1
     end
@@ -181,7 +181,7 @@ module Base64
   # Returns the base64-decoded version of *data* as a string.
   # This will decode either the normal or urlsafe alphabets.
   def decode_string(data) : String
-    slice = Slice.unsafe_readonly(data)
+    slice = data.to_slice
     String.new(decode_size(slice.size)) do |buf|
       appender = buf.appender
       from_base64(slice) { |byte| appender << byte }

diff --git a/src/crc32/crc32.cr b/src/crc32/crc32.cr
@@ -10,7 +10,7 @@ module CRC32
   end
 
   def self.update(data, crc32 : UInt32) : UInt32
-    slice = Slice.unsafe_readonly(data)
+    slice = data.to_slice
     LibZ.crc32(crc32, slice, slice.size).to_u32
   end
 

diff --git a/src/crypto/subtle.cr b/src/crypto/subtle.cr
@@ -10,8 +10,8 @@ module Crypto::Subtle
   #
   # NOTE: *x* and *y* must be able to respond to `to_slice`.
   def self.constant_time_compare(x, y) : Bool
-    x = Slice.unsafe_readonly(x)
-    y = Slice.unsafe_readonly(y)
+    x = x.to_slice
+    y = y.to_slice
     return false if x.size != y.size
 
     v = 0_u8

diff --git a/src/digest/base.cr b/src/digest/base.cr
@@ -4,7 +4,7 @@ abstract class Digest::Base
   # Returns the hash of *data*. *data* must respond to `#to_slice`.
   def self.digest(data)
     digest do |ctx|
-      ctx.update(Slice.unsafe_readonly(data))
+      ctx.update(data.to_slice)
     end
   end
 
@@ -50,7 +50,7 @@ abstract class Digest::Base
       yield ctx
     end
 
-    Slice.unsafe_readonly(hashsum).hexstring
+    hashsum.to_slice.hexstring
   end
 
   # Returns the base64-encoded hash of *data*.

diff --git a/src/digest/md5.cr b/src/digest/md5.cr
@@ -14,7 +14,7 @@ class Digest::MD5 < Digest::Base
   end
 
   def update(data)
-    slice = Slice.unsafe_readonly(data)
+    slice = data.to_slice
     update(slice.to_unsafe, slice.bytesize.to_u32)
   end
 

diff --git a/src/digest/sha1.cr b/src/digest/sha1.cr
@@ -20,7 +20,7 @@ class Digest::SHA1 < Digest::Base
   end
 
   def update(data)
-    message_array = Slice.unsafe_readonly(data)
+    message_array = data.to_slice
     message_array.each do |byte|
       @message_block[@message_block_index] = byte & 0xFF_u8
       @message_block_index += 1

diff --git a/src/http/common.cr b/src/http/common.cr
@@ -237,7 +237,7 @@ module HTTP
   # HTTP.dequote_string(quoted) # => %q("foo\bar")
   # ```
   def self.dequote_string(str)
-    data = Slice.unsafe_readonly(str)
+    data = str.to_slice
     quoted_pair_index = data.index('\\'.ord)
     return str unless quoted_pair_index
 

diff --git a/src/http/web_socket/protocol.cr b/src/http/web_socket/protocol.cr
@@ -84,7 +84,7 @@ class HTTP::WebSocket::Protocol
   end
 
   def send(data : String)
-    send(data.to_unsafe_slice, Opcode::TEXT)
+    send(data.to_slice, Opcode::TEXT)
   end
 
   def send(data : Bytes)
@@ -215,23 +215,23 @@ class HTTP::WebSocket::Protocol
 
   def ping(message = nil)
     if message
-      send(Slice.unsafe_readonly(message), Opcode::PING)
+      send(message.to_slice, Opcode::PING)
     else
       send(Bytes.empty, Opcode::PING)
     end
   end
 
   def pong(message = nil)
     if message
-      send(Slice.unsafe_readonly(message), Opcode::PONG)
+      send(message.to_slice, Opcode::PONG)
     else
       send(Bytes.empty, Opcode::PONG)
     end
   end
 
   def close(message = nil)
     if message
-      send(Slice.unsafe_readonly(message), Opcode::CLOSE)
+      send(message.to_slice, Opcode::CLOSE)
     else
       send(Bytes.empty, Opcode::CLOSE)
     end

diff --git a/src/io/memory.cr b/src/io/memory.cr
@@ -68,7 +68,7 @@ class IO::Memory
   # io.print "hi" # raises IO::Error
   # ```
   def self.new(string : String)
-    new string.to_unsafe_slice, writeable: false
+    new string.to_slice, writeable: false
   end
 
   # See `IO#read(slice)`.
@@ -361,7 +361,7 @@ class IO::Memory
 
     old_writeable = @writeable
     old_resizeable = @resizeable
-    io = IO::Memory.new(to_unsafe_slice[offset, bytesize], writeable: false)
+    io = IO::Memory.new(to_slice[offset, bytesize], writeable: false)
     begin
       @writeable = false
       @resizeable = false
@@ -407,21 +407,7 @@ class IO::Memory
     String.new @buffer, @bytesize
   end
 
-  # Returns a `Slice` over the internal buffer. Modifying the slice
-  # modifies the internal buffer.
-  #
-  # This method is **unsafe** because the internal buffer might
-  # point to bytes that came from a string literal, which are
-  # placed in the ROM (read-only memory) section data of the program,
-  # and attempting to mutate the returned Bytes might result
-  # in segmentation fault. You should use `to_slice` instead unless
-  # you know the returned bytes won't be mutated, to save an
-  # extra memory allocation.
-  def to_unsafe_slice : Bytes
-    Slice.new(@buffer, @bytesize)
-  end
-
-  # Returns a copy of the underlying bytes.
+  # Returns the underlying bytes.
   #
   # ```
   # io = IO::Memory.new
@@ -430,12 +416,12 @@ class IO::Memory
   # io.to_slice # => Bytes[104, 101, 108, 108, 111]
   # ```
   def to_slice : Bytes
-    to_unsafe_slice.clone
+    Slice.new(@buffer, @bytesize, read_only: !@writeable)
   end
 
   # Appends this internal buffer to the given `IO`.
   def to_s(io)
-    io.write(to_unsafe_slice)
+    io.write(to_slice)
   end
 
   private def check_writeable

diff --git a/src/json/lexer/string_based.cr b/src/json/lexer/string_based.cr
@@ -55,7 +55,7 @@ class JSON::Lexer::StringBased < JSON::Lexer
   end
 
   def slice_range(start_pos, end_pos)
-    @reader.string.to_unsafe_slice[start_pos, end_pos - start_pos]
+    @reader.string.to_slice[start_pos, end_pos - start_pos]
   end
 
   private def next_char_no_column_increment

diff --git a/src/number.cr b/src/number.cr
@@ -44,8 +44,8 @@ struct Number
   # ints = Int64.slice(1, 2, 3)
   # ints.class # => Slice(Int64)
   # ```
-  macro slice(*nums)
-    %slice = Slice({{@type}}).new({{nums.size}})
+  macro slice(*nums, read_only = false)
+    %slice = Slice({{@type}}).new({{nums.size}}, read_only: {{read_only}})
     {% for num, i in nums %}
       %slice.to_unsafe[{{i}}] = {{@type}}.new({{num}})
     {% end %}

diff --git a/src/openssl/cipher.cr b/src/openssl/cipher.cr
@@ -51,7 +51,7 @@ class OpenSSL::Cipher
   end
 
   def update(data)
-    slice = Slice.unsafe_readonly(data)
+    slice = data.to_slice
     buffer_length = slice.size + block_size
     buffer = Bytes.new(buffer_length)
     if LibCrypto.evp_cipherupdate(@ctx, buffer, pointerof(buffer_length), slice, slice.size) != 1

diff --git a/src/openssl/hmac.cr b/src/openssl/hmac.cr
@@ -15,8 +15,8 @@ class OpenSSL::HMAC
           when :sha512    then LibCrypto.evp_sha512
           else                 raise "Unsupported digest algorithm: #{algorithm}"
           end
-    key_slice = Slice.unsafe_readonly(key)
-    data_slice = Slice.unsafe_readonly(data)
+    key_slice = key.to_slice
+    data_slice = data.to_slice
     buffer = Bytes.new(128)
     LibCrypto.hmac(evp, key_slice, key_slice.size, data_slice, data_slice.size, buffer, out buffer_len)
     buffer[0, buffer_len.to_i]

diff --git a/src/openssl/ssl/context.cr b/src/openssl/ssl/context.cr
@@ -295,7 +295,7 @@ abstract class OpenSSL::SSL::Context
   def alpn_protocol=(protocol : String)
     proto = Bytes.new(protocol.bytesize + 1)
     proto[0] = protocol.bytesize.to_u8
-    protocol.to_unsafe_slice.copy_to(proto.to_unsafe + 1, protocol.bytesize)
+    protocol.to_slice.copy_to(proto.to_unsafe + 1, protocol.bytesize)
     self.alpn_protocol = proto
   end