Repository navigation
Conversation
Converting an argument (`to_int`, `to_str`, `to_s`, ...) may call back into Ruby code, which may call `StringIO#reopen` on the same stream. `reopen(other)` replaces the underlying `struct StringIO` and frees the old one, so a pointer fetched before the conversion became stale. Fetch the pointer again after the conversions, and after yielding in `each_line`. In `strio_init`, let `rb_io_extract_modeenc` write into a local instead of `ptr->flags`. Reported-by: Zabith (https://hackerone.com/nmzabith) Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Stream replacement during callbacks can bypass read-mode and closed-stream checks, unexpectedly close shared streams, and retain incompatible separators that produce invalid strings.
4 open issues
What changed in this PR
This PR addresses stale-pointer risks in StringIO when argument conversions or iterator blocks call Ruby code that reopens the same stream.
Changes:
- Refresh underlying stream pointers after conversions and
each_lineyields. - Extract initialization mode flags into a local variable before updating stream data.
- Add regression tests for reopening during callbacks and rejecting reads into the replacement stream’s underlying string.
| File | Description |
|---|---|
| test/stringio/test_stringio.rb | Adds regression tests for reopening during conversions and iteration. |
| ext/stringio/stringio.c | Refreshes stream data after callbacks and updates initialization and read-buffer handling. |
Review effort: Balanced
Comment on lines
+604
to
+606
| ptr->flags &= ~FMODE_READWRITE; | ||
| StringValue(string); | ||
| ptr = StringIO(self); |
Member
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
| } | ||
| switch (NIL_P(whence) ? 0 : NUM2LONG(whence)) { | ||
| w = NIL_P(whence) ? 0 : NUM2LONG(whence); | ||
| ptr = StringIO(self); |
| } | ||
|
|
||
| while (!NIL_P(line = strio_getline(&arg, ptr))) { | ||
| while (!NIL_P(line = strio_getline(&arg, StringIOForRead(self)))) { |
| } | ||
|
|
||
| while (!NIL_P(line = strio_getline(&arg, ptr))) { | ||
| while (!NIL_P(line = strio_getline(&arg, StringIOForRead(self)))) { |
nobu
reviewed
Nov 12, 2026
| VALUE v = rb_funcall(c, '&', 1, INT2FIX(0xff)); | ||
| const char cc = NUM2INT(v) & 0xFF; | ||
| strio_unget_bytes(ptr, &cc, 1); | ||
| c = rb_str_new(&cc, 1); |
Member
There was a problem hiding this comment.
c is known to be an Integer here, so isn't NUM2INT safe?
Member
There was a problem hiding this comment.
NUM2INT() is safe but rb_funcall(c, '&', 1, INT2FIX(0xff)) may call redefined Integer#&.
`string=` cleared the read/write flags before converting the argument. If the conversion reopens the receiver, the flags of the old stream are cleared but never restored, because the new flags are set on the replacement. Another stream that shares the old data (e.g. one made by `dup`) is left closed. The clearing is a leftover of the old code that closed the stream for a `nil` argument. The flags are overwritten after the conversion, so just remove it, and fetch the data only once after the conversion. This also stops leaving the stream closed when the conversion raises. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Apply the same fix as the C extension to the JRuby extension. `StringIO#reopen(other)` replaces `ptr`, so data fetched before an argument conversion may belong to the old stream. Convert arguments before taking the lock and fetch `ptr` again after the conversions in `read`, `pread`, `seek`, `string=`, `truncate`, `ungetc`, `ungetbyte` and `strioInit`. `readBuf` now compares the buffer with the current underlying string instead of the one passed in. Use `num2int`/`num2long` instead of `fix2int`/`fix2long` for `lineno=`, `pos=`, `read`, `pread` and `truncate`. The latter assume the argument is a `Fixnum` and raise `ClassCastException` for an object that responds to `to_int`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`seek` checked whether the stream is closed before converting `whence`. `whence.to_int` may reopen the receiver onto a closed stream, and then `seek` changed the position of that closed stream instead of raising `IOError`. Check the closed state after the conversion instead. This also matches `IO#seek`, which converts `whence` before checking the closed state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`each_line` checked readability only once before the loop. If the block closed the stream, or reopened it onto a stream that is not readable, `each_line` kept reading lines from it. Use `readable()` to fetch the stream for each line, as the JRuby extension and `IO` do. Also remove the `readable()` call at the beginning of `gets`, `each_line` and `readlines`. Its result is overwritten by `prepare_getline_args()`, which checks readability after converting the arguments. As a result, these methods convert the arguments before checking readability, and `each_line` without a block returns an enumerator even for a closed stream, as `IO` and the JRuby extension do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s on JRuby `write` checked writability only before `to_s`. If `to_s` reopened the receiver onto a read-only stream, `write` wrote to that stream. Check writability again after the conversion, as the C extension does. `each_byte` and `each_codepoint` fetched the data only once before the loop. If the block reopened the receiver, they kept reading the old data. Fetch the data for each byte or codepoint, as `strio_to_read()` in the C extension does, and don't hold the lock while yielding. As a side effect, `each_byte` and `each_codepoint` no longer raise `NullPointerException` for a null device stream, and they read data appended in the block, as the C extension does. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
`each_codepoint` fetched the encoding only once before the loop. If the block reopened the receiver onto a stream in another encoding, or changed the encoding by `set_encoding`, the rest of the data was read in the old encoding. Fetch the encoding for each codepoint, as `each_char` does. On JRuby, also pass the end of the string instead of the end of the backing array to `StringSupport.codePoint()`. It read bytes after the end of the string when the string shared its backing array and ended with a truncated character. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
`strio_init()` fetched the data by `StringIOForRead()` after the argument conversions. It doesn't check whether the receiver is frozen, so `reopen` modified the receiver even if a conversion froze it. Use `StringIO()` instead, as other methods and the JRuby extension do. `strio_init()` is also used by `initialize`. So calling `initialize` again on a frozen `StringIO` now raises `FrozenError` instead of modifying it, as the JRuby extension does. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
`rb_str_conv_enc()` and `rb_enc_find_index()` may load a transcoder or an encoding. Loading calls `to_path` of objects in `$LOAD_PATH`, which may reopen the stream. `write`, `ungetc` and `set_encoding_by_bom` kept using the pointer fetched before that. Fetch the pointer again after them. `set_encoding_by_bom()` detects the BOM again if the underlying string is changed. The fetched stream may be a null device stream, so check it: * `write` returns 0 for a null device stream. This also fixes a crash of `write` to a null device stream with an encoding, such as `StringIO.new(nil, external_encoding: "UTF-8")`, on both of the C extension and the JRuby extension. * `detect_bom()` accepts `nil`. This also fixes a crash of `set_encoding_by_bom` of a null device stream on both extensions. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Converting an argument (
to_int,to_str,to_s, ...) may call back into Ruby code, which may callStringIO#reopenon the same stream.reopen(other)replaces the underlyingstruct StringIOand frees the old one, so a pointer fetched before the conversion became stale.Fetch the pointer again after the conversions, and after yielding in
each_line. Instrio_init, letrb_io_extract_modeencwrite into a local instead ofptr->flags.Reported-by: Zabith (https://hackerone.com/nmzabith)