From c3f21194144b6dcb183929924b376a4df62199c2 Mon Sep 17 00:00:00 2001 From: BurdetteLamar Date: Sun, 27 Sep 2026 16:04:29 -0500 Subject: [PATCH 1/6] [DOC] Harmonize zero? methods --- file.c | 98 ++++++++++++++++++++++++++++++++------------- pathname_builtin.rb | 41 +++++++++++++------ 2 files changed, 98 insertions(+), 41 deletions(-) diff --git a/file.c b/file.c index ed51562de1071e..eb352896c7fe87 100644 --- a/file.c +++ b/file.c @@ -2401,39 +2401,53 @@ rb_file_file_p(VALUE obj, VALUE fname) } /* + * :markup: markdown + * call-seq: - * File.empty?(object) -> true or false * File.zero?(object) -> true or false + * File.empty?(object) -> true or false * - * Returns whether the given +object+ exists and has size zero. + * Returns whether the given `object` exists and has size zero. * - * The given +object+ may be the path to a directory (possibly non-existent): + * The given `object` may be the path to a file: * - * dirpath = 'foo' - * File.empty?(dirpath) # => false # Directory does not exist. - * dir = Dir.mkdir(dirpath) - * # The directory size is filesystem-dependent; - * # for a directory with no children, may or may not be zero. - * File.size(dirpath) # => 4096 - * File.empty?(dirpath) # => false + * ```ruby + * filepath = '/tmp/t.tmp' + * File.write(filepath, 'foo') # File has non-zero size. + * File.zero?(filepath) # => false + * File.truncate(filepath, 0) # File has zero size. + * File.zero?(filepath) # => true + * File.delete(filepath) # Clean up. + * ``` * - * The given +object+ may be the path to a file (possibly non-existent): + * The given `object` may be the path to a directory: * - * filepath = File.join(dirpath, 't.tmp') - * File.empty?(filepath) # => false # File does not exist. - * File.write(filepath, '') - * File.size(filepath) # => 0 - * File.empty?(filepath) # => true # File exists; size zero. - * File.size(dirpath) # => 4096 - * File.empty?(dirpath) # => false - * File.write(filepath, 'bar') - * File.size(filepath) # => 3 - * File.empty?(filepath) # => false # File exists; size non-zero. - * FileUtils.rm_rf(dirpath) # Clean up. + * ```ruby + * dirpath = '/tmp/foo' + * Dir.mkdir(dirpath) + * Dir.new(dirpath).children.size # => 0 + * # Size is filesystem-dependent; may or may not be zero. + * File.size(dirpath) # => 4096 + * File.zero?(dirpath) # => false + * filepath = '/tmp/foo/t.tmp' # => "/tmp/foo/t.tmp" + * File.write(filepath, 'foo') # Add a child. + * Dir.new(dirpath).children.size # => 1 + * File.size(dirpath) # => 4096 + * File.zero?(dirpath) # => false + * FileUtils.rm_rf(dirpath) # Clean up. + * ``` + * + * The given `object` may be an IO object: * - * The given +object+ may be an IO object: + * ```ruby + * File.zero?($stdin) # => true + * ``` * - * File.empty?($stdin) # => true + * The given object may be none of the above: + * + * ```ruby + * File.zero?('nosuch') # => false + * ``` * */ @@ -7410,12 +7424,40 @@ rb_stat_f(VALUE obj) } /* - * call-seq: - * stat.zero? -> true or false + * :markup: markdown + * + * call-seq: + * zero? -> true or false + * + * Returns whether the entry at the path in `self` has size zero. + * + * The entry may be a file: + * + * ```ruby + * filepath = '/tmp/t.tmp' + * File.write(filepath, 'foo') + * File.stat(filepath).zero? # => false + * File.truncate(filepath, 0) + * File.stat(filepath).zero? # => true + * File.delete(filepath) # Clean up. + * ``` * - * Returns +true+ if stat is a zero-length file; +false+ otherwise. + * The entry may be a directory: * - * File.stat("testfile").zero? #=> false + * ```ruby + * dirpath = '/tmp/foo' + * Dir.mkdir(dirpath) + * stat = File.stat(dirpath) + * # Size is filesystem-dependent; may or may not be zero. + * stat.size # => 4096 + * stat.zero? # => false + * filepath = File.join(dirpath, 't.tmp') # => "/tmp/foo/t.tmp" + * File.write(filepath, 'foo') + * stat = File.stat(dirpath) + * stat.size # => 4096 + * stat.zero? # => false + * FileUtils.rm_rf(dirpath) # Clean up. + * ``` * */ diff --git a/pathname_builtin.rb b/pathname_builtin.rb index 834888250e6b66..e3d4174b2fdb60 100644 --- a/pathname_builtin.rb +++ b/pathname_builtin.rb @@ -2796,24 +2796,39 @@ def writable_real?() FileTest.writable_real?(@path) end # call-seq: # zero? -> true or false # - # Returns whether the entry represented by `self` exists and has size zero: + # Returns whether the entry at the path in `self` exists and has size zero. # + # The entry may be a file: + # + # ```ruby + # pn = Pathname('/tmp/t.tmp') + # pn.write('foo') + # pn.zero? # => false + # pn.truncate(0) pn.zero? # => true + # pn.delete # Clean up. # ``` - # dir_pn = Pathname('example_dir') - # dir_pn.zero? # => false # Dir does not exist. + # + # The entry may be a directory: + # + # ```ruby + # dir_pn = Pathname('/tmp/foo') # dir_pn.mkdir - # dir_pn.zero? # => false # Directory never has size zero. - # dir_pn.empty? # => true # But this one is empty. + # dir_pn.children.size # => 0 + # # Size is filesystem-dependent; may or may not be zero. + # dir_pn.size # => 4096 + # dir_pn.zero? # => false + # file_pn = dir_pn / 't.tmp' # => # + # file_pn.write('foo') # Add a file. + # dir_pn.children.size # => 1 + # dir_pn.size # => 4096 + # dir_pn.zero? # => false + # dir_pn.rmtree + # ``` # - # file_pn = Pathname('example_dir/example.txt') - # file_pn.zero? # => false # File does not exist. - # file_pn.write('') - # file_pn.zero? # => true - # file_pn.write('foo') - # file_pn.zero? # => false + # The entry may be neither of the above: # - # file_pn.delete - # dir_pn.delete + # ```ruby + # Pathname('nosuch').zero? # => false # ``` # def zero?() FileTest.zero?(@path) end From 4d56c955c45d1306b5a13065712b98d6a898eab5 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Mon, 28 Sep 2026 15:07:20 +1300 Subject: [PATCH 2/6] Revalidate `IO::Buffer` byte access after Ruby callbacks. (#19086) Treat encoding coercion, `to_io`, and scheduler capability checks as callouts that can change the buffer before byte access begins. - Copy `get_string` bytes through the existing locked-for-reading helper after encoding coercion, without retaining metadata pointers across it. - Obtain the writable buffer after `to_io` conversion in `read` and `pread`. - Reacquire and validate the buffer before native I/O fallback when the scheduler does not handle `read`, `write`, `pread`, or `pwrite`. Add regression coverage using the existing buffer and slice APIs. Scheduler probes shrink a slice without freeing its underlying storage, making stale length handling observable without accessing released memory. Also cover encoding callbacks and frozen-state changes during I/O coercion. This is an independent correctness extraction from #18911. It introduces no new classes, slice representation, or public API. --- io_buffer.c | 61 ++++++++++++++++++++------------ test/fiber/test_io_buffer.rb | 50 ++++++++++++++++++++++++++ test/ruby/test_io_buffer.rb | 68 ++++++++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+), 23 deletions(-) diff --git a/io_buffer.c b/io_buffer.c index 000e0cf9a6f7d6..f9cf37f5644141 100644 --- a/io_buffer.c +++ b/io_buffer.c @@ -3243,6 +3243,23 @@ io_buffer_copy(int argc, VALUE *argv, VALUE self) return rb_io_buffer_locked_for_reading(source, io_buffer_copy_from_readable, (VALUE)&arguments); } +struct io_buffer_get_string_arguments { + size_t offset; + size_t length; + rb_encoding *encoding; +}; + +static VALUE +io_buffer_get_string_locked(const void *base, size_t size, VALUE _arguments) +{ + struct io_buffer_get_string_arguments *arguments = (void *)_arguments; + if (size_sum_is_bigger_than(arguments->offset, arguments->length, size)) { + rb_raise(rb_eArgError, "Specified offset+length is bigger than the buffer size!"); + } + const char *data = base ? (const char *)base + arguments->offset : NULL; + return rb_enc_str_new(data, arguments->length, arguments->encoding); +} + /* * call-seq: get_string([offset, [length, [encoding]]]) -> string * @@ -3262,26 +3279,13 @@ io_buffer_get_string(int argc, VALUE *argv, VALUE self) { rb_check_arity(argc, 0, 3); - size_t offset, length; - struct rb_io_buffer *buffer = io_buffer_extract_offset_length(self, argc, argv, &offset, &length); - - rb_encoding *encoding; - if (argc >= 3) { - encoding = rb_find_encoding(argv[2]); - } - else { - encoding = rb_ascii8bit_encoding(); - } - - const void *base; - size_t size; - io_buffer_get_bytes_for_reading(buffer, &base, &size); + struct io_buffer_get_string_arguments arguments; + io_buffer_extract_offset_length(self, argc, argv, &arguments.offset, &arguments.length); - io_buffer_validate_range(buffer, offset, length); - - const char *data = base ? (const char*)base + offset : NULL; - - return rb_enc_str_new(data, length, encoding); + // Encoding coercion may invoke Ruby and change the buffer. Retain no + // metadata or byte pointer across it; resolve under the subsequent lock. + arguments.encoding = argc >= 3 ? rb_find_encoding(argv[2]) : rb_ascii8bit_encoding(); + return rb_io_buffer_locked_for_reading(self, io_buffer_get_string_locked, (VALUE)&arguments); } /* @@ -3482,9 +3486,8 @@ io_buffer_read_internal(void *_argument) VALUE rb_io_buffer_read(VALUE self, VALUE io, size_t offset, size_t length) { - struct rb_io_buffer *buffer = get_io_buffer_for_writing(self); - io = rb_io_get_io(io); + struct rb_io_buffer *buffer = get_io_buffer_for_writing(self); io_buffer_validate_range(buffer, offset, length); @@ -3497,6 +3500,10 @@ rb_io_buffer_read(VALUE self, VALUE io, size_t offset, size_t length) if (!UNDEF_P(result)) { return result; } + + // The scheduler capability check can invoke Ruby and change the buffer. + buffer = get_io_buffer_for_writing(self); + io_buffer_validate_range(buffer, offset, length); } void *base; @@ -3573,9 +3580,8 @@ io_buffer_pread_internal(void *_argument) VALUE rb_io_buffer_pread(VALUE self, VALUE io, rb_off_t from, size_t offset, size_t length) { - struct rb_io_buffer *buffer = get_io_buffer_for_writing(self); - io = rb_io_get_io(io); + struct rb_io_buffer *buffer = get_io_buffer_for_writing(self); io_buffer_validate_range(buffer, offset, length); @@ -3588,6 +3594,9 @@ rb_io_buffer_pread(VALUE self, VALUE io, rb_off_t from, size_t offset, size_t le if (!UNDEF_P(result)) { return result; } + + buffer = get_io_buffer_for_writing(self); + io_buffer_validate_range(buffer, offset, length); } void *base; @@ -3682,6 +3691,9 @@ rb_io_buffer_write(VALUE self, VALUE io, size_t offset, size_t length) if (!UNDEF_P(result)) { return result; } + + buffer = get_io_buffer(self); + io_buffer_validate_range(buffer, offset, length); } const void *base; @@ -3765,6 +3777,9 @@ rb_io_buffer_pwrite(VALUE self, VALUE io, rb_off_t from, size_t offset, size_t l if (!UNDEF_P(result)) { return result; } + + buffer = get_io_buffer(self); + io_buffer_validate_range(buffer, offset, length); } const void *base; diff --git a/test/fiber/test_io_buffer.rb b/test/fiber/test_io_buffer.rb index d52baaf6c7f382..fafbf38fc6cd08 100644 --- a/test/fiber/test_io_buffer.rb +++ b/test/fiber/test_io_buffer.rb @@ -3,10 +3,60 @@ require_relative 'scheduler' require 'timeout' +require 'tempfile' class TestFiberIOBuffer < Test::Unit::TestCase MESSAGE = "Hello World" + class ResizingProbeScheduler < IOBufferScheduler + attr_accessor :probe_method, :probe_buffer + + def respond_to?(name, include_private = false) + if name == @probe_method && @probe_buffer + buffer = @probe_buffer + @probe_buffer = nil + buffer.resize(0) + return false + end + super + end + end + + [:read, :write, :pread, :pwrite].each do |method| + define_method("test_native_#{method}_fallback_revalidates_range_after_scheduler_probe") do + buffer = IO::Buffer.new(8) + begin + buffer.set_string("AAAABBBB") + view = buffer.slice(0, 4) + Tempfile.create("io-buffer-fallback") do |file| + file.binmode + file.write("cccccccc") + file.rewind + + Thread.new do + scheduler = ResizingProbeScheduler.new + Fiber.set_scheduler(scheduler) + Fiber.schedule do + scheduler.probe_method = :"io_#{method}" + scheduler.probe_buffer = view + args = [file] + args << 0 if [:pread, :pwrite].include?(method) + assert_raise(ArgumentError, method.to_s) {view.public_send(method, *args)} + end + end.value + + assert_predicate view, :empty? + refute_predicate buffer, :locked? + assert_equal "AAAABBBB", buffer.get_string + file.rewind + assert_equal "cccccccc", file.read + end + ensure + buffer.free + end + end + end + def test_read_write_blocking omit "UNIXSocket is not defined!" unless defined?(UNIXSocket) diff --git a/test/ruby/test_io_buffer.rb b/test/ruby/test_io_buffer.rb index c20853d692131f..4c1305accf3fa9 100644 --- a/test/ruby/test_io_buffer.rb +++ b/test/ruby/test_io_buffer.rb @@ -1006,6 +1006,74 @@ def to_str assert_equal 1_000_000, encoding.buffer.get_string(0, 1_000_000, encoding).length end + def test_get_string_resolves_storage_after_encoding_coercion + buffer = IO::Buffer.new(8) + previous = nil + buffer.set_string("original") + encoding = Object.new + encoding.define_singleton_method(:to_str) do + # Retain the old allocation to make stale reads observable without + # accessing released memory. The replacement cannot reuse its address. + previous = buffer.transfer + buffer.resize(8) + buffer.set_string("replaced") + "BINARY" + end + + assert_equal "repl", buffer.get_string(0, 4, encoding) + assert_equal "original", previous.get_string + refute_predicate buffer, :locked? + ensure + previous&.free + buffer&.free + end + + def test_get_string_range_error_preserves_existing_lock + buffer = IO::Buffer.new(8) + view = buffer.slice(0, 4) + encoding = Object.new + encoding.define_singleton_method(:to_str) do + view.resize(0) + "BINARY" + end + + buffer.locked do + assert_raise(ArgumentError) {view.get_string(0, 4, encoding)} + assert_predicate buffer, :locked? + end + refute_predicate buffer, :locked? + ensure + buffer&.free + end + + def test_read_rechecks_permissions_after_io_coercion + [:read, :pread].each do |method| + buffer = IO::Buffer.new(8) + begin + buffer.set_string("original") + view = buffer.slice(0, 4) + Tempfile.create("io-buffer-coercion") do |file| + file.binmode + file.write("data") + file.rewind + proxy = Object.new + proxy.define_singleton_method(:to_io) do + view.freeze + file + end + args = [proxy] + args << 0 if method == :pread + assert_raise(FrozenError, method.to_s) {view.public_send(method, *args)} + assert_equal "original", buffer.get_string + assert_equal "data", file.read + refute_predicate buffer, :locked? + end + ensure + buffer.free + end + end + end + def test_zero_length_get_string buffer = IO::Buffer.new.slice(0, 0) assert_equal "", buffer.get_string From 11c91bf4189f07ade9e817554a2498db2b00fc4d Mon Sep 17 00:00:00 2001 From: Nobuyoshi Nakada Date: Mon, 28 Sep 2026 10:51:47 +0900 Subject: [PATCH 3/6] [DOC] Update dates in updated man pages only --- .github/workflows/check_misc.yml | 4 ++-- common.mk | 11 ++++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/.github/workflows/check_misc.yml b/.github/workflows/check_misc.yml index dd574ba2d7982e..fddc48024e5dfb 100644 --- a/.github/workflows/check_misc.yml +++ b/.github/workflows/check_misc.yml @@ -55,8 +55,8 @@ jobs: - name: Check if date in man pages is up-to-date run: | git fetch origin --depth=1 "${GITHUB_OLD_SHA}" - git diff --exit-code --name-only "${GITHUB_OLD_SHA}" HEAD -- man || - make V=1 GIT=git BASERUBY=ruby update-man-date + make V=1 GIT=git BASERUBY=ruby update-man-date \ + MANPAGES_TO_UPDATE="$(git diff --exit-code --name-only "${GITHUB_OLD_SHA}" HEAD -- man)" git diff --color --no-ext-diff --ignore-submodules --exit-code -- man env: GITHUB_OLD_SHA: ${{ github.event.pull_request.base.sha }} diff --git a/common.mk b/common.mk index e1357be829449c..8e94eac2619e8b 100644 --- a/common.mk +++ b/common.mk @@ -2029,9 +2029,14 @@ sudo-precheck: PHONY update-man-date: PHONY $(Q) $(BASERUBY) -I"$(tooldir)/lib" -rvcs -i -p \ - -e 'BEGIN{@vcs=VCS.detect(ARGV.shift)}' \ - -e '$$_.sub!(/^(\.Dd ).*/){$$1+@vcs.author_date(@vcs.relative_to(ARGF.path)).strftime("%B %d, %Y")}' \ - "$(srcdir)" "$(srcdir)"/man/*.1 + -C "$(srcdir)" \ + -e 'BEGIN{@vcs=VCS.detect; ARGV.replace(Dir.glob(ARGV))}' \ + -e '$$_.sub!(/^\.Dd \K.*/){' \ + -e 'STDOUT.puts "Updating #{ARGF.path}"' \ + -e '@vcs.author_date(@vcs.relative_to(ARGF.path)).strftime("%B %d, %Y")' \ + -e '}' \ + $(MANPAGES_TO_UPDATE) +MANPAGES_TO_UPDATE = man/*.1 .PHONY: ChangeLog ChangeLog: From 05c266871d459b7a32c2f4755296312fdf617b99 Mon Sep 17 00:00:00 2001 From: Hiroshi SHIBATA Date: Mon, 28 Sep 2026 10:20:07 +0900 Subject: [PATCH 4/6] [ruby/rubygems] Send X-Gemfile-Source on redirects only within the same origin Gem::RemoteFetcher and the Bundler downloader resent the header on every redirect, including one to another origin. It carries the source URI that Bundler replaces with a mirror, credentials included, so it is meant for the mirror's origin only, and RFC 9110 (Section 15.4) asks a client following a redirect to consider removing such fields. https://github.com/ruby/rubygems/commit/f52afd1b92 Co-Authored-By: Claude Opus 5.5 --- lib/bundler/fetcher/downloader.rb | 4 ++ lib/rubygems/remote_fetcher.rb | 7 ++- .../bundler/fetcher/downloader_spec.rb | 7 ++- spec/bundler/bundler/fetcher_spec.rb | 35 +++++++++++ test/rubygems/test_gem_remote_fetcher.rb | 60 +++++++++++++++++++ 5 files changed, 108 insertions(+), 5 deletions(-) diff --git a/lib/bundler/fetcher/downloader.rb b/lib/bundler/fetcher/downloader.rb index 5457fbbbcc7299..e0dd8eec0abf5f 100644 --- a/lib/bundler/fetcher/downloader.rb +++ b/lib/bundler/fetcher/downloader.rb @@ -59,6 +59,10 @@ def fetch(uri, headers = {}, counter = 0) if [new_uri.scheme, new_uri.host, new_uri.port] == [uri.scheme, uri.host, uri.port] new_uri.user = uri.user new_uri.password = uri.password + else + # see Gem::RemoteFetcher#fetch_http. A nil value removes the header + # that ConnectionPools adds from its override_headers. + headers = headers.merge("X-Gemfile-Source" => nil) end fetch(new_uri, headers, counter + 1) when Gem::Net::HTTPRequestedRangeNotSatisfiable diff --git a/lib/rubygems/remote_fetcher.rb b/lib/rubygems/remote_fetcher.rb index fbe56348ce5551..b60c94e217d84a 100644 --- a/lib/rubygems/remote_fetcher.rb +++ b/lib/rubygems/remote_fetcher.rb @@ -221,7 +221,7 @@ def fetch_file(uri, *_) ## # HTTP Fetcher. Dispatched by +fetch_path+. Use it instead. - def fetch_http(uri, last_modified = nil, head = false, depth = 0) + def fetch_http(uri, last_modified = nil, head = false, depth = 0, headers = self.headers) fetch_type = head ? Gem::Net::HTTP::Head : Gem::Net::HTTP::Get response = request uri, fetch_type, last_modified do |req| headers.each {|k,v| req.add_field(k,v) } @@ -246,8 +246,11 @@ def fetch_http(uri, last_modified = nil, head = false, depth = 0) # see Gem::CompactIndexClient::HTTPFetcher#fetch same_origin = [location.scheme, location.host, location.port] == [uri.scheme, uri.host, uri.port] location.userinfo = uri.userinfo if same_origin && !location.userinfo + # X-Gemfile-Source carries the source URI that Bundler mirrors, + # credentials included, and is meant for the mirror's origin only. + headers = headers.except("X-Gemfile-Source") unless same_origin - fetch_http(location, last_modified, head, depth + 1) + fetch_http(location, last_modified, head, depth + 1, headers) else custom_error = response["X-Error-Message"] error_detail = custom_error || response.message diff --git a/spec/bundler/bundler/fetcher/downloader_spec.rb b/spec/bundler/bundler/fetcher/downloader_spec.rb index dcb59854c41bbf..47cc346a181f29 100644 --- a/spec/bundler/bundler/fetcher/downloader_spec.rb +++ b/spec/bundler/bundler/fetcher/downloader_spec.rb @@ -39,12 +39,13 @@ context "when the request response is a Gem::Net::HTTPRedirection" do let(:http_response) { Gem::Net::HTTPRedirection.new(httpv, 308, "Moved") } + let(:options) { {} } before { http_response["location"] = "http://www.redirect-uri.com/api/v2/endpoint" } it "should try to fetch the redirect uri and iterate the # requests counter" do expect(subject).to receive(:fetch).with(Gem::URI("http://www.uri-to-fetch.com/api/v2/endpoint"), options, 0).and_call_original - expect(subject).to receive(:fetch).with(Gem::URI("http://www.redirect-uri.com/api/v2/endpoint"), options, 1) + expect(subject).to receive(:fetch).with(Gem::URI("http://www.redirect-uri.com/api/v2/endpoint"), { "X-Gemfile-Source" => nil }, 1) subject.fetch(uri, options, counter) end @@ -67,7 +68,7 @@ it "should not set the user and password for the redirect uri" do expect(subject).to receive(:fetch).with(uri, options, 0).and_call_original - expect(subject).to receive(:fetch).with(Gem::URI("https://www.uri-to-fetch.com:8443/api/v1/endpoint"), options, 1) + expect(subject).to receive(:fetch).with(Gem::URI("https://www.uri-to-fetch.com:8443/api/v1/endpoint"), { "X-Gemfile-Source" => nil }, 1) subject.fetch(uri, options, counter) end end @@ -79,7 +80,7 @@ it "should not set the user and password for the redirect uri" do expect(subject).to receive(:fetch).with(uri, options, 0).and_call_original - expect(subject).to receive(:fetch).with(Gem::URI("https://www.uri-to-fetch.com:8080/api/v1/endpoint"), options, 1) + expect(subject).to receive(:fetch).with(Gem::URI("https://www.uri-to-fetch.com:8080/api/v1/endpoint"), { "X-Gemfile-Source" => nil }, 1) subject.fetch(uri, options, counter) end end diff --git a/spec/bundler/bundler/fetcher_spec.rb b/spec/bundler/bundler/fetcher_spec.rb index 8bac5548b9241c..e8c8348539d303 100644 --- a/spec/bundler/bundler/fetcher_spec.rb +++ b/spec/bundler/bundler/fetcher_spec.rb @@ -57,6 +57,41 @@ fetcher.send(:connection).override_headers["X-Gemfile-Source"] ).to eq("http://zombo.com") end + + it "stops sending the 'X-Gemfile-Source' header once a redirect leaves the origin" do + previous_client = Gem::Request::ConnectionPools.client + require_rack_test + require_relative "../support/artifice/helpers/endpoint" + require_relative "../support/artifice/helpers/artifice" + + sent = [] + endpoint = Class.new(Endpoint) do + get "/:hop" do + sent << [request.host, env["HTTP_X_GEMFILE_SOURCE"]] + case params[:hop] + when "first" then redirect "https://gems.example.org/second" + when "second" then redirect "https://cdn.example.org/third" + when "third" then redirect "https://cdn.example.org/last" + else "body" + end + end + end + + Artifice.activate_with(endpoint) + Gem::Request::ConnectionPools.client = Gem::Net::HTTP + + fetcher.send(:downloader).fetch(Gem::URI("https://gems.example.org/first")) + + expect(sent).to eq([ + ["gems.example.org", "http://zombo.com"], + ["gems.example.org", "http://zombo.com"], + ["cdn.example.org", nil], + ["cdn.example.org", nil], + ]) + ensure + Artifice.deactivate + Gem::Request::ConnectionPools.client = previous_client + end end context "when there is no rubygems source mirror set" do diff --git a/test/rubygems/test_gem_remote_fetcher.rb b/test/rubygems/test_gem_remote_fetcher.rb index 3e1e3a3e79ffcf..1b1bfa48e4e3a4 100644 --- a/test/rubygems/test_gem_remote_fetcher.rb +++ b/test/rubygems/test_gem_remote_fetcher.rb @@ -704,6 +704,66 @@ def res.body assert_equal [url, "https://gems.example.com:8080/real"], fetcher.instance_variable_get(:@requested) end + def test_fetch_http_redirects_keep_gemfile_source_on_same_origin + fetcher = Gem::RemoteFetcher.new nil + @fetcher = fetcher + source = "https://user:pass@source.example.com/" + fetcher.headers["X-Gemfile-Source"] = source + + def fetcher.request(uri, request_class, last_modified = nil) + req = request_class.new uri.request_uri + yield req + (@sent ||= []) << req["X-Gemfile-Source"] + if @sent.size > 1 + res = Gem::Net::HTTPOK.new nil, 200, nil + def res.body + "real_path" + end + else + res = Gem::Net::HTTPFound.new nil, 302, nil + res.add_field "Location", "https://gems.example.com/real" + end + res + end + + data = fetcher.fetch_http Gem::URI.parse("https://gems.example.com/redirect") + + assert_equal "real_path", data + assert_equal [source, source], fetcher.instance_variable_get(:@sent) + end + + def test_fetch_http_redirects_drop_gemfile_source_on_another_origin + fetcher = Gem::RemoteFetcher.new nil + @fetcher = fetcher + source = "https://user:pass@source.example.com/" + fetcher.headers["X-Gemfile-Source"] = source + + def fetcher.request(uri, request_class, last_modified = nil) + req = request_class.new uri.request_uri + yield req + (@sent ||= []) << req["X-Gemfile-Source"] + case @sent.size + when 1 + res = Gem::Net::HTTPFound.new nil, 302, nil + res.add_field "Location", "https://gems.example.com:8443/redirect" + when 2 + res = Gem::Net::HTTPFound.new nil, 302, nil + res.add_field "Location", "https://gems.example.com:8443/real" + else + res = Gem::Net::HTTPOK.new nil, 200, nil + def res.body + "real_path" + end + end + res + end + + data = fetcher.fetch_http Gem::URI.parse("https://gems.example.com/redirect") + + assert_equal "real_path", data + assert_equal [source, nil, nil], fetcher.instance_variable_get(:@sent) + end + def test_fetch_http_redirects_to_non_https_redacts_location fetcher = Gem::RemoteFetcher.new nil @fetcher = fetcher From c568687e452d474e8bc916597b3e9a2d1ae14757 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Mon, 28 Sep 2026 17:13:38 +1300 Subject: [PATCH 5/6] Treat empty `IO::Buffer` ranges as non-overlapping. (#19088) --- io_buffer.c | 13 ++-- spec/ruby/core/io/buffer/overlap_spec.rb | 65 +++++++++++++++++++ test/ruby/test_io_buffer.rb | 79 ++++++++++++++++++++++++ 3 files changed, 152 insertions(+), 5 deletions(-) create mode 100644 spec/ruby/core/io/buffer/overlap_spec.rb diff --git a/io_buffer.c b/io_buffer.c index f9cf37f5644141..5a0f78ac3ef554 100644 --- a/io_buffer.c +++ b/io_buffer.c @@ -4015,11 +4015,14 @@ io_buffer_not(VALUE self) static inline int io_buffer_overlaps(const struct rb_io_buffer *a, const struct rb_io_buffer *b) { - if (a->base > b->base) { - return io_buffer_overlaps(b, a); - } - - return (b->base >= a->base) && (b->base < (void*)((unsigned char *)a->base + a->size)); + if (a->size == 0 || b->size == 0) return 0; + + // Compare address differences, without ordering unrelated pointers or + // constructing end addresses that could overflow. + uintptr_t a_start = (uintptr_t)a->base; + uintptr_t b_start = (uintptr_t)b->base; + if (a_start <= b_start) return b_start - a_start < a->size; + return a_start - b_start < b->size; } static inline void diff --git a/spec/ruby/core/io/buffer/overlap_spec.rb b/spec/ruby/core/io/buffer/overlap_spec.rb new file mode 100644 index 00000000000000..70a34e5f065ae4 --- /dev/null +++ b/spec/ruby/core/io/buffer/overlap_spec.rb @@ -0,0 +1,65 @@ +require_relative '../../../spec_helper' + +[:and!, :or!, :xor!].each do |operation| + describe "IO::Buffer##{operation} range checking" do + before :each do + @buffer = IO::Buffer.new(8) + @buffer.set_string("abcdefgh") + end + + after :each do + @buffer.free + end + + ruby_version_is "4.1" do + it "accepts an empty destination at the start, inside, or end of the mask" do + [0, 4, 8].each do |offset| + destination = @buffer.slice(offset, 0) + destination.should_not.null? + destination.public_send(operation, @buffer).should.equal?(destination) + destination.should.empty? + @buffer.get_string.should == "abcdefgh" + end + end + end + + it "rejects actual overlap in either address order" do + [ + [[0, 4], [0, 4]], + [[0, 4], [2, 4]], + [[2, 4], [0, 4]], + [[1, 6], [2, 2]], + [[2, 2], [1, 6]], + ].each do |left, right| + destination = @buffer.slice(*left) + mask = @buffer.slice(*right) + -> { destination.public_send(operation, mask) }.should.raise( + IO::Buffer::MaskError, "Mask overlaps source buffer!" + ) + end + @buffer.get_string.should == "abcdefgh" + end + + # Adjacent-range overlap was fixed in Ruby 3.4 ([Bug #20933]). + ruby_version_is "3.4" do + it "accepts adjacent non-empty ranges in either address order" do + [[[0, 4], [4, 4]], [[4, 4], [0, 4]]].each do |left, right| + @buffer.set_string("abcdefgh") + destination = @buffer.slice(*left) + mask = @buffer.slice(*right) + original_mask = mask.get_string + destination.public_send(operation, mask).should.equal?(destination) + mask.get_string.should == original_mask + end + end + end + + it "rejects empty masks even when the destination is empty" do + [@buffer, @buffer.slice(4, 0)].each do |destination| + -> { destination.public_send(operation, @buffer.slice(4, 0)) }.should.raise( + IO::Buffer::MaskError, "Zero-length mask given!" + ) + end + end + end +end diff --git a/test/ruby/test_io_buffer.rb b/test/ruby/test_io_buffer.rb index 4c1305accf3fa9..ea09512f99b6e4 100644 --- a/test/ruby/test_io_buffer.rb +++ b/test/ruby/test_io_buffer.rb @@ -1547,6 +1547,85 @@ def test_inplace_operators assert_equal IO::Buffer.for("\xce\xcd\xcc\xcb\xce\xcd\xcc\xcb\xce\xcd"), source.dup.not! end + [:and!, :or!, :xor!].each do |operation| + define_method("test_#{operation.to_s.delete('!')}_empty_range_does_not_overlap") do + buffer = IO::Buffer.new(8) + buffer.set_string("abcdefgh") + [0, 4, 8].each do |offset| + view = buffer.slice(offset, 0) + refute_predicate view, :null? + assert_nothing_raised do + assert_same view, view.public_send(operation, buffer) + end + assert_predicate view, :empty? + assert_equal "abcdefgh", buffer.get_string + end + ensure + buffer&.free + end + end + + def test_inplace_operators_reject_nonempty_overlap + buffer = IO::Buffer.new(8) + buffer.set_string("abcdefgh") + ranges = [ + [[0, 4], [0, 4]], + [[0, 4], [2, 4]], + [[2, 4], [0, 4]], + [[1, 6], [2, 2]], + [[2, 2], [1, 6]], + ] + [:and!, :or!, :xor!].each do |operation| + ranges.each do |left, right| + error = assert_raise(IO::Buffer::MaskError) do + buffer.slice(*left).public_send(operation, buffer.slice(*right)) + end + assert_equal "Mask overlaps source buffer!", error.message + end + assert_equal "abcdefgh", buffer.get_string + end + ensure + buffer&.free + end + + def test_inplace_operators_accept_disjoint_ranges + buffer = IO::Buffer.new(8) + ranges = [ + [[0, 4], [4, 4]], + [[4, 4], [0, 4]], + [[0, 2], [6, 2]], + [[6, 2], [0, 2]], + ] + [:and!, :or!, :xor!].each do |operation| + ranges.each do |left, right| + buffer.set_string("abcdefgh") + target = buffer.slice(*left) + mask = buffer.slice(*right) + original_mask = mask.get_string + assert_same target, target.public_send(operation, mask) + assert_equal original_mask, mask.get_string + end + end + ensure + buffer&.free + end + + def test_inplace_operators_still_reject_empty_masks + buffer = IO::Buffer.new(8) + buffer.set_string("abcdefgh") + [:and!, :or!, :xor!].each do |operation| + [buffer, buffer.slice(4, 0)].each do |target| + error = assert_raise(IO::Buffer::MaskError) do + target.public_send(operation, buffer.slice(4, 0)) + end + assert_equal "Zero-length mask given!", error.message + end + end + assert_equal "abcdefgh", buffer.get_string + ensure + buffer&.free + end + def test_operators_raise_on_freed_self inner = IO::Buffer.new(IO::Buffer::PAGE_SIZE) slice = inner.slice(0, 8) From 1692691913212019ab184943f37a9a1788d2fe12 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Mon, 28 Sep 2026 19:16:19 +1300 Subject: [PATCH 6/6] Make `IO::Buffer` slices offset-based so they survive source relocation. (#19090) --- ext/-test-/io_buffer/io_buffer.c | 19 ++ include/ruby/io/buffer.h | 5 +- io_buffer.c | 296 ++++++++++++++-------- spec/ruby/core/io/buffer/free_spec.rb | 10 +- spec/ruby/core/io/buffer/null_spec.rb | 14 +- spec/ruby/core/io/buffer/readonly_spec.rb | 31 +++ spec/ruby/core/io/buffer/resize_spec.rb | 44 ++++ spec/ruby/core/io/buffer/transfer_spec.rb | 11 +- spec/ruby/core/io/buffer/valid_spec.rb | 57 ++++- test/ruby/test_io_buffer.rb | 84 +++++- 10 files changed, 451 insertions(+), 120 deletions(-) diff --git a/ext/-test-/io_buffer/io_buffer.c b/ext/-test-/io_buffer/io_buffer.c index 60ff067db03d1b..3fd595f394a25a 100644 --- a/ext/-test-/io_buffer/io_buffer.c +++ b/ext/-test-/io_buffer/io_buffer.c @@ -205,6 +205,23 @@ io_buffer_free_locked(VALUE self, VALUE buffer) return rb_io_buffer_free_locked(buffer); } +static VALUE +io_buffer_get_bytes_flags(VALUE self, VALUE buffer) +{ + void *base; + size_t size; + return UINT2NUM(rb_io_buffer_get_bytes(buffer, &base, &size)); +} + +static VALUE +io_buffer_get_bytes_address(VALUE self, VALUE buffer) +{ + void *base; + size_t size; + rb_io_buffer_get_bytes(buffer, &base, &size); + return PTR2NUM(base); +} + void Init_io_buffer(void) { @@ -229,4 +246,6 @@ Init_io_buffer(void) rb_define_singleton_method(mIOBuffer, "unlock", io_buffer_unlock, 1); rb_define_singleton_method(mIOBuffer, "new_locked", io_buffer_new_locked, 1); rb_define_singleton_method(mIOBuffer, "free_locked", io_buffer_free_locked, 1); + rb_define_singleton_method(mIOBuffer, "get_bytes_flags", io_buffer_get_bytes_flags, 1); + rb_define_singleton_method(mIOBuffer, "get_bytes_address", io_buffer_get_bytes_address, 1); } diff --git a/include/ruby/io/buffer.h b/include/ruby/io/buffer.h index 2d0244411b9cb6..007aaa94592a0b 100644 --- a/include/ruby/io/buffer.h +++ b/include/ruby/io/buffer.h @@ -96,8 +96,9 @@ VALUE rb_io_buffer_free(VALUE self); // not exactly one. VALUE rb_io_buffer_free_locked(VALUE self); -// Access the internal buffer and flags. Validates the pointers. If the returned -// base is NULL, the returned size is always zero. +// Access the buffer and flags. Validates the pointers. READONLY reflects both +// the view's own restriction and its source's current permissions. If the +// returned base is NULL, the returned size is always zero. // The pointers may not remain valid if the source buffer is manipulated. // Consider using rb_io_buffer_lock if needed. enum rb_io_buffer_flags rb_io_buffer_get_bytes(VALUE self, void **base, size_t *size); diff --git a/io_buffer.c b/io_buffer.c index 5a0f78ac3ef554..f1c8b86efd3898 100644 --- a/io_buffer.c +++ b/io_buffer.c @@ -57,8 +57,17 @@ enum { }; struct rb_io_buffer { + // Without a source (source == Qnil), this is an absolute pointer to owned + // or borrowed memory. Ownership is determined by the flags, not by the + // presence of a source. + // + // With a source (String or IO::Buffer), this is a byte offset into it. + // The absolute pointer is resolved as `source_base + offset` on demand + // (see io_buffer_try_get_bytes), so source relocation preserves the + // logical range. Use io_buffer_slice_offset() to read the offset. void *base; size_t size; + enum rb_io_buffer_flags flags; // Locking and unlocking are performed with the GVL held. size_t lock_count; @@ -207,7 +216,14 @@ io_buffer_zero(struct rb_io_buffer *buffer) static void io_buffer_initialize(VALUE self, struct rb_io_buffer *buffer, void *base, size_t size, enum rb_io_buffer_flags flags, VALUE source) { - if (base) { + if (source != Qnil) { + // The buffer is backed by another object (e.g. a String). Here `base` + // is a byte *offset* into that source rather than an absolute pointer, + // and the memory is not owned by this buffer. The absolute base is + // resolved on demand as `source_base + offset` (see + // io_buffer_try_get_bytes), so it stays valid if the source moves. + } + else if (base) { // If we are provided a pointer, we use it. } else if (size) { @@ -356,6 +372,14 @@ io_buffer_slice_p(struct rb_io_buffer *buffer) return rb_typeddata_is_kind_of(buffer->source, &rb_io_buffer_type); } +// For a slice (io_buffer_slice_p), the `base` field stores the byte offset of +// the slice within its (root) source rather than an absolute pointer. +static inline size_t +io_buffer_slice_offset(const struct rb_io_buffer *buffer) +{ + return (uintptr_t)buffer->base; +} + // Return the buffer which owns the lock count. A slice backed by another // buffer shares that source buffer's lock count. Other external sources, such // as strings, manage their own lifetime and do not share buffer lock state. @@ -532,7 +556,9 @@ static VALUE io_buffer_for_make_instance(VALUE klass, VALUE string, enum rb_io_b if (!(flags & RB_IO_BUFFER_READONLY)) rb_str_modify(string); - io_buffer_initialize(instance, buffer, RSTRING_PTR(string), RSTRING_LEN(string), flags, string); + // String-backed buffers are offset-based: pass offset 0 (the whole string), + // resolved as `RSTRING_PTR(string) + offset` on demand. + io_buffer_initialize(instance, buffer, (void *)0, RSTRING_LEN(string), flags, string); return instance; } @@ -1078,35 +1104,62 @@ rb_io_buffer_initialize(int argc, VALUE *argv, VALUE self) return self; } +// Resolve the current base pointer and size of a buffer, following slice +// indirection. A slice of another IO::Buffer stores a logical `offset` into +// its (root) source and resolves its base as `source_base + offset` here, so +// it stays valid even if the source's allocation is moved by a resize. A +// String-backed buffer stores an offset into its pinned String and is +// range-validated. Returns non-zero if the buffer is valid, and sets +// `*base`/`*size` accordingly (NULL/0 when invalid). static int -io_buffer_validate_slice(VALUE source, void *base, size_t size) -{ +io_buffer_try_get_bytes(struct rb_io_buffer *buffer, void **base, size_t *size) +{ + // Symmetry: the resolved base is `source_base + buffer->base`, where + // `buffer->base` is a byte offset into the source (and is the absolute + // pointer for an owning buffer, whose source contributes 0): + // + // owning (source == Qnil): source_base = 0, base = buffer->base + // String-backed: source_base = RSTRING_PTR, base = 0 + offset + // slice (IO::Buffer): source_base = resolved(source), base = + offset + // + // Note: a valid buffer may still resolve to a NULL base (e.g. an empty + // buffer, or an empty slice of an empty source). Validity means "the range + // exists in the source"; whether the resolved base is NULL is a separate + // property (see IO::Buffer#null?). + if (buffer->source == Qnil) { + // Owning (root) buffer: `base` is absolute (source contributes 0). + *base = buffer->base; + *size = buffer->size; + return 1; + } + + // Source-backed buffer: `base` is an offset into the source. void *source_base = NULL; size_t source_size = 0; - if (RB_TYPE_P(source, T_STRING)) { - RSTRING_GETMEM(source, source_base, source_size); + if (io_buffer_slice_p(buffer)) { + // Slice of another IO::Buffer: resolve the (root) source recursively. + if (!io_buffer_try_get_bytes(get_io_buffer(buffer->source), &source_base, &source_size)) { + *base = NULL; + *size = 0; + return 0; + } } else { - rb_io_buffer_get_bytes(source, &source_base, &source_size); + // String-backed buffer: the (pinned) String content is the source. + RSTRING_GETMEM(buffer->source, source_base, source_size); } - uintptr_t source_address = (uintptr_t)source_base; - uintptr_t address = (uintptr_t)base; - - // Base is out of range: - if (address < source_address) return 0; - - uintptr_t offset = address - source_address; - - // Base is beyond the end of the source: - if (offset > source_size) return 0; - - // End is beyond the end of the source: - if (size > source_size - (size_t)offset) return 0; + size_t offset = io_buffer_slice_offset(buffer); + if (offset <= source_size && buffer->size <= source_size - offset) { + *base = source_base ? (char *)source_base + offset : NULL; + *size = buffer->size; + return 1; + } - // It seems okay: - return 1; + *base = NULL; + *size = 0; + return 0; } static int @@ -1114,7 +1167,9 @@ io_buffer_validate(struct rb_io_buffer *buffer) { if (buffer->source != Qnil) { // Only slices incur this overhead, unfortunately... better safe than sorry! - return io_buffer_validate_slice(buffer->source, buffer->base, buffer->size); + void *base = NULL; + size_t size = 0; + return io_buffer_try_get_bytes(buffer, &base, &size); } else { return 1; @@ -1126,18 +1181,12 @@ rb_io_buffer_get_bytes(VALUE self, void **base, size_t *size) { struct rb_io_buffer *buffer = get_io_buffer(self); - if (io_buffer_validate(buffer)) { - if (buffer->base) { - *base = buffer->base; - *size = buffer->size; - - return buffer->flags; - } + if (io_buffer_try_get_bytes(buffer, base, size)) { + enum rb_io_buffer_flags flags = buffer->flags; + if (io_buffer_readonly_p(buffer)) flags |= RB_IO_BUFFER_READONLY; + return flags; } - *base = NULL; - *size = 0; - return 0; } @@ -1145,8 +1194,7 @@ rb_io_buffer_get_bytes(VALUE self, void **base, size_t *size) static void io_buffer_validate_for_writing(struct rb_io_buffer *buffer) { - if (buffer->flags & RB_IO_BUFFER_READONLY || - (!NIL_P(buffer->source) && OBJ_FROZEN(buffer->source))) { + if (io_buffer_readonly_p(buffer)) { rb_raise(rb_eIOBufferAccessError, "Buffer is not writable!"); } @@ -1170,13 +1218,7 @@ io_buffer_get_bytes_for_writing(struct rb_io_buffer *buffer, void **base, size_t { io_buffer_validate_for_writing(buffer); - if (buffer->base) { - *base = buffer->base; - *size = buffer->size; - } else { - *base = NULL; - *size = 0; - } + io_buffer_try_get_bytes(buffer, base, size); } void @@ -1200,13 +1242,9 @@ io_buffer_get_bytes_for_reading(struct rb_io_buffer *buffer, const void **base, { io_buffer_validate_for_reading(buffer); - if (buffer->base) { - *base = buffer->base; - *size = buffer->size; - } else { - *base = NULL; - *size = 0; - } + void *writable_base = NULL; + io_buffer_try_get_bytes(buffer, &writable_base, size); + *base = writable_base; } void @@ -1234,9 +1272,14 @@ rb_io_buffer_to_s(VALUE self) VALUE result = rb_str_new_cstr("#<"); rb_str_append(result, rb_class_name(CLASS_OF(self))); - rb_str_catf(result, " %p+%"PRIdSIZE, buffer->base, buffer->size); - if (buffer->base == NULL) { + // Resolve the current base (following slice indirection) for display: + void *base = NULL; + size_t size = 0; + io_buffer_try_get_bytes(buffer, &base, &size); + rb_str_catf(result, " %p+%"PRIdSIZE, base, buffer->size); + + if (base == NULL) { rb_str_cat2(result, " NULL"); } @@ -1268,7 +1311,7 @@ rb_io_buffer_to_s(VALUE self) rb_str_cat2(result, " PRIVATE"); } - if (buffer->flags & RB_IO_BUFFER_READONLY) { + if (io_buffer_readonly_p(buffer)) { rb_str_cat2(result, " READONLY"); } @@ -1367,7 +1410,9 @@ rb_io_buffer_inspect(VALUE self) VALUE result = rb_io_buffer_to_s(self); - if (io_buffer_validate(buffer)) { + void *base = NULL; + size_t total = 0; + if (io_buffer_try_get_bytes(buffer, &base, &total) && base) { // Limit the maximum size generated by inspect: size_t size = buffer->size; int clamped = 0; @@ -1377,7 +1422,7 @@ rb_io_buffer_inspect(VALUE self) clamped = 1; } - io_buffer_hexdump(result, RB_IO_BUFFER_INSPECT_HEXDUMP_WIDTH, buffer->base, size, 0, 0); + io_buffer_hexdump(result, RB_IO_BUFFER_INSPECT_HEXDUMP_WIDTH, base, size, 0, 0); if (clamped) { rb_str_catf(result, "\n(and %" PRIuSIZE " more bytes not printed)", buffer->size - size); @@ -1404,17 +1449,18 @@ rb_io_buffer_size(VALUE self) /* * call-seq: valid? -> true or false * - * A buffer which is not a slice is always valid, including a null buffer. - * Only slices can become invalid. + * A buffer without a source is always valid, including a null buffer. A + * source-backed buffer is valid when its offset and length fit within its + * source's current size. * - * A slice is valid when its entire recorded memory range is contained within - * its source's current memory range. It can become invalid if its source is - * freed, transferred, shrunk past the slice, or reallocated at a different - * address. Validity is dynamic: if the source later contains the same address - * range again, the slice becomes valid again. + * Relocating a source does not invalidate a source-backed buffer. Freeing, + * transferring, or shrinking the source can make it invalid; if the same + * source later grows to include the range again, the buffer becomes valid + * and refers to the current contents at its original offset. * - * #valid?, #null? and #empty? describe independent properties. For example, - * an invalid slice can still have a non-null address and a non-zero size. + * An empty source-backed range at offset zero can be valid even when its + * source has no storage. #valid?, #null? and #empty? describe distinct + * properties: a buffer can be valid, null, and empty at the same time. */ static VALUE rb_io_buffer_valid_p(VALUE self) @@ -1446,7 +1492,11 @@ rb_io_buffer_null_p(VALUE self) { struct rb_io_buffer *buffer = get_io_buffer(self); - return RBOOL(buffer->base == NULL); + void *base = NULL; + size_t size = 0; + io_buffer_try_get_bytes(buffer, &base, &size); + + return RBOOL(base == NULL); } /* @@ -1498,8 +1548,8 @@ rb_io_buffer_external_p(VALUE self) * requested size is less than the IO::Buffer::PAGE_SIZE and it was not * requested to be mapped on creation. * - * Internal buffers can be resized, and such an operation will typically - * invalidate all slices, but not always. + * Internal buffers can be resized. Slices remain valid if their ranges still + * fit within the resized buffer, including when its storage is relocated. */ static VALUE rb_io_buffer_internal_p(VALUE self) @@ -1519,8 +1569,8 @@ rb_io_buffer_internal_p(VALUE self) * IO::Buffer::MAPPED flag or if the size was at least IO::Buffer::PAGE_SIZE, * or backed by a file if created with ::map. * - * Mapped buffers can usually be resized, and such an operation will typically - * invalidate all slices, but not always. + * Mapped buffers can usually be resized. Slices remain valid if their ranges + * still fit within the resized buffer, including when its mapping is moved. */ static VALUE rb_io_buffer_mapped_p(VALUE self) @@ -1615,7 +1665,20 @@ rb_io_buffer_private_p(VALUE self) static int io_buffer_readonly_p(struct rb_io_buffer *buffer) { - return buffer->flags & RB_IO_BUFFER_READONLY; + if (buffer->flags & RB_IO_BUFFER_READONLY) + return 1; + + VALUE source = buffer->source; + if (NIL_P(source)) + return 0; + + if (OBJ_FROZEN(source)) + return 1; + + if (RB_TYPE_P(source, T_STRING)) + return 0; + + return io_buffer_readonly_p(get_io_buffer(source)); } /* @@ -1626,6 +1689,9 @@ io_buffer_readonly_p(struct rb_io_buffer *buffer) * * A buffer created by IO::Buffer.for without a block is read-only, as is one * backed by a frozen string or a read-only file. + * + * A slice derives read-only access from its current source. Replacing the + * source's storage can therefore change the slice's read-only status. */ static VALUE rb_io_buffer_readonly_p(VALUE self) @@ -1941,10 +2007,12 @@ rb_io_buffer_hexdump(int argc, VALUE *argv, VALUE self) VALUE result = Qnil; - if (io_buffer_validate(buffer) && buffer->base) { + void *base = NULL; + size_t size = 0; + if (io_buffer_try_get_bytes(buffer, &base, &size) && base) { result = rb_str_buf_new(io_buffer_hexdump_output_size(width, length, 1)); - io_buffer_hexdump(result, width, buffer->base, offset+length, offset, 1); + io_buffer_hexdump(result, width, base, offset+length, offset, 1); } return result; @@ -1958,16 +2026,22 @@ rb_io_buffer_slice(struct rb_io_buffer *buffer, VALUE self, size_t offset, size_ VALUE instance = rb_io_buffer_type_allocate(rb_class_of(self)); struct rb_io_buffer *slice = get_io_buffer(instance); - slice->flags |= (buffer->flags & RB_IO_BUFFER_READONLY); - slice->base = buffer->base ? (char*)buffer->base + offset : NULL; slice->size = length; - // Slices retain their root buffer. If this buffer is already a slice, - // retain its root directly rather than building a chain of slices: + // Slices retain their root buffer and store a logical offset into it, + // rather than an absolute base pointer, so they remain valid across a + // resize that relocates the source. The base is resolved on demand as + // `source_base + offset`. If this buffer is already a slice, retain its + // root directly rather than building a chain of slices, folding this + // slice's offset into the existing one: if (io_buffer_slice_p(buffer)) { + // Fold this slice's offset into the parent slice's offset (relative to + // the shared root), stored in `base`: + slice->base = (void *)(uintptr_t)(io_buffer_slice_offset(buffer) + offset); RB_OBJ_WRITE(instance, &slice->source, buffer->source); } else { + slice->base = (void *)(uintptr_t)offset; RB_OBJ_WRITE(instance, &slice->source, self); } @@ -1983,6 +2057,8 @@ rb_io_buffer_slice(struct rb_io_buffer *buffer, VALUE self, size_t offset, size_ * The slicing happens without copying memory. The slice retains its root * buffer and becomes invalid if that root is freed, transferred, resized so * that the slice is outside its bounds, or otherwise invalidated. + * Reallocating the root's storage does not invalidate the slice; it keeps the + * same logical offset into the root. * * If the offset is not given, it will be zero. If the offset is negative, it * will raise an ArgumentError. @@ -2121,28 +2197,20 @@ io_buffer_resize_slice(struct rb_io_buffer *slice, size_t size) { struct rb_io_buffer *source = get_io_buffer(slice->source); - if (!io_buffer_validate(source)) { - rb_raise(rb_eIOBufferInvalidatedError, "Buffer is invalid!"); - } - - if (source->base == NULL || slice->base == NULL) { - rb_raise(rb_eIOBufferInvalidatedError, "Buffer is invalid!"); - } - - uintptr_t source_address = (uintptr_t)source->base; - uintptr_t slice_address = (uintptr_t)slice->base; + void *source_base = NULL; + size_t source_size = 0; - if (slice_address < source_address) { + if (!io_buffer_try_get_bytes(source, &source_base, &source_size)) { rb_raise(rb_eIOBufferInvalidatedError, "Buffer is invalid!"); } - uintptr_t offset = slice_address - source_address; + size_t offset = io_buffer_slice_offset(slice); - if (offset > source->size) { + if (offset > source_size) { rb_raise(rb_eIOBufferInvalidatedError, "Buffer is invalid!"); } - if (size > source->size - (size_t)offset) { + if (size > source_size - offset) { rb_raise(rb_eArgError, "Resized slice exceeds its source buffer!"); } @@ -2168,15 +2236,19 @@ rb_io_buffer_resize(VALUE self, size_t size) rb_raise(rb_eIOBufferLockedError, "Cannot resize locked buffer!"); } + // An external buffer (including a String-backed buffer, whose base is an + // offset into the source) does not own its memory and cannot be resized. + // This must be checked before the empty-buffer case below, since a + // String-backed buffer at offset 0 has a NULL `base`. + if (buffer->flags & RB_IO_BUFFER_EXTERNAL) { + rb_raise(rb_eIOBufferAccessError, "Cannot resize external buffer!"); + } + if (buffer->base == NULL) { io_buffer_initialize(self, buffer, NULL, size, io_flags_for_size(size), Qnil); return; } - if (buffer->flags & RB_IO_BUFFER_EXTERNAL) { - rb_raise(rb_eIOBufferAccessError, "Cannot resize external buffer!"); - } - if (size == 0) { io_buffer_release(buffer); return; @@ -4013,16 +4085,22 @@ io_buffer_not(VALUE self) } static inline int -io_buffer_overlaps(const struct rb_io_buffer *a, const struct rb_io_buffer *b) +io_buffer_overlaps(struct rb_io_buffer *a, struct rb_io_buffer *b) { - if (a->size == 0 || b->size == 0) return 0; + // Resolve the current base pointers (following slice indirection): + void *a_base = NULL, *b_base = NULL; + size_t a_size = 0, b_size = 0; + if (!io_buffer_try_get_bytes(a, &a_base, &a_size)) return 0; + if (!io_buffer_try_get_bytes(b, &b_base, &b_size)) return 0; + + if (a_size == 0 || b_size == 0 || a_base == NULL || b_base == NULL) return 0; - // Compare address differences, without ordering unrelated pointers or - // constructing end addresses that could overflow. - uintptr_t a_start = (uintptr_t)a->base; - uintptr_t b_start = (uintptr_t)b->base; - if (a_start <= b_start) return b_start - a_start < a->size; - return a_start - b_start < b->size; + // Compare integer address differences, without ordering unrelated C + // pointers or constructing end addresses that could overflow. + uintptr_t a_start = (uintptr_t)a_base; + uintptr_t b_start = (uintptr_t)b_base; + if (a_start <= b_start) return b_start - a_start < a_size; + return a_start - b_start < b_size; } static inline void @@ -4075,7 +4153,7 @@ io_buffer_and_inplace(VALUE self, VALUE mask) size_t mask_size; io_buffer_get_bytes_for_reading(mask_buffer, &mask_base, &mask_size); - memory_and_inplace(base, size, mask_buffer->base, mask_buffer->size); + memory_and_inplace(base, size, (unsigned char *)mask_base, mask_size); return self; } @@ -4123,7 +4201,7 @@ io_buffer_or_inplace(VALUE self, VALUE mask) size_t mask_size; io_buffer_get_bytes_for_reading(mask_buffer, &mask_base, &mask_size); - memory_or_inplace(base, size, mask_buffer->base, mask_buffer->size); + memory_or_inplace(base, size, (unsigned char *)mask_base, mask_size); return self; } @@ -4171,7 +4249,7 @@ io_buffer_xor_inplace(VALUE self, VALUE mask) size_t mask_size; io_buffer_get_bytes_for_reading(mask_buffer, &mask_base, &mask_size); - memory_xor_inplace(base, size, mask_buffer->base, mask_buffer->size); + memory_xor_inplace(base, size, (unsigned char *)mask_base, mask_size); return self; } @@ -4277,7 +4355,9 @@ io_buffer_memory_view_get(VALUE self, rb_memory_view_t *view, int flags) { struct rb_io_buffer *buffer = get_io_buffer(self); - if (buffer->base == NULL || !io_buffer_validate(buffer)) { + void *base = NULL; + size_t size = 0; + if (!io_buffer_try_get_bytes(buffer, &base, &size) || base == NULL) { return false; } @@ -4289,7 +4369,7 @@ io_buffer_memory_view_get(VALUE self, rb_memory_view_t *view, int flags) readonly = false; } } - rb_memory_view_init_as_byte_array(view, self, buffer->base, buffer->size, readonly); + rb_memory_view_init_as_byte_array(view, self, base, buffer->size, readonly); if (flags & RUBY_MEMORY_VIEW_FORMAT) { view->format = "C"; } @@ -4338,7 +4418,9 @@ io_buffer_memory_view_available_p(VALUE self) { struct rb_io_buffer *buffer = get_io_buffer(self); - return buffer->base != NULL && io_buffer_validate(buffer); + void *base = NULL; + size_t size = 0; + return io_buffer_try_get_bytes(buffer, &base, &size) && base != NULL; } static const rb_memory_view_entry_t io_buffer_memory_view_entry = { diff --git a/spec/ruby/core/io/buffer/free_spec.rb b/spec/ruby/core/io/buffer/free_spec.rb index fe3a774201cbd4..24aadb677808ba 100644 --- a/spec/ruby/core/io/buffer/free_spec.rb +++ b/spec/ruby/core/io/buffer/free_spec.rb @@ -127,8 +127,16 @@ slice = buffer.slice(0, 2) buffer.free - slice.null?.should == false slice.valid?.should == false + + ruby_version_is ""..."4.1" do + slice.null?.should == false + end + + ruby_version_is "4.1" do + # Offset-based slices resolve their base from the freed source. + slice.null?.should == true + end end end end diff --git a/spec/ruby/core/io/buffer/null_spec.rb b/spec/ruby/core/io/buffer/null_spec.rb index 83837014922b1c..d4f24267a71cf0 100644 --- a/spec/ruby/core/io/buffer/null_spec.rb +++ b/spec/ruby/core/io/buffer/null_spec.rb @@ -25,12 +25,22 @@ @buffer.slice(3, 0).null?.should == false end - it "is false for an invalid slice with a recorded address" do + it "reflects an invalid slice whose source was freed" do @buffer = IO::Buffer.new(4) slice = @buffer.slice(0, 2) @buffer.free slice.valid?.should == false - slice.null?.should == false + + ruby_version_is ""..."4.1" do + # Address-based slices retain the recorded (non-null) address. + slice.null?.should == false + end + + ruby_version_is "4.1" do + # Offset-based slices resolve their base from the (now freed) source, + # so they resolve to a null address. + slice.null?.should == true + end end end diff --git a/spec/ruby/core/io/buffer/readonly_spec.rb b/spec/ruby/core/io/buffer/readonly_spec.rb index 4eefc9f29fac4a..41850e62ec855f 100644 --- a/spec/ruby/core/io/buffer/readonly_spec.rb +++ b/spec/ruby/core/io/buffer/readonly_spec.rb @@ -25,4 +25,35 @@ @buffer = IO::Buffer.new(0) @buffer.readonly?.should == false end + + ruby_version_is "4.1" do + it "reflects its source's current permissions" do + @buffer = IO::Buffer.new(8) + slice = @buffer.slice(2, 4) + + @buffer.free + @buffer.send(:initialize, 8, IO::Buffer::INTERNAL | IO::Buffer::READONLY) + + slice.should.valid? + slice.should.readonly? + -> { slice.set_string("test") }.should.raise(IO::Buffer::AccessError) + + @buffer.free + @buffer.resize(8) + + slice.should.valid? + slice.should_not.readonly? + slice.set_string("test") + @buffer.get_string.should == "\0\0test\0\0" + end + + it "is true for a slice whose source is frozen" do + buffer = IO::Buffer.new(8) + slice = buffer.slice(2, 4) + buffer.freeze + + slice.should.readonly? + -> { slice.set_string("test") }.should.raise(IO::Buffer::AccessError) + end + end end diff --git a/spec/ruby/core/io/buffer/resize_spec.rb b/spec/ruby/core/io/buffer/resize_spec.rb index 229d46f93b00a3..233aac503c2f44 100644 --- a/spec/ruby/core/io/buffer/resize_spec.rb +++ b/spec/ruby/core/io/buffer/resize_spec.rb @@ -165,6 +165,50 @@ ruby_version_is "4.1" do context "with a slice of a buffer" do + it "keeps an empty view valid when its source is null" do + @buffer = IO::Buffer.new(0) + slice = @buffer.slice(0, 0) + + slice.resize(0) + slice.should.valid? + slice.should.null? + slice.should.empty? + + @buffer.resize(4) + slice.resize(4) + slice.get_string.should == "\0" * 4 + end + + it "restores validity by resizing an offset-zero slice to zero" do + @buffer = IO::Buffer.new(8) + slice = @buffer.slice(0, 4) + @buffer.free + + slice.should_not.valid? + -> { slice.resize(1) }.should.raise(ArgumentError) + slice.resize(0).should.equal?(slice) + slice.should.valid? + slice.should.null? + + @buffer.resize(8) + @buffer.set_string("abcdefgh") + slice.resize(4) + slice.get_string.should == "abcd" + end + + it "rejects resizing a slice whose offset is beyond a null source" do + @buffer = IO::Buffer.new(8) + slice = @buffer.slice(2, 4) + @buffer.free + + -> { slice.resize(0) }.should.raise(IO::Buffer::InvalidatedError) + slice.should_not.valid? + + @buffer.resize(8) + @buffer.set_string("abcdefgh") + slice.get_string.should == "cdef" + end + it "changes the size of the view without modifying the source" do @buffer = IO::Buffer.for("abcdef").dup slice = @buffer.slice(2, 2) diff --git a/spec/ruby/core/io/buffer/transfer_spec.rb b/spec/ruby/core/io/buffer/transfer_spec.rb index 82e1380aab6f94..97f552e417f5b1 100644 --- a/spec/ruby/core/io/buffer/transfer_spec.rb +++ b/spec/ruby/core/io/buffer/transfer_spec.rb @@ -121,8 +121,17 @@ slice = buffer.slice(0, 2) @buffer = buffer.transfer - slice.null?.should == false slice.valid?.should == false + + ruby_version_is ""..."4.1" do + slice.null?.should == false + end + + ruby_version_is "4.1" do + # Offset-based slices resolve their base from the now-nullified source. + slice.null?.should == true + end + -> { slice.get_string }.should.raise(IO::Buffer::InvalidatedError, "Buffer has been invalidated!") end end diff --git a/spec/ruby/core/io/buffer/valid_spec.rb b/spec/ruby/core/io/buffer/valid_spec.rb index 4bab75653053db..a27fe4525a3c60 100644 --- a/spec/ruby/core/io/buffer/valid_spec.rb +++ b/spec/ruby/core/io/buffer/valid_spec.rb @@ -41,8 +41,9 @@ end end - # "A buffer becomes invalid if it is a slice of another buffer (or string) - # which has been freed or re-allocated at a different address." + # A slice tracks a logical [offset, length) range of its source. It becomes + # invalid only if the source is freed, or resized so that the slice's range + # no longer fits; it survives a resize that merely relocates the source. context "with a slice" do it "is true for a slice of a live buffer" do @buffer = IO::Buffer.new(4) @@ -56,24 +57,61 @@ slice = @buffer.slice(0, 0) slice.valid?.should == true + slice.null?.should == true + slice.empty?.should == true slice.get_string.should == "" end - it "tracks whether its empty range exists in the source" do + it "survives a resize that relocates the source" do @buffer = IO::Buffer.new(0) slice = @buffer.slice(0, 0) slice.valid?.should == true + # Growing the source may relocate its allocation, but the slice's + # [0, 0) range still exists within it, so the slice stays valid: @buffer.resize(1) - slice.valid?.should == false - -> { slice.get_string }.should.raise(IO::Buffer::InvalidatedError) + slice.valid?.should == true + slice.get_string.should == "" @buffer.resize(0) slice.valid?.should == true slice.get_string.should == "" end + it "keeps referring to the same range after the source is relocated" do + @buffer = IO::Buffer.new(8) + blocker = IO::Buffer.new(8) + begin + @buffer.set_string("ABCDEFGH") + slice = @buffer.slice(2, 4) + slice.get_string.should == "CDEF" + + # A large grow is likely to relocate the source allocation; the slice + # continues to refer to bytes [2, 6) of the (preserved) contents: + @buffer.resize(1 << 20) + slice.valid?.should == true + slice.get_string.should == "CDEF" + slice.set_string("test") + @buffer.get_string(0, 8).should == "ABtestGH" + ensure + blocker&.free + end + end + + it "can become valid again when the source grows to include its range" do + @buffer = IO::Buffer.new(8) + @buffer.set_string("ABCDEFGH") + slice = @buffer.slice(4, 4) + + @buffer.resize(4) + slice.valid?.should == false + + @buffer.resize(8) + slice.valid?.should == true + slice.get_string.should == "\0" * 4 + end + it "is false when its empty range no longer belongs to the source" do @buffer = IO::Buffer.new(1) slice = @buffer.slice(1, 0) @@ -112,8 +150,15 @@ @buffer.free slice.valid?.should == false - slice.null?.should == false slice.empty?.should == false + + ruby_version_is ""..."4.1" do + slice.null?.should == false + end + + ruby_version_is "4.1" do + slice.null?.should == true + end end it "can be true for a non-null empty slice" do diff --git a/test/ruby/test_io_buffer.rb b/test/ruby/test_io_buffer.rb index ea09512f99b6e4..4acbc506a2a23e 100644 --- a/test/ruby/test_io_buffer.rb +++ b/test/ruby/test_io_buffer.rb @@ -601,9 +601,34 @@ def test_resize_invalidated_slice slice = inner.slice(0, 8) inner.free - assert_raise(IO::Buffer::InvalidatedError) do + assert_raise(ArgumentError) do slice.resize(16) end + + slice.resize(0) + assert_predicate slice, :valid? + assert_predicate slice, :null? + assert_predicate slice, :empty? + slice.free + end + + def test_resize_invalidated_slice_beyond_null_source + inner = IO::Buffer.new(IO::Buffer::PAGE_SIZE) + slice = inner.slice(2, 8) + inner.free + + assert_raise(IO::Buffer::InvalidatedError) do + slice.resize(0) + end + refute_predicate slice, :valid? + + inner.resize(IO::Buffer::PAGE_SIZE) + inner.set_string("abcdefghij") + assert_predicate slice, :valid? + assert_equal "cdefghij", slice.get_string + ensure + inner&.free unless inner&.null? + slice&.free unless slice&.null? end def test_resize_after_free @@ -750,6 +775,63 @@ def test_slice_readonly assert_equal "Hello World", hello end + def test_slice_readonly_permission_follows_source_replacement + buffer = IO::Buffer.new(8) + slice = buffer.slice(2, 4) + + buffer.free + buffer.send(:initialize, 8, IO::Buffer::INTERNAL | IO::Buffer::READONLY) + + assert_predicate slice, :valid? + assert_predicate slice, :readonly? + assert_equal IO::Buffer::READONLY, Bug::IOBuffer.get_bytes_flags(slice) & IO::Buffer::READONLY + assert_raise(IO::Buffer::AccessError) {slice.set_string("test")} + + buffer.free + buffer.resize(8) + + assert_predicate slice, :valid? + refute_predicate slice, :readonly? + assert_equal 0, Bug::IOBuffer.get_bytes_flags(slice) & IO::Buffer::READONLY + slice.set_string("test") + assert_equal "\0\0test\0\0", buffer.get_string + ensure + buffer&.free unless buffer&.null? + end + + def test_slice_of_frozen_source_is_readonly + buffer = IO::Buffer.new(8) + slice = buffer.slice(2, 4) + buffer.freeze + + assert_predicate slice, :readonly? + assert_equal IO::Buffer::READONLY, Bug::IOBuffer.get_bytes_flags(slice) & IO::Buffer::READONLY + assert_raise(IO::Buffer::AccessError) {slice.set_string("test")} + end + + def test_slice_tracks_same_range_after_source_reallocation + buffer = IO::Buffer.new(8) + blocker = IO::Buffer.new(8) + buffer.set_string("ABCDEFGH") + slice = buffer.slice(2, 4) + + original_address = Bug::IOBuffer.get_bytes_address(buffer) + buffer.resize(1 << 20) + relocated_address = Bug::IOBuffer.get_bytes_address(buffer) + omit "resize did not relocate the allocation" if relocated_address == original_address + + assert_predicate slice, :valid? + assert_equal "CDEF", slice.get_string + slice.set_string("test") + assert_equal "ABtestGH", buffer.get_string(0, 8) + assert_true MemoryViewTestUtils.set_data(slice, 1, "?".ord) + assert_equal "ABt?stGH", buffer.get_string(0, 8) + ensure + blocker&.free + slice&.free unless slice&.null? + buffer&.free unless buffer&.null? + end + def test_string_backed_slice_is_invalidated_when_root_is_freed buffer = IO::Buffer.for("Hello World") slice = buffer.slice(0, 5)