[GR-8904] Check bounds for FFI::Pointer accesses

eregon · eregon · commit 15763c3fbd93 · 2020-09-24T18:29:22.000Z
PullRequest: truffleruby/2004
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -38,6 +38,7 @@ Compatibility:
 * Pass the final `super` specs (#2104, @chrisseaton).
 * Fix arity for arguments with optional kwargs (#1669, @ssnickolay)
 * Fix arity for `Proc` (#2098, @ssnickolay)
+* Check bounds for `FFI::Pointer` accesses when the size of the memory behind is known. 
 
 Performance:
 
diff --git a/bench/optcarrot/lib/optcarrot/driver/misc.rb b/bench/optcarrot/lib/optcarrot/driver/misc.rb
@@ -105,7 +105,7 @@ def icon_data
       end
       dat = dat.bytes.map {|clr| palette[clr - 35] }
 
-      return width, height, pixels.write_string(dat.pack("V*"))
+      return width, height, pixels.write_bytes(dat.pack("V*"))
     end
   end
 end
diff --git a/lib/truffle/socket/basic_socket.rb b/lib/truffle/socket/basic_socket.rb
@@ -98,7 +98,7 @@ def setsockopt(*args)
       end
     elsif optval.is_a?(String)
       Truffle::Socket::Foreign.memory_pointer(optval.bytesize) do |pointer|
-        pointer.write_string(optval)
+        pointer.write_bytes(optval)
 
         error = Truffle::Socket::Foreign
           .setsockopt(Primitive.io_fd(self), level, optname, pointer, optval.bytesize)
@@ -129,7 +129,7 @@ def send(message, flags, dest_sockaddr = nil)
     end
 
     Truffle::Socket::Foreign.char_pointer(bytes) do |buffer|
-      buffer.write_string(message)
+      buffer.write_bytes(message)
 
       if dest_sockaddr.is_a?(String)
         addr = Truffle::Socket.sockaddr_class_for_socket(self)
@@ -258,7 +258,7 @@ def recvmsg_nonblock(max_msg_len = nil, flags = 0, max_control_len = nil, except
     address    = nil
 
     begin
-      msg_buffer.write_string(message)
+      msg_buffer.write_bytes(message)
 
       header.message = io_vec
 
diff --git a/lib/truffle/socket/socket.rb b/lib/truffle/socket/socket.rb
@@ -165,7 +165,7 @@ def self.gethostbyaddr(addr, family = nil)
     family = Truffle::Socket.address_family(family)
 
     Truffle::Socket::Foreign.char_pointer(addr.bytesize) do |in_pointer|
-      in_pointer.write_string(addr)
+      in_pointer.write_bytes(addr)
 
       out_pointer = Truffle::Socket::Foreign
         .gethostbyaddr(in_pointer, in_pointer.total, family)
diff --git a/lib/truffle/socket/truffle/foreign.rb b/lib/truffle/socket/truffle/foreign.rb
@@ -97,7 +97,7 @@ module Foreign
       def self.bind(descriptor, sockaddr)
         sockaddr_p = Truffle::FFI::Pool.stack_alloc(:char, sockaddr.bytesize)
         begin
-          sockaddr_p.write_string(sockaddr, sockaddr.bytesize)
+          sockaddr_p.write_bytes(sockaddr)
           _bind(descriptor, sockaddr_p, sockaddr.bytesize)
         ensure
           Truffle::FFI::Pool.stack_free(sockaddr_p)
@@ -110,7 +110,7 @@ def self.connect(descriptor, sockaddr)
         sockaddr_p = Truffle::FFI::Pool.stack_alloc(:char, sockaddr.bytesize)
 
         begin
-          sockaddr_p.write_string(sockaddr, sockaddr.bytesize)
+          sockaddr_p.write_bytes(sockaddr)
 
           _connect(descriptor, sockaddr_p, sockaddr.bytesize)
         ensure
@@ -201,7 +201,7 @@ def self.getnameinfo(sockaddr, flags = ::Socket::NI_NUMERICHOST | ::Socket::NI_N
           :char, sockaddr.bytesize, :char, ::Socket::NI_MAXHOST, :char, ::Socket::NI_MAXSERV)
 
         begin
-          sockaddr_p.write_string(sockaddr, sockaddr.bytesize)
+          sockaddr_p.write_bytes(sockaddr)
 
           if reverse_lookup
             err = _getnameinfo(sockaddr_p, sockaddr.bytesize, node,
diff --git a/lib/truffle/socket/truffle/foreign/linger.rb b/lib/truffle/socket/truffle/foreign/linger.rb
@@ -33,7 +33,7 @@ class Linger < ::FFI::Struct
         def self.from_string(string)
           linger = new
 
-          linger.pointer.write_string(string, string.bytesize)
+          linger.pointer.write_bytes(string)
 
           linger
         end
diff --git a/lib/truffle/socket/truffle/foreign/sockaddr_in.rb b/lib/truffle/socket/truffle/foreign/sockaddr_in.rb
@@ -33,7 +33,7 @@ class SockaddrIn < ::FFI::Struct
 
         def self.with_sockaddr(addr)
           pointer = Foreign.memory_pointer(addr.bytesize)
-          pointer.write_string(addr, addr.bytesize)
+          pointer.write_bytes(addr)
 
           new(pointer)
         end
diff --git a/lib/truffle/socket/truffle/foreign/sockaddr_in6.rb b/lib/truffle/socket/truffle/foreign/sockaddr_in6.rb
@@ -33,7 +33,7 @@ class SockaddrIn6 < ::FFI::Struct
 
         def self.with_sockaddr(addr)
           pointer = Foreign.memory_pointer(addr.bytesize)
-          pointer.write_string(addr, addr.bytesize)
+          pointer.write_bytes(addr)
 
           new(pointer)
         end
diff --git a/lib/truffle/socket/truffle/foreign/sockaddr_un.rb b/lib/truffle/socket/truffle/foreign/sockaddr_un.rb
@@ -37,7 +37,7 @@ def self.with_sockaddr(addr)
           end
 
           pointer = Foreign.memory_pointer(size)
-          pointer.write_string(addr, addr.bytesize)
+          pointer.write_bytes(addr)
 
           new(pointer)
         end
diff --git a/spec/ffi/pointer_spec.rb b/spec/ffi/pointer_spec.rb
@@ -155,7 +155,6 @@ def to_ptr
     end
 
     it "access beyond bounds should raise IndexError" do
-      skip "not yet supported on TruffleRuby" if RUBY_ENGINE == "truffleruby"
       expect { @mptr.slice(4, 4).get_int(4) }.to raise_error(IndexError)
     end
   end
diff --git a/spec/ffi/rbx/memory_pointer_spec.rb b/spec/ffi/rbx/memory_pointer_spec.rb
@@ -130,13 +130,11 @@ module CTest
   end
 
   it "raises an error if you try putting a long into a pointer of size 1" do
-    skip "not yet supported on TruffleRuby" if RUBY_ENGINE == "truffleruby"
     m = FFI::MemoryPointer.new(1)
     expect { m.write_long(10) }.to raise_error(IndexError)
   end
 
   it "raises an error if you try putting an int into a pointer of size 1" do
-    skip "not yet supported on TruffleRuby" if RUBY_ENGINE == "truffleruby"
     m = FFI::MemoryPointer.new(1)
     expect { m.write_int(10) }.to raise_error(IndexError)
   end
diff --git a/spec/ruby/core/kernel/shared/require.rb b/spec/ruby/core/kernel/shared/require.rb
@@ -351,9 +351,13 @@
 
           symlink_to_code_dir_two = tmp("codesymlinktwo")
           File.symlink("#{CODE_LOADING_DIR}/b", symlink_to_code_dir_two)
-          $LOAD_PATH.unshift(symlink_to_code_dir_two)
+          begin
+            $LOAD_PATH.unshift(symlink_to_code_dir_two)
 
-          @object.require('load_fixture').should be_false
+            @object.require('load_fixture').should be_false
+          ensure
+            rm_r symlink_to_code_dir_two
+          end
         end
       end
 
diff --git a/src/main/java/org/truffleruby/core/CoreLibrary.java b/src/main/java/org/truffleruby/core/CoreLibrary.java
@@ -77,6 +77,7 @@
 import org.truffleruby.core.time.RubyTime;
 import org.truffleruby.core.tracepoint.RubyTracePoint;
 import org.truffleruby.extra.RubyAtomicReference;
+import org.truffleruby.extra.ffi.Pointer;
 import org.truffleruby.extra.ffi.RubyPointer;
 import org.truffleruby.language.Nil;
 import org.truffleruby.language.NotProvided;
@@ -808,6 +809,8 @@ private void initializeConstants() {
         setConstant(truffleFFIModule, "TYPE_ENUM", NativeTypes.TYPE_ENUM);
         setConstant(truffleFFIModule, "TYPE_VARARGS", NativeTypes.TYPE_VARARGS);
 
+        setConstant(truffleFFIPointerClass, "UNBOUNDED", Pointer.UNBOUNDED);
+
         setConstant(objectClass, "RUBY_VERSION", frozenUSASCIIString(TruffleRuby.LANGUAGE_VERSION));
         setConstant(objectClass, "RUBY_PATCHLEVEL", 0);
         setConstant(objectClass, "RUBY_REVISION", frozenUSASCIIString(TruffleRuby.LANGUAGE_REVISION));
diff --git a/src/main/java/org/truffleruby/extra/ffi/Pointer.java b/src/main/java/org/truffleruby/extra/ffi/Pointer.java
@@ -26,6 +26,7 @@ public class Pointer implements AutoCloseable {
 
     public static final Pointer NULL = new Pointer(0);
     public static final long SIZE = Long.BYTES;
+    public static final long UNBOUNDED = Long.MAX_VALUE;
 
     public static final Pointer[] EMPTY_ARRAY = new Pointer[0];
 
@@ -49,7 +50,7 @@ public static Pointer calloc(long size) {
     private FinalizerReference finalizerRef = null;
 
     public Pointer(long address) {
-        this(address, 0);
+        this(address, UNBOUNDED);
     }
 
     public Pointer(long address, long size) {
diff --git a/src/main/java/org/truffleruby/extra/ffi/PointerNodes.java b/src/main/java/org/truffleruby/extra/ffi/PointerNodes.java
@@ -158,6 +158,28 @@ protected long address(RubyPointer pointer) {
 
     }
 
+    @Primitive(name = "pointer_size")
+    public static abstract class PointerSizeNode extends PrimitiveArrayArgumentsNode {
+
+        @Specialization
+        protected long size(RubyPointer pointer) {
+            return pointer.pointer.getSize();
+        }
+
+    }
+
+    @CoreMethod(names = "total=", required = 1)
+    public static abstract class PointerSetSizeNode extends CoreMethodArrayArgumentsNode {
+
+        @Specialization
+        protected long setSize(RubyPointer pointer, long size) {
+            final Pointer old = pointer.pointer;
+            pointer.pointer = new Pointer(old.getAddress(), size);
+            return size;
+        }
+
+    }
+
     @CoreMethod(names = "autorelease?")
     public static abstract class PointerIsAutoreleaseNode extends CoreMethodArrayArgumentsNode {
 
diff --git a/src/main/ruby/truffleruby/core/io.rb b/src/main/ruby/truffleruby/core/io.rb
@@ -1560,7 +1560,7 @@ def ioctl(command, arg = 0)
       # is still necessary.
       buffer_size = 4096 if buffer_size < 4096
       buffer = Truffle::FFI::MemoryPointer.new(buffer_size)
-      buffer.write_string(arg, arg.bytesize)
+      buffer.write_bytes(arg)
       real_arg = buffer.address
     else
       real_arg = Primitive.rb_to_int(arg)
diff --git a/src/main/ruby/truffleruby/core/posix.rb b/src/main/ruby/truffleruby/core/posix.rb
@@ -418,7 +418,7 @@ def self.write_string_native(io, string, continue_on_eagain)
     length = string.bytesize
     buffer = Primitive.io_thread_buffer_allocate(length)
     begin
-      buffer.write_string string
+      buffer.write_bytes string
 
       written = 0
       while written < length
@@ -466,7 +466,7 @@ def self.write_string_nonblock_native(io, string)
     length = string.bytesize
     buffer = Primitive.io_thread_buffer_allocate(length)
     begin
-      buffer.write_string string
+      buffer.write_bytes string
       written = Truffle::POSIX.write(fd, buffer, length)
 
       if written < 0
diff --git a/src/main/ruby/truffleruby/core/process.rb b/src/main/ruby/truffleruby/core/process.rb
@@ -215,9 +215,10 @@ def self.setproctitle_darwin(title)
     argv0_address = Truffle::POSIX._NSGetArgv.read_pointer.read_pointer
 
     @_argv0_max_length ||= argv0_address.read_string.bytesize
+    argv0_address = argv0_address.slice(0, @_argv0_max_length)
 
     new_title = setproctitle_truncate_title(title, @_argv0_max_length)
-    argv0_address.write_string new_title
+    argv0_address.write_bytes new_title
   end
   private_class_method :setproctitle_darwin
 
@@ -259,8 +260,8 @@ def self.setproctitle_linux_from_proc_maps(title)
 
     new_title = setproctitle_truncate_title(title, @_argv0_max_length)
 
-    argv0_ptr = FFI::Pointer.new(:char, @_argv0_address)
-    argv0_ptr.write_string(new_title)
+    argv0_ptr = FFI::Pointer.new(:char, @_argv0_address).slice(0, @_argv0_max_length)
+    argv0_ptr.write_bytes(new_title)
 
     new_command = File.binread('/proc/self/cmdline')
     raise 'failed' unless new_command.start_with?(new_title)
diff --git a/src/main/ruby/truffleruby/core/truffle/ffi/pointer.rb b/src/main/ruby/truffleruby/core/truffle/ffi/pointer.rb
@@ -39,10 +39,6 @@ class AbstractMemory
   end
 
   class Pointer < AbstractMemory
-    # Indicates how many bytes the chunk of memory that is pointed to takes up.
-    attr_accessor :total
-    alias_method :size, :total
-
     # Indicates how many bytes the type that the pointer is cast as uses.
     attr_accessor :type_size
 
@@ -78,16 +74,21 @@ def initialize(type = nil, address)
     end
 
     def initialize_copy(from)
-      total = from.total
-      raise RuntimeError, 'cannot duplicate unbounded memory area' unless total
+      total = Primitive.pointer_size(from)
+      raise RuntimeError, 'cannot duplicate unbounded memory area' unless total != UNBOUNDED
       Primitive.pointer_malloc self, total
       Primitive.pointer_copy_memory address, from.address, total
       self
     end
 
+    def size
+      Primitive.pointer_size(self)
+    end
+    alias_method :total, :size
+
     def clear
-      raise RuntimeError, 'cannot clear unbounded memory area' unless @total
-      Primitive.pointer_clear self, @total
+      raise RuntimeError, 'cannot clear unbounded memory area' unless Primitive.pointer_size(self) != UNBOUNDED
+      Primitive.pointer_clear self, Primitive.pointer_size(self)
     end
 
     def inspect
@@ -115,8 +116,8 @@ def null?
 
     def +(offset)
       ptr = Pointer.new(address + offset)
-      if total
-        ptr.total = total - offset
+      if Primitive.pointer_size(self) != UNBOUNDED
+        ptr.total = Primitive.pointer_size(self) - offset
       end
       ptr
     end
@@ -143,7 +144,8 @@ def network_order(start, size)
     end
 
     private def check_bounds(offset, length)
-      if offset < 0 || (@total && length > @total)
+      size = Primitive.pointer_size(self)
+      if offset < 0 || offset + length > size
         raise IndexError, "Memory access offset=#{offset} size=#{length} is out of bounds"
       end
     end
@@ -212,6 +214,7 @@ def get_array_of_string(offset, count = nil)
     end
 
     def get_bytes(offset, length)
+      check_bounds(offset, length)
       Primitive.pointer_read_bytes address + offset, length
     end
 
@@ -227,6 +230,7 @@ def put_bytes(offset, str, index = 0, length = nil)
         end
         length = str.bytesize - index
       end
+      check_bounds(offset, length)
       Primitive.pointer_write_bytes address + offset, str, index, length
       self
     end
@@ -296,10 +300,10 @@ def put(type, offset, value)
   class MemoryPointer < Pointer
     def initialize(type, count = 1, clear = true)
       super(type, 0)
-      @total = @type_size * (count || 1)
+      total = @type_size * (count || 1)
 
-      Primitive.pointer_malloc self, @total
-      Primitive.pointer_clear self, @total if clear
+      Primitive.pointer_malloc self, total
+      Primitive.pointer_clear self, total if clear
     end
 
     def self.new(type, count = 1, clear = true)
diff --git a/src/main/ruby/truffleruby/core/truffle/ffi/pointer_access.rb b/src/main/ruby/truffleruby/core/truffle/ffi/pointer_access.rb
diff --git a/tool/generate-pointer-methods.rb b/tool/generate-pointer-methods.rb
