Project

General

Profile

Actions

Bug #22195

closed

IO::Buffer read after free silently returns empty data instead of raising

Bug #22195: IO::Buffer read after free silently returns empty data instead of raising

Added by himura467 (Akito Shitara) about 2 months ago. Updated 11 days ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:126083]

Description

Reading from an IO::Buffer after calling #free does not raise. It behaves like an empty buffer:

buffer = IO::Buffer.for("Hello World")
buffer.free
buffer.get_string # => "" (no error)

Why this looks like an unintended regression

Originally (https://github.com/ruby/ruby/commit/e30920354f, Ruby 3.1–3.3), accessing a freed buffer raised IO::Buffer::AllocationError ("The buffer is not allocated!").

https://github.com/ruby/ruby/commit/c5cf4d4e12 made zero-length buffer operations succeed instead of raising, for [Bug #19542] and [Bug #18805]. Since a freed buffer also has base == NULL, size == 0, it was caught in the same code path and stopped raising too. Neither ticket mentions freed buffers, so this appears to be an accidental side effect rather than a decision.
Note that the rdoc of #free still promises that access after free raises.

Proposal

How about marking a buffer internally as freed when its memory is released, so that:

  • Any byte access raises IO::Buffer::AllocationError again, restoring the pre-3.4 documented behavior.
  • #resize re-allocates and clears the mark, as #free's rdoc promises ("You can resize a freed buffer to re-allocate it").
  • The zero-length buffer semantics from https://github.com/ruby/ruby/pull/9532 are preserved.

Updated by himura467 (Akito Shitara) about 2 months ago Actions #1

  • Subject changed from `IO::Buffer` read after free silently returns empty data instead of raising to IO::Buffer read after free silently returns empty data instead of raising

Updated by himura467 (Akito Shitara) about 2 months ago Actions #2

  • Description updated (diff)

Updated by ioquatix (Samuel Williams) 13 days ago Actions #4 [ruby-core:126467]

Thank you for reporting this. I have reviewed the proposed FREED flag and the NACK use case, and I am not convinced that distinguishing a freed buffer from an empty, unallocated buffer is the right semantic model.

An IO::Buffer is fundamentally described by a base pointer and a size. The state {NULL, 0} is a valid empty span: an operation over the range [0, 0) does not dereference the pointer. This is the same state produced by IO::Buffer.new(0) and, after #18432, resize(0). Zero-length operations such as get_string returning "" are therefore well-defined and safe.

Calling free also releases the backing allocation and leaves {NULL, 0}. Adding a separate FREED bit makes two physically and operationally equivalent empty states behave differently based only on how they were reached:

buffer.resize(0)
buffer.get_string # => ""

buffer.free
buffer.get_string # raises AllocationError

There is no memory-safety difference here because neither state retains a pointer that can be dereferenced. null? already reports that no backing allocation exists, so code that specifically requires allocated storage can check it.

The NACK example is primarily about enforcing the lifetime of borrowed request memory. Its issue acknowledges that memory safety is already preserved and that the undesirable result is a silent empty value after application code retains a request-scoped buffer. I think lifetime invalidation is better represented directly rather than by changing the meaning of free for every IO::Buffer. With the recently merged slice-root semantics, for example, a bridge can retain a private root buffer, expose slices, and free the root at the end of the request. Any retained slice then raises IO::Buffer::InvalidatedError.

I think we should first decide what free means conceptually:

  1. It invalidates the object until it is explicitly reallocated; or
  2. It releases its storage and resets it to the canonical empty/null state.

My current preference is the second model. It is simpler, agrees with the existing {NULL, 0} representation, and keeps safe zero-length operations composable. If a caller needs stronger lifetime enforcement for borrowed memory, that should use explicit invalidation or ownership semantics rather than an internal historical flag.

For that reason, I do not think we should merge the FREED flag without a concrete case that cannot be addressed through null?, request-level lifetime tracking, or root/slice invalidation.

Perhaps as a 3rd option, it's better to just assign nil to your reference to the buffer, e.g.

if buffer = @buffer
  @buffer = nil
  buffer.free
end

Updated by himura467 (Akito Shitara) 13 days ago Actions #5 [ruby-core:126469]

Thanks for the review.

I filed this because the rdoc for #free says that access after free raises, while it does not, so either the implementation or the documentation had to be wrong:

  • "After the buffer is freed, no further operations can be performed on it."
  • "You can resize a freed buffer to re-allocate it."
  • a worked example showing get_value and get_string raising IO::Buffer::AllocationError.

That is the first of the two models you listed: invalidating the object until it is explicitly reallocated. If the current behavior is the intended one, I have no objection to the semantics, and I withdraw the proposal.

In that case, I think the documentation needs to match the implementation.

Updated by himura467 (Akito Shitara) 13 days ago · Edited Actions #6 [ruby-core:126470]

I think one thing here still needs fixing.

IO::Buffer.new(0) and IO::Buffer.new(0).slice(0, 0) are both {NULL, 0}, but they do not behave the same. On master (4.1.0dev, 37325e9f7a):

IO::Buffer.new(0).get_string              # => ""
IO::Buffer.for("").slice(0, 0).get_string # => ""
IO::Buffer.new.slice(0, 0).get_string     # => ""
IO::Buffer.new(0).slice(0, 0).get_string  # => IO::Buffer::InvalidatedError
IO::Buffer.new(0).slice(0, 0).valid?      # => false

io_buffer_initialize returns early when base and size are both zero, so the root keeps base == NULL, and io_buffer_validate_slice rejects any slice whose source has a NULL base, without looking at the length. The slice is therefore invalid as soon as it is created, although nothing was freed or transferred and no pointer is ever dereferenced.

#18805 asked that a buffer with size 0 always return an empty string, and #19542 asked for dup, <=>, each and get_values to work on empty buffers. All of them work on IO::Buffer.new(0) and raise on IO::Buffer.new(0).slice(0, 0). The fix in GH-9532 went into io_buffer_get_bytes_for_reading and io_buffer_get_bytes_for_writing, which a slice of an unallocated buffer never reaches: validation rejects it first.

Updated by Anonymous 11 days ago Actions #7

  • Status changed from Open to Closed

Applied in changeset git|281c2d4c19f3e7a10772ae0e62ba01ae3695500a.


Document IO::Buffer#free empty state. (#18471)

[Bug #22195]

Actions

Also available in: PDF Atom