irb in a Windows console: Fiddle finds A exports, pack("p") writes a real address

.github/workflows/ci.yml

@@ -219,6 +219,9 @@ jobs:
             cmd //c ironruby.cmd smoke.rb
             cmd //c ironruby.cmd names.rb
             cmd //c ir.cmd -e "puts RUBY_ENGINE"
+            # irb loads reline, which on Windows looks up Win32 functions through Fiddle by
+            # their names without the A suffix; 4.0.2 died there, at a pipe as at a console.
+            printf 'puts 6 * 7\n' | cmd //c irb.cmd | grep -x 42
             cmd //c ironruby.cmd Util/windows-desktop-check.rb
             rubyw="Src/Console/bin/Debug/$TFM/ironrubyw.exe"
             ruby subsystem.rb "$rubyw"
@@ -227,6 +230,7 @@ jobs:
             ./ironruby smoke.rb
             ./ironruby names.rb
             ./ir.sh -e 'puts RUBY_ENGINE'
+            printf 'puts 6 * 7\n' | ./irb.sh | grep -x 42
           fi
 
       # IO#write_nonblock on a socket claims only what the kernel took (#14). On Linux the
@@ -373,6 +377,13 @@ jobs:
           [ "$RUNNER_OS" = Windows ] && EXE=.exe
           cd "Src/IronRuby.Tests/bin/Debug/$TFM"
           "./IronRuby.Tests$EXE" > results.txt 2>&1 || true
+          # A run that died - an access violation in native code ends the process - leaves no
+          # "> FAILED" line to count; one that finished ends in PASSED or in its Repro line.
+          if ! grep -qE 'PASSED.?$|^Repro: ' results.txt; then
+            echo "::error::the test run did not finish"
+            tail -n 40 results.txt
+            exit 1
+          fi
           failures=$(grep -c '^> FAILED' results.txt || true)
           echo "failure lines: $failures (baseline $BASELINE)"
           echo "::group::failing tests"

Src/IronRuby.Tests/RubyTests.cs

@@ -730,6 +730,9 @@ public Tests(Driver/*!*/ driver) {
                 Zlib_Streams1,
                 Process_SpawnKill1,
                 File_DriveLetters1,
+                Fiddle_AnsiSuffix1,
+                PackPointer_RealAddress1,
+                PackPointer_RelineWin32Api1,
                 Dir_ChdirRelative1,
                 File_ExpandTildeOrder1,
                 File_FlockReadpartialRlimit1,

Src/IronRuby.Tests/Runtime/MiscTests.cs

@@ -945,6 +945,107 @@ p Pathname.new('C:/x/y').absolute? == windows, Pathname.new('/x').absolute?
 ");
         }
 
