[GR-21864] Backport to 20.1.

PullRequest: truffleruby/1603

Author: gilles-duboscq

CHANGELOG.md

@@ -64,6 +64,7 @@ Bug fixes:
 * Fix issue where interpolated string matched `#` within string as being a variable (#1495).
 * Fix `File.join` to raise error on strings with null bytes.
 * Fix initialization of Ruby Thread for foreign thread created in Java.
+* Thread local IO buffers are now allocated using a stack to ensure safe operating if a signal handler uses one during an IO operation.
 
 Compatibility:
 

lib/truffle/socket/truffle/foreign.rb

@@ -96,32 +96,42 @@ module Foreign
 
       def self.bind(descriptor, sockaddr)
         sockaddr_p = Truffle::FFI::Pool.stack_alloc(:char, sockaddr.bytesize)
-
-        sockaddr_p.write_string(sockaddr, sockaddr.bytesize)
-
-        _bind(descriptor, sockaddr_p, sockaddr.bytesize)
+        begin
+          sockaddr_p.write_string(sockaddr, sockaddr.bytesize)
+          _bind(descriptor, sockaddr_p, sockaddr.bytesize)
+        ensure
+          Truffle::FFI::Pool.stack_free(sockaddr_p)
+        end
       end
 
       def self.connect(descriptor, sockaddr)
         sockaddr = Socket.coerce_to_string(sockaddr)
 
         sockaddr_p = Truffle::FFI::Pool.stack_alloc(:char, sockaddr.bytesize)
 
-        sockaddr_p.write_string(sockaddr, sockaddr.bytesize)
+        begin
+          sockaddr_p.write_string(sockaddr, sockaddr.bytesize)
 
-        _connect(descriptor, sockaddr_p, sockaddr.bytesize)
+          _connect(descriptor, sockaddr_p, sockaddr.bytesize)
+        ensure
+          Truffle::FFI::Pool.stack_free(sockaddr_p)
+        end
       end
 
       def self.getsockopt(descriptor, level, optname)
         val, length = Truffle::FFI::Pool.stack_alloc(:char, 256, :socklen_t, 1)
 
-        length.write_int(256)
+        begin
+          length.write_int(256)
 
-        err = _getsockopt(descriptor, level, optname, val, length)
+          err = _getsockopt(descriptor, level, optname, val, length)
 
-        Errno.handle('Unable to get socket option') unless err == 0
+          Errno.handle('Unable to get socket option') unless err == 0
 
-        val.read_string(length.read_int)
+          val.read_string(length.read_int)
+        ensure
+          Truffle::FFI::Pool.stack_free(val)
+        end
       end
 
       def self.getaddrinfo(host, service = nil, family = nil, socktype = nil,
@@ -134,6 +144,7 @@ def self.getaddrinfo(host, service = nil, family = nil, socktype = nil,
         hints[:ai_flags]    = flags || 0
 
         res_p = Truffle::FFI::Pool.stack_alloc(:pointer, 1)
+
         res_p.clear
         err   = _getaddrinfo(host, service, hints.pointer, res_p)
 
@@ -172,6 +183,7 @@ def self.getaddrinfo(host, service = nil, family = nil, socktype = nil,
 
           # Be sure to feed a legit pointer to freeaddrinfo
           freeaddrinfo(ptr) unless ptr.null?
+          Truffle::FFI::Pool.stack_free(res_p)
         end
       end
 
@@ -188,52 +200,64 @@ def self.getnameinfo(sockaddr, flags = ::Socket::NI_NUMERICHOST | ::Socket::NI_N
         sockaddr_p, node, service = Truffle::FFI::Pool.stack_alloc(
           :char, sockaddr.bytesize, :char, ::Socket::NI_MAXHOST, :char, ::Socket::NI_MAXSERV)
 
-        sockaddr_p.write_string(sockaddr, sockaddr.bytesize)
+        begin
+          sockaddr_p.write_string(sockaddr, sockaddr.bytesize)
 
-        if reverse_lookup
-          err = _getnameinfo(sockaddr_p, sockaddr.bytesize, node,
-                             ::Socket::NI_MAXHOST, nil, 0, 0)
+          if reverse_lookup
+            err = _getnameinfo(sockaddr_p, sockaddr.bytesize, node,
+                               ::Socket::NI_MAXHOST, nil, 0, 0)
 
-          name_info[2] = node.read_string if err == 0
-        end
+            name_info[2] = node.read_string if err == 0
+          end
 
-        err = _getnameinfo(sockaddr_p, sockaddr.bytesize, node,
-                           ::Socket::NI_MAXHOST, service,
-                           ::Socket::NI_MAXSERV, flags)
+          err = _getnameinfo(sockaddr_p, sockaddr.bytesize, node,
+                             ::Socket::NI_MAXHOST, service,
+                             ::Socket::NI_MAXSERV, flags)
 
-        raise SocketError, gai_strerror(err) unless err == 0
+          raise SocketError, gai_strerror(err) unless err == 0
 
-        sa_family = SockaddrIn.new(sockaddr_p)[:sin_family]
+          sa_family = SockaddrIn.new(sockaddr_p)[:sin_family]
 
-        name_info[0] = ::Socket::Constants::AF_TO_FAMILY[sa_family]
-        name_info[1] = service.read_string
-        name_info[3] = node.read_string
+          name_info[0] = ::Socket::Constants::AF_TO_FAMILY[sa_family]
+          name_info[1] = service.read_string
+          name_info[3] = node.read_string
 
-        name_info[2] = name_info[3] unless name_info[2]
+          name_info[2] = name_info[3] unless name_info[2]
 
-        name_info
+          name_info
+        ensure
+          Truffle::FFI::Pool.stack_free(sockaddr_p)
+        end
       end
 
       def self.getpeername(descriptor)
         sockaddr_storage_p, len_p = Truffle::FFI::Pool.stack_alloc(:char, 128, :socklen_t, 1)
-        len_p.write_int(128)
+        begin
+          len_p.write_int(128)
 
-        err = _getpeername(descriptor, sockaddr_storage_p, len_p)
+          err = _getpeername(descriptor, sockaddr_storage_p, len_p)
 
-        Errno.handle('getpeername(2)') unless err == 0
+          Errno.handle('getpeername(2)') unless err == 0
 
-        sockaddr_storage_p.read_string(len_p.read_int)
+          sockaddr_storage_p.read_string(len_p.read_int)
+        ensure
+          Truffle::FFI::Pool.stack_free(sockaddr_storage_p)
+        end
       end
 
       def self.getsockname(descriptor)
         sockaddr_storage_p, len_p = Truffle::FFI::Pool.stack_alloc(:char, 128, :socklen_t, 1)
 
-        len_p.write_int(128)
-        err = _getsockname(descriptor, sockaddr_storage_p, len_p)
+        begin
+          len_p.write_int(128)
+          err = _getsockname(descriptor, sockaddr_storage_p, len_p)
 
-        Errno.handle('getsockname(2)') unless err == 0
+          Errno.handle('getsockname(2)') unless err == 0
 
-        sockaddr_storage_p.read_string(len_p.read_int)
+          sockaddr_storage_p.read_string(len_p.read_int)
+        ensure
+          Truffle::FFI::Pool.stack_free(sockaddr_storage_p)
+        end
       end
 
       def self.pack_sockaddr_in(host, port, family = ::Socket::AF_UNSPEC,
@@ -278,6 +302,7 @@ def self.pack_sockaddr_in(host, port, family = ::Socket::AF_UNSPEC,
           ptr = res_p.read_pointer
 
           freeaddrinfo(ptr) unless ptr.null?
+          Truffle::FFI::Pool.stack_free(res_p)
         end
       end
 
@@ -304,12 +329,16 @@ def self.getpeereid(*)
 
       def self.socketpair(family, type, protocol)
         pointer = Truffle::FFI::Pool.stack_alloc(:int, 2)
-        pointer.clear
-        status = _socketpair(family, type, protocol, pointer)
+        begin
+          pointer.clear
+          status = _socketpair(family, type, protocol, pointer)
 
-        Errno.handle('socketpair(2)') unless status == 0
+          Errno.handle('socketpair(2)') unless status == 0
 
-        pointer.read_array_of_int(2)
+          pointer.read_array_of_int(2)
+        ensure
+          Truffle::FFI::Pool.stack_free(pointer)
+        end
       end
 
       def self.char_pointer(length, &block)
@@ -346,11 +375,15 @@ def self.ip_to_bytes(family, address)
 
         pointer = Truffle::FFI::Pool.stack_alloc(:pointer, size)
 
-        status = inet_pton(family, address, pointer)
+        begin
+          status = inet_pton(family, address, pointer)
 
-        Errno.handle('inet_pton()') if status < 1
+          Errno.handle('inet_pton()') if status < 1
 
-        pointer.get_array_of_uchar(0, size)
+          pointer.get_array_of_uchar(0, size)
+        ensure
+          Truffle::FFI::Pool.stack_free(pointer)
+        end
       end
     end
   end

src/main/java/org/truffleruby/core/support/IONodes.java

@@ -82,6 +82,7 @@
 import org.truffleruby.core.rope.RopeOperations;
 import org.truffleruby.core.string.StringNodes.MakeStringNode;
 import org.truffleruby.core.thread.GetCurrentRubyThreadNode;
+import org.truffleruby.core.thread.ThreadLocalBuffer;
 import org.truffleruby.core.thread.ThreadManager.BlockingAction;
 import org.truffleruby.extra.ffi.Pointer;
 import org.truffleruby.language.Visibility;
@@ -480,8 +481,8 @@ protected int write(int fd, DynamicObject string) {
 
     }
 
-    @Primitive(name = "io_get_thread_buffer")
-    public static abstract class GetThreadBufferNode extends PrimitiveArrayArgumentsNode {
+    @Primitive(name = "io_thread_buffer_allocate")
+    public static abstract class IOThreadBufferAllocateNode extends PrimitiveArrayArgumentsNode {
 
         @Specialization
         protected DynamicObject getThreadBuffer(VirtualFrame frame, long size,
@@ -496,23 +497,27 @@ protected DynamicObject getThreadBuffer(VirtualFrame frame, long size,
         }
 
         public static Pointer getBuffer(DynamicObject rubyThread, long size, ConditionProfile sizeProfile) {
-            final Pointer buffer = Layouts.THREAD.getIoBuffer(rubyThread);
-
-            if (sizeProfile.profile(buffer.getSize() >= size)) {
-                return buffer;
-            } else {
-                return reallocateBuffer(size, rubyThread, buffer);
-            }
-        }
-
-        @TruffleBoundary
-        private static Pointer reallocateBuffer(long size, final DynamicObject rubyThread, final Pointer buffer) {
-            buffer.freeNoAutorelease();
-            final Pointer newBuffer = Pointer.malloc(Math.max(size * 2, 1024));
+            final ThreadLocalBuffer buffer = Layouts.THREAD.getIoBuffer(rubyThread);
+            final ThreadLocalBuffer newBuffer = buffer.allocate(size, sizeProfile);
             Layouts.THREAD.setIoBuffer(rubyThread, newBuffer);
-            return newBuffer;
+            return newBuffer.start;
         }
 
     }
 
+    @Primitive(name = "io_thread_buffer_free")
+    public static abstract class IOThreadBufferFreeNode extends PrimitiveArrayArgumentsNode {
+
+        @Specialization
+        protected Object getThreadBuffer(VirtualFrame frame, DynamicObject pointer,
+                @Cached GetCurrentRubyThreadNode currentThreadNode,
+                @Cached("createBinaryProfile()") ConditionProfile freeProfile) {
+            DynamicObject thread = currentThreadNode.executeGetRubyThread(frame);
+            final ThreadLocalBuffer threadBuffer = Layouts.THREAD.getIoBuffer(thread);
+            assert threadBuffer.start.getAddress() == Layouts.POINTER.getPointer(pointer).getAddress();
+            Layouts.THREAD.setIoBuffer(thread, threadBuffer.free(freeProfile));
+            return nil;
+        }
+    }
+
 }

src/main/java/org/truffleruby/core/thread/ThreadLayout.java

@@ -17,7 +17,6 @@
 import org.truffleruby.core.InterruptMode;
 import org.truffleruby.core.basicobject.BasicObjectLayout;
 import org.truffleruby.core.fiber.FiberManager;
-import org.truffleruby.extra.ffi.Pointer;
 import org.truffleruby.language.threadlocal.ThreadLocalGlobals;
 
 import com.oracle.truffle.api.object.DynamicObject;
@@ -57,7 +56,7 @@ Object[] build(
             @Nullable @Volatile Object value,
             AtomicBoolean wakeUp,
             @Volatile int priority,
-            Pointer ioBuffer,
+            ThreadLocalBuffer ioBuffer,
             Object threadGroup,
             String sourceLocation,
             Object name);
@@ -112,9 +111,9 @@ Object[] build(
 
     void setPriority(DynamicObject object, int value);
 
-    Pointer getIoBuffer(DynamicObject object);
+    ThreadLocalBuffer getIoBuffer(DynamicObject object);
 
-    void setIoBuffer(DynamicObject object, Pointer value);
+    void setIoBuffer(DynamicObject object, ThreadLocalBuffer value);
 
     Object getThreadGroup(DynamicObject object);
 

src/main/java/org/truffleruby/core/thread/ThreadLocalBuffer.java

@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2020 Oracle and/or its affiliates. All rights reserved. This
+ * code is released under a tri EPL/GPL/LGPL license. You can use it,
+ * redistribute it and/or modify it under the terms of the:
+ *
+ * Eclipse Public License version 2.0, or
+ * GNU General Public License version 2, or
+ * GNU Lesser General Public License version 2.1.
+ */
+package org.truffleruby.core.thread;
+
+import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
+import com.oracle.truffle.api.profiles.ConditionProfile;
+
+import org.truffleruby.extra.ffi.Pointer;
+
+public final class ThreadLocalBuffer {
+
+    public static final ThreadLocalBuffer NULL_BUFFER = new ThreadLocalBuffer(true, Pointer.NULL, 0, 0, null);
+
+    final boolean ownsBuffer;
+    public final Pointer start;
+    final long used;
+    final long remaining;
+    final ThreadLocalBuffer parent;
+
+    private ThreadLocalBuffer(
+            boolean isBlockStart,
+            Pointer start,
+            long used,
+            long remaining,
+            ThreadLocalBuffer parent) {
+        this.ownsBuffer = isBlockStart;
+        this.start = start;
+        this.used = used;
+        this.remaining = remaining;
+        this.parent = parent;
+    }
+
+    public ThreadLocalBuffer free(ConditionProfile freeProfile) {
+        if (freeProfile.profile(ownsBuffer)) {
+            start.freeNoAutorelease();
+        }
+        return parent;
+    }
+
+    public void freeAll() {
+        ThreadLocalBuffer current = this;
+        while (current != null) {
+            current.free(ConditionProfile.getUncached());
+            current = current.parent;
+        }
+    }
+
+    public ThreadLocalBuffer allocate(long size, ConditionProfile allocationProfile) {
+        if (allocationProfile.profile(remaining >= size)) {
+            return new ThreadLocalBuffer(
+                    false,
+                    new Pointer(this.start.getAddress() + this.used, size),
+                    size,
+                    remaining - size,
+                    this);
+        } else {
+            return allocateNewBlock(size);
+        }
+    }
+
+    @TruffleBoundary
+    private ThreadLocalBuffer allocateNewBlock(long size) {
+        // Allocate a new buffer. Chain it if we aren't the default thread buffer, otherwise make a new default buffer.
+        final long blockSize = Math.max(size, 1024);
+        if (this.parent != null) {
+            return new ThreadLocalBuffer(true, Pointer.malloc(blockSize), size, blockSize - size, this);
+        } else {
+            // Free the old block
+            this.free(ConditionProfile.getUncached());
+            // Create new bigger block
+            final ThreadLocalBuffer newParent = new ThreadLocalBuffer(
+                    true,
+                    Pointer.malloc(blockSize),
+                    0,
+                    blockSize,
+                    null);
+            return new ThreadLocalBuffer(
+                    false,
+                    new Pointer(newParent.start.getAddress(), size),
+                    size,
+                    blockSize - size,
+                    newParent);
+        }
+    }
+}