[GR-18163] Always terminate native strings with enough \0 bytes (#2704)

PullRequest: truffleruby/3455

Author: eregon

CHANGELOG.md

@@ -22,6 +22,7 @@ Compatibility:
 * Range literals of integers are now created at parse time like in CRuby (#2622, @aardvark179).
 * Fix `IO.pipe` - allow overriding `IO.new` that is used to create new pipes (#2692, @andykonchin).
 * Fix exception message when there are missing or extra keyword arguments - it contains all the missing/extra keywords now (#1522, @andrykonchin).
+* Always terminate native strings with enough `\0` bytes (#2704, @eregon).
 
 Performance:
 

spec/ruby/optional/capi/ext/string_spec.c

@@ -437,6 +437,12 @@ VALUE string_spec_RSTRING_PTR_read(VALUE self, VALUE str, VALUE path) {
   return capacities;
 }
 
+VALUE string_spec_RSTRING_PTR_null_terminate(VALUE self, VALUE str, VALUE min_length) {
+  char* ptr = RSTRING_PTR(str);
+  char* end = ptr + RSTRING_LEN(str);
+  return rb_str_new(end, FIX2LONG(min_length));
+}
+
 VALUE string_spec_StringValue(VALUE self, VALUE str) {
   return StringValue(str);
 }
@@ -662,6 +668,7 @@ void Init_string_spec(void) {
   rb_define_method(cls, "RSTRING_PTR_after_funcall", string_spec_RSTRING_PTR_after_funcall, 2);
   rb_define_method(cls, "RSTRING_PTR_after_yield", string_spec_RSTRING_PTR_after_yield, 1);
   rb_define_method(cls, "RSTRING_PTR_read", string_spec_RSTRING_PTR_read, 2);
+  rb_define_method(cls, "RSTRING_PTR_null_terminate", string_spec_RSTRING_PTR_null_terminate, 2);
   rb_define_method(cls, "StringValue", string_spec_StringValue, 1);
   rb_define_method(cls, "SafeStringValue", string_spec_SafeStringValue, 1);
   rb_define_method(cls, "rb_str_hash", string_spec_rb_str_hash, 1);

spec/ruby/optional/capi/string_spec.rb

@@ -97,6 +97,32 @@ def inspect
     end
   end
 
+  describe "rb_str_set_len on a UTF-16 String" do
+    before :each do
+      @str = "abcdefghij".force_encoding(Encoding::UTF_16BE)
+      # Make sure to unshare the string
+      @s.rb_str_modify(@str)
+    end
+
+    it "inserts two NULL bytes at the length" do
+      @s.rb_str_set_len(@str, 4).b.should == "abcd".b
+      @s.rb_str_set_len(@str, 8).b.should == "abcd\x00\x00gh".b
+    end
+  end
+
+  describe "rb_str_set_len on a UTF-32 String" do
+    before :each do
+      @str = "abcdefghijkl".force_encoding(Encoding::UTF_32BE)
+      # Make sure to unshare the string
+      @s.rb_str_modify(@str)
+    end
+
+    it "inserts four NULL bytes at the length" do
+      @s.rb_str_set_len(@str, 4).b.should == "abcd".b
+      @s.rb_str_set_len(@str, 12).b.should == "abcd\x00\x00\x00\x00ijkl".b
+    end
+  end
+
   describe "rb_str_buf_new" do
     it "returns the equivalent of an empty string" do
       buf = @s.rb_str_buf_new(10, nil)
@@ -592,6 +618,12 @@ def inspect
       capacities[0].should < capacities[1]
       str.should == "fixture file contents to test read() with RSTRING_PTR"
     end
+
+    it "terminates the string with at least (encoding min length) \\0 bytes" do
+      @s.RSTRING_PTR_null_terminate("abc", 1).should == "\x00"
+      @s.RSTRING_PTR_null_terminate("abc".encode("UTF-16BE"), 2).should == "\x00\x00"
+      @s.RSTRING_PTR_null_terminate("abc".encode("UTF-32BE"), 4).should == "\x00\x00\x00\x00"
+    end
   end
 
   describe "RSTRING_LEN" do

src/main/java/org/truffleruby/cext/CExtNodes.java

@@ -131,7 +131,10 @@
 public class CExtNodes {
 
     public static Pointer newNativeStringPointer(int capacity, RubyLanguage language) {
-        return Pointer.mallocAutoRelease(capacity + 1, language);
+        // We need up to 4 \0 bytes for UTF-32. Always use 4 for speed rather than checking the encoding min length.
+        Pointer pointer = Pointer.mallocAutoRelease(capacity + 4, language);
+        pointer.writeInt(capacity, 0);
+        return pointer;
     }
 
     private static long getNativeStringCapacity(Pointer pointer) {
@@ -732,13 +735,24 @@ public abstract static class RbStrSetLenNode extends CoreMethodArrayArgumentsNod
         protected RubyString strSetLen(RubyString string, int newByteLength,
                 @Cached RubyStringLibrary libString,
                 @Cached StringToNativeNode stringToNativeNode,
-                @Cached MutableTruffleString.FromNativePointerNode fromNativePointerNode) {
+                @Cached MutableTruffleString.FromNativePointerNode fromNativePointerNode,
+                @Cached ConditionProfile minLengthOneProfile) {
             var pointer = stringToNativeNode.executeToNative(string);
 
-            pointer.writeByte(newByteLength, (byte) 0); // Like MRI
+            var encoding = libString.getEncoding(string);
+            int minLength = encoding.jcoding.minLength();
+            // Like MRI
+            if (minLengthOneProfile.profile(minLength == 1)) {
+                pointer.writeByte(newByteLength, (byte) 0);
+            } else if (minLength == 2) {
+                pointer.writeShort(newByteLength, (short) 0);
+            } else if (minLength == 4) {
+                pointer.writeInt(newByteLength, 0);
+            } else {
+                throw CompilerDirectives.shouldNotReachHere();
+            }
 
-            var newNativeTString = fromNativePointerNode.execute(pointer, 0, newByteLength,
-                    libString.getTEncoding(string), false);
+            var newNativeTString = fromNativePointerNode.execute(pointer, 0, newByteLength, encoding.tencoding, false);
             string.setTString(newNativeTString);
 
             return string;
@@ -802,7 +816,6 @@ static MutableTruffleString resize(Pointer pointer, int newCapacity, int newByte
                 RubyLanguage language) {
             final Pointer newPointer = newNativeStringPointer(newCapacity, language);
             newPointer.writeBytes(0, pointer, 0, Math.min(pointer.getSize(), newCapacity));
-            newPointer.writeByte(newCapacity, (byte) 0); // Like MRI
 
             return fromNativePointerNode.execute(newPointer, 0, newByteLength, tencoding, false);
         }
@@ -1225,7 +1238,6 @@ public static Pointer allocateAndCopyToNative(AbstractTruffleString tstring, Tru
                 int capacity, TruffleString.CopyToNativeMemoryNode copyToNativeMemoryNode, RubyLanguage language) {
             final Pointer pointer = newNativeStringPointer(capacity, language);
             copyToNativeMemoryNode.execute(tstring, 0, pointer, 0, capacity, tencoding);
-            pointer.writeByte(capacity, (byte) 0); // Like MRI
             return pointer;
         }
 

src/main/java/org/truffleruby/extra/ffi/Pointer.java

@@ -170,11 +170,6 @@ public void writePointer(long offset, long address) {
         writeLong(offset, address);
     }
 
-    public void writeZeroTerminatedBytes(long offset, byte[] bytes, int start, int length) {
-        writeBytes(offset, bytes, start, length);
-        writeByte(offset + length, (byte) 0);
-    }
-
     @TruffleBoundary
     public void writeBytes(long destByteOffset, long size, byte value) {
         assert address + destByteOffset != 0 || size == 0;
@@ -191,17 +186,6 @@ public void writeBytes(long destByteOffset, Pointer source, int sourceByteOffset
         UNSAFE.copyMemory(source.getAddress() + sourceByteOffset, address + destByteOffset, bytesToCopy);
     }
 
-    @TruffleBoundary
-    public void writeBytes(long destByteOffset, byte[] source, int sourceByteOffset, int bytesToCopy) {
-        assert address + destByteOffset != 0 || bytesToCopy == 0;
-        assert source != null;
-        assert sourceByteOffset >= 0;
-        assert bytesToCopy >= 0;
-
-        UNSAFE.copyMemory(source, Unsafe.ARRAY_BYTE_BASE_OFFSET + sourceByteOffset, null, address + destByteOffset,
-                bytesToCopy);
-    }
-
     public byte readByte(long offset) {
         assert address + offset != 0;
         return UNSAFE.getByte(address + offset);