+        /// <summary>
+        /// Fiddle::Handle finds a Win32 function by the name without its A/W suffix, as CRuby's
+        /// fiddle does (it tries "A" next): kernel32 exports FillConsoleOutputCharacterA and ...W
+        /// but no FillConsoleOutputCharacter, and reline asks for that one - irb died at startup
+        /// on Windows without it. Elsewhere there is no suffix to try; the answers are the same.
+        /// </summary>
+        public void Fiddle_AnsiSuffix1() {
+            TestOutput(@"
+require 'fiddle'
+if File::ALT_SEPARATOR == '\\'
+  k = Fiddle::Handle.new('kernel32')
+  p k['GetModuleHandle'] == k['GetModuleHandleA'], !k.sym_defined?('FillConsoleOutputCharacter').nil?
+  begin
+    k['NoSuchFunction']
+  rescue Fiddle::DLError => e
+    p e.message
+  end
+else
+  p true, true
+  p 'unknown symbol ""NoSuchFunction""'
+end
+", @"
+true
+true
+""unknown symbol \""NoSuchFunction\""""
+");
+        }
+
+        /// <summary>
+        /// [str].pack("p") is a real address, as in CRuby: C reads the String there, and what C
+        /// writes there through Fiddle lands in the String. It used to be an index into the
+        /// packed string's list of Strings, and C reading it crashed the process.
+        /// </summary>
+        public void PackPointer_RealAddress1() {
+            TestOutput(@"
+require 'fiddle'
+lib = case RUBY_PLATFORM
+      when /mswin|mingw/ then 'msvcrt.dll'
+      when /darwin/ then '/usr/lib/libSystem.B.dylib'
+      else 'libc.so.6'
+      end
+libc = Fiddle.dlopen(lib)
+strlen = Fiddle::Function.new(libc['strlen'], [Fiddle::TYPE_VOIDP], Fiddle::TYPE_SIZE_T)
+memset = Fiddle::Function.new(libc['memset'], [Fiddle::TYPE_VOIDP, Fiddle::TYPE_INT, Fiddle::TYPE_SIZE_T], Fiddle::TYPE_VOIDP)
+
+# C only gets an address that can be one: what pack wrote before was an index, and C reading
+# or writing there kills the process, which no test can report.
+s = 'hello'.dup
+addr = [s].pack('p').unpack1('J')
+buf = ""\0"" * 6
+buf_addr = [buf].pack('p').unpack1('J')
+if addr > 0x10000 && buf_addr > 0x10000
+  p Fiddle::Pointer.new(addr).to_s, strlen.call(addr)
+  memset.call(buf_addr, 0x41, 3)
+  p buf
+else
+  p [:not_an_address, addr, buf_addr]
+end
+p [s].pack('p').unpack1('p'), [nil].pack('p').unpack1('J')
+begin
+  [addr].pack('J').unpack1('p')
+rescue ArgumentError => e
+  p e.message
+end
+", @"
+""hello""
+5
+""AAA\u0000\u0000\u0000""
+""hello""
+0
+""no associated pointer""
+");
+        }
+
+        /// <summary>
+        /// reline's own Win32API, unmodified, hands the console API its buffers as
+        /// [str].pack(""p"").unpack(""q*"") and reads what the call wrote from str - which is
+        /// what irb does at startup in a Windows console, and what crashed there.
+        /// </summary>
+        public void PackPointer_RelineWin32Api1() {
+            TestOutput(@"
+if File::ALT_SEPARATOR == '\\'
+  require 'ruby4'
+  require 'reline'
+  api = Reline::Windows::Win32API.new('kernel32', 'GetModuleFileNameA', ['L', 'P', 'L'], 'L')
+  path = ""\0"" * 260
+  if [path].pack('p').unpack1('J') > 0x10000
+    n = api.call(0, path, 260)
+    p n > 0, path[0, n].downcase.end_with?('.exe')
+  else
+    p :not_an_address
+  end
+else
+  p true, true
+end
+", @"
+true
+true
+");
+        }
+
         /// <summary>
         /// A relative Dir.chdir is resolved once, from the directory in force - Dir.pwd keeping
         /// 8.3 short names once made it twice, so ".." went up two levels.

Src/Libraries/Builtins/PackedPointers.cs

@@ -0,0 +1,120 @@
+/* ****************************************************************************
+ *
+ * PackedPointers - the addresses Array#pack("p") and ("P") write.
+ *
+ * This source code is subject to terms and conditions of the Apache License,
+ * Version 2.0. A copy of the license can be found in the License.html file at
+ * the root of this distribution.
+ *
+ * ***************************************************************************/
+
+using System;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+
+namespace IronRuby.Builtins {
+
+    /// <summary>
+    /// CRuby's pack("p") writes the address of the String's own buffer, NUL-terminated, and C
+    /// reads and writes the String through it: reline's Win32API passes every buffer to the
+    /// console API as [str].pack("p").unpack("q*"), and reads what the call wrote from str.
+    /// A MutableString has no such address - its bytes live on a heap that moves them, and C
+    /// writing behind its back would leave its cached state stale - so each String pack is asked
+    /// to point at gets a native block of its own, holding its bytes and a NUL, for as long as
+    /// the String lives. pack writes the block's address. Fiddle recognises the address among a
+    /// call's pointer arguments and copies the String into the block before the call and the
+    /// block back into the String after it (Fiddle.NativeCall.ToPointerArgument), which is
+    /// what the String's own buffer would have given. A String that has grown past its block
+    /// since gets a new one, at a new address, as CRuby's buffer moves when it is reallocated.
+    /// </summary>
+    internal static class PackedPointers {
+        private static readonly ConditionalWeakTable<MutableString, Block> _byString = new ConditionalWeakTable<MutableString, Block>();
+        private static readonly ConcurrentDictionary<long, WeakReference<Block>> _byAddress = new ConcurrentDictionary<long, WeakReference<Block>>();
+        private static readonly object _lock = new object();
+
+        internal sealed class Block {
+            internal readonly MutableString/*!*/ String;
+            internal readonly IntPtr Address;
+            /// <summary>Bytes the block holds, the NUL included.</summary>
+            internal readonly int Capacity;
+            private readonly WeakReference<Block>/*!*/ _entry;
+
+            internal Block(MutableString/*!*/ str, int capacity) {
+                String = str;
+                Capacity = capacity;
+                Address = Marshal.AllocHGlobal(capacity);
+                _entry = new WeakReference<Block>(this);
+                _byAddress[(long)Address] = _entry;
+            }
+
+            ~Block() {
+                // Forget the address before the memory goes, so it cannot be found for a block
+                // that a later allocation gets at the same place.
+                ((ICollection<KeyValuePair<long, WeakReference<Block>>>)_byAddress).Remove(
+                    new KeyValuePair<long, WeakReference<Block>>((long)Address, _entry));
+                Marshal.FreeHGlobal(Address);
+            }
+
+            internal void Forget() {
+                ((ICollection<KeyValuePair<long, WeakReference<Block>>>)_byAddress).Remove(
+                    new KeyValuePair<long, WeakReference<Block>>((long)Address, _entry));
+            }
+
+            /// <summary>Puts the String's bytes and a NUL into the block; answers how many bytes.</summary>
+            internal int Refresh() {
+                byte[] bytes = String.ToByteArray();
+                int count = Math.Min(bytes.Length, Capacity - 1);
+                if (count > 0) {
+                    Marshal.Copy(bytes, 0, Address, count);
+                }
+                Marshal.WriteByte(Address, count, 0);
+                return count;
+            }
+
+            /// <summary>Puts what C left in the block back into the String.</summary>
+            internal void CopyBack(int count) {
+                if (String.IsFrozen) {
+                    return;
+                }
+                count = Math.Min(count, String.GetByteCount());
+                for (int i = 0; i < count; i++) {
+                    String.SetByte(i, Marshal.ReadByte(Address, i));
+                }
+            }
+        }
+
+        /// <summary>The address pack writes for <paramref name="str"/>, its bytes put there.</summary>
+        internal static long AddressOf(MutableString/*!*/ str) {
+            int needed = str.GetByteCount() + 1;
+            Block block;
+            lock (_lock) {
+                if (!_byString.TryGetValue(str, out block) || block.Capacity < needed) {
+                    if (block != null) {
+                        // the old block is freed once nothing holds it; its address is found no more
+                        _byString.Remove(str);
+                        block.Forget();
+                    }
+                    block = new Block(str, needed);
+                    _byString.Add(str, block);
+                }
+            }
+            block.Refresh();
+            return (long)block.Address;
+        }
+
+        /// <summary>The block at <paramref name="address"/>, if pack wrote that address for a live String.</summary>
+        internal static Block Find(long address) {
+            WeakReference<Block> entry;
+            Block block;
+            return (address != 0 && _byAddress.TryGetValue(address, out entry) && entry.TryGetTarget(out block)) ? block : null;
+        }
+
+        /// <summary>Whether <paramref name="address"/> is where pack put <paramref name="str"/> last.</summary>
+        internal static bool IsAddressOf(MutableString/*!*/ str, long address) {
+            Block block;
+            return _byString.TryGetValue(str, out block) && (long)block.Address == address;
+        }
+    }
+}

Src/Libraries/Builtins/RubyEncoder.cs

@@ -1499,11 +1499,12 @@ private static bool IsKnownDirective(char c) {
                         case 'P': {
                             // MRI writes the address of the string and remembers the string
                             // itself alongside the result, so that unpacking can find it again.
-                            // An address into a managed heap would mean nothing, so what goes
-                            // into the bytes is a one-based index into that list of strings.
-                            // The list travels as an instance variable, which is what makes it
-                            // survive #dup and not survive a round trip through a Symbol -
-                            // exactly the rule MRI's associated-object table follows.
+                            // The address is a real one, of a native block holding the string's
+                            // bytes (PackedPointers), so C can read and - through Fiddle - write
+                            // the string as it does in MRI. The list of strings travels as an
+                            // instance variable, which is what makes it survive #dup and not
+                            // survive a round trip through a Symbol - exactly the rule MRI's
+                            // associated-object table follows.
                             count = 1;
                             object pointerArg = GetPackArg(self, i);
                             long pointer = 0;
@@ -1514,7 +1515,7 @@ private static bool IsKnownDirective(char c) {
                                         pointers = new RubyArray();
                                     }
                                     pointers.Add(target);
-                                    pointer = pointers.Count;
+                                    pointer = PackedPointers.AddressOf(target);
                                 }
                             }
                             WritePointer(stream, pointer);
@@ -1578,15 +1579,24 @@ private static object Dereference(RubyContext/*!*/ context, MutableString/*!*/ s
                 return null;
             }
 
+            // As MRI's: one of the strings packed into this one, the one whose bytes are at that
+            // address now - a string that has outgrown its block since is at another.
             object stored;
             RubyArray pointers;
-            if (!context.TryGetInstanceVariable(self, PackedPointersVariable, out stored) ||
-                (pointers = stored as RubyArray) == null ||
-                pointer < 1 || pointer > pointers.Count) {
+            MutableString target = null;
+            if (context.TryGetInstanceVariable(self, PackedPointersVariable, out stored) && (pointers = stored as RubyArray) != null) {
+                foreach (object candidate in pointers) {
+                    var str = candidate as MutableString;
+                    if (str != null && PackedPointers.IsAddressOf(str, pointer)) {
+                        target = str;
+                        break;
+                    }
+                }
+            }
+            if (target == null) {
                 throw RubyExceptions.CreateArgumentError("no associated pointer");
             }
 
-            var target = (MutableString)pointers[(int)pointer - 1];
             if (directive.Directive == 'p') {
                 return target.Clone();
             }

Src/Libraries/Fiddle/FiddleCall.cs

@@ -302,6 +302,11 @@ internal struct Temporary {
                 internal MutableString String;
                 internal int Count;
                 internal bool CopyBack;
+                /// <summary>
+                /// The block is a String's from pack("p") (PackedPointers), not the call's: it is
+                /// copied back but not freed, and held here so that it outlives the call.
+                /// </summary>
+                internal PackedPointers.Block Packed;
             }
 
             internal static long ToLong(object value) {
@@ -385,7 +390,21 @@ internal static long ToPointerArgument(RespondToStorage/*!*/ respondTo, UnaryOpS
                 }
 
                 if (value is int || value is BigInteger || value is long) {
-                    return ToLong(value);
+                    long address = ToLong(value);
+                    // An address pack("p") wrote for a String: C gets the String's bytes as they
+                    // are now, and what it writes there goes back into the String, as through the
+                    // String's own buffer in CRuby - reline's Win32API passes its buffers so.
+                    PackedPointers.Block packed = PackedPointers.Find(address);
+                    if (packed != null) {
+                        temporaries.Add(new Temporary {
+                            Block = packed.Address,
+                            String = packed.String,
+                            Count = packed.Refresh(),
+                            CopyBack = !isConst && !packed.String.IsFrozen,
+                            Packed = packed
+                        });
+                    }
+                    return address;
                 }
 
                 var handle = value as Handle;
@@ -431,6 +450,13 @@ internal static void ReleaseTemporaries(List<Temporary>/*!*/ temporaries, List<P
 
                 for (int i = 0; i < temporaries.Count; i++) {
                     Temporary t = temporaries[i];
+                    if (t.Packed != null) {
+                        // the String's own block: it stays, for the String and its address
+                        if (t.CopyBack) {
+                            t.Packed.CopyBack(t.Count);
+                        }
+                        continue;
+                    }
                     if (t.CopyBack && t.String != null && !t.String.IsFrozen) {
                         for (int j = 0; j < t.Count; j++) {
                             t.String.SetByte(j, Marshal.ReadByte(t.Block, j));