Bug #22195
closedIO::Buffer read after free silently returns empty data instead of raising
Description
Reading from an IO::Buffer after calling #free does not raise. It behaves like an empty buffer:
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::AllocationErroragain, restoring the pre-3.4 documented behavior. #resizere-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
- 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
- Description updated (diff)
Updated by himura467 (Akito Shitara) about 2 months ago
Updated by ioquatix (Samuel Williams) 13 days ago
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:
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:
- It invalidates the object until it is explicitly reallocated; or
- 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.
Updated by himura467 (Akito Shitara) 13 days ago
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_valueandget_stringraisingIO::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
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
- Status changed from Open to Closed
Applied in changeset git|281c2d4c19f3e7a10772ae0e62ba01ae3695500a.
Document IO::Buffer#free empty state. (#18471)
[Bug #22195]