Correctly release the underlying file mapping. (#9340)

* Avoiding using `Tempfile` which was retaining the file preventing it from unlinking.
ruby · Dec 25, 2023 · 260bf60 · 260bf60
1 parent 5af64ff
commit 260bf60
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 26 deletions.
diff --git a/include/ruby/io/buffer.h b/include/ruby/io/buffer.h
@@ -58,6 +58,9 @@ enum rb_io_buffer_flags {
 
     // The buffer is read-only and cannot be modified.
     RB_IO_BUFFER_READONLY = 128,
+
+    // The buffer is backed by a file.
+    RB_IO_BUFFER_FILE = 256,
 };
 
 // Represents the endian of the data types.

diff --git a/io_buffer.c b/io_buffer.c
@@ -155,17 +155,7 @@ io_buffer_map_file(struct rb_io_buffer *buffer, int descriptor, size_t size, rb_
     buffer->size = size;
 
     buffer->flags |= RB_IO_BUFFER_MAPPED;
-}
-
-// Release the memory associated with a mapped buffer.
-static inline void
-io_buffer_unmap(void* base, size_t size)
-{
-#ifdef _WIN32
-    VirtualFree(base, 0, MEM_RELEASE);
-#else
-    munmap(base, size);
-#endif
+    buffer->flags |= RB_IO_BUFFER_FILE;
 }
 
 static void
@@ -234,7 +224,16 @@ io_buffer_free(struct rb_io_buffer *buffer)
         }
 
         if (buffer->flags & RB_IO_BUFFER_MAPPED) {
-            io_buffer_unmap(buffer->base, buffer->size);
+#ifdef _WIN32
+            if (buffer->flags & RB_IO_BUFFER_FILE) {
+                UnmapViewOfFile(buffer->base);
+            }
+            else {
+                VirtualFree(buffer->base, 0, MEM_RELEASE);
+            }
+#else
+            munmap(buffer->base, buffer->size);
+#endif
         }
 
         // Previously we had this, but we found out due to the way GC works, we
@@ -245,19 +244,20 @@ io_buffer_free(struct rb_io_buffer *buffer)
 
         buffer->base = NULL;
 
-#if defined(_WIN32)
-        if (buffer->mapping) {
-            CloseHandle(buffer->mapping);
-            buffer->mapping = NULL;
-        }
-#endif
         buffer->size = 0;
         buffer->flags = 0;
         buffer->source = Qnil;
 
         return 1;
     }
 
+#if defined(_WIN32)
+    if (buffer->mapping) {
+        CloseHandle(buffer->mapping);
+        buffer->mapping = NULL;
+    }
+#endif
+
     return 0;
 }
 
@@ -926,6 +926,10 @@ rb_io_buffer_to_s(VALUE self)
         rb_str_cat2(result, " MAPPED");
     }
 
+    if (buffer->flags & RB_IO_BUFFER_FILE) {
+        rb_str_cat2(result, " FILE");
+    }
+
     if (buffer->flags & RB_IO_BUFFER_SHARED) {
         rb_str_cat2(result, " SHARED");
     }

diff --git a/test/ruby/test_io_buffer.rb b/test/ruby/test_io_buffer.rb
@@ -519,23 +519,22 @@ def test_shared
   end
 
   def test_private
-    omit if RUBY_PLATFORM =~ /mswin|mingw/
+    tmpdir = Dir.tmpdir
+    buffer_path = File.join(tmpdir, "buffer.txt")
+    File.write(buffer_path, "Hello World")
 
-    Tempfile.create("buffer.txt") do |io|
-      io.write("Hello World")
-
-      buffer = IO::Buffer.map(io, nil, 0, IO::Buffer::PRIVATE)
+    File.open(buffer_path) do |file|
+      buffer = IO::Buffer.map(file, nil, 0, IO::Buffer::PRIVATE)
       assert buffer.private?
       refute buffer.readonly?
 
       buffer.set_string("J")
 
       # It was not changed because the mapping was private:
-      io.seek(0)
-      assert_equal "Hello World", io.read
+      file.seek(0)
+      assert_equal "Hello World", file.read
     ensure
       buffer&.free
-      io&.close
     end
   end
 end