Forum: Ruby-core [ruby-trunk - Bug #8728][Open] strio_substr can put invalid pointer into substring

F1d37642fdaa1662ff46e4c65731e9ab?d=identicon&s=25 Charles Nutter (headius)
on 2013-08-03 23:31
(Received via mailing list)
Issue #8728 has been reported by headius (Charles Nutter).

----------------------------------------
Bug #8728: strio_substr can put invalid pointer into substring
https://bugs.ruby-lang.org/issues/8728

Author: headius (Charles Nutter)
Status: Open
Priority: Low
Assignee:
Category: ext
Target version: next minor
ruby -v: 2.1.0-dev
Backport: 1.9.3: UNKNOWN, 2.0.0: UNKNOWN


There's a bug in strio_substr when reading a zero-length string from a
StringIO when the position is past the end of the internal string.

static VALUE
strio_substr(struct StringIO *ptr, long pos, long len)
{
    VALUE str = ptr->string;
    rb_encoding *enc = rb_enc_get(str);
    long rlen = RSTRING_LEN(str) - pos;

    if (len > rlen) len = rlen;
    if (len < 0) len = 0;
    return rb_enc_str_new(RSTRING_PTR(str)+pos, len, enc);
}

Logic in strio_read passes ptr->pos directly to this function:

...
    if (NIL_P(str)) {
  str = strio_substr(ptr, ptr->pos, len);
  if (binary) rb_enc_associate(str, rb_ascii8bit_encoding());
    }
...

Logic above will check if ptr->pos is >= the string length, but *only*
if the requested length is greater than zero:

...
      case 1:
  if (!NIL_P(argv[0])) {
      len = NUM2LONG(argv[0]);
      if (len < 0) {
    rb_raise(rb_eArgError, "negative length %ld given", len);
      }
      if (len > 0 && ptr->pos >= RSTRING_LEN(ptr->string)) {
    if (!NIL_P(str)) rb_str_resize(str, 0);
    return Qnil;
      }
      binary = 1;
      break;
  }
...

So the following code, which seeks way beyond the end of the StringIO
string, ends up calling strio_substr with the position as-is, resulting
in a RubyString pointing off into invalid memory:

sio = StringIO.new("")
sio.seek(10000000000)
str = sio.read(0)

It doesn't manifest as a crash, as far as I can tell, because the
resulting string is of length zero. There's shortcuts in the string
logic to never dereference the string's pointer if the length is zero.

However...the pointer is completely invalid. It seems like bad form to
reference invalid memory, even if you're pretty sure you won't
dereference it.

A simple fix would be to have strio_substr use some other ptr offset
when pos does not point at valid memory, but I'm not sure what offset
that should be. A better fix would probably be to have it always return
a new empty string when len == 0.

I have ported much of this logic into JRuby and today could not figure
out how pos gets validated before creating this string. As it turns out,
it does not...so I will be doing the empty string fix in JRuby.
F1d37642fdaa1662ff46e4c65731e9ab?d=identicon&s=25 Charles Nutter (headius)
on 2013-08-03 23:44
(Received via mailing list)
Issue #8728 has been updated by headius (Charles Nutter).


I will go ahead and fix this with an empty string.
----------------------------------------
Bug #8728: strio_substr can put invalid pointer into substring
https://bugs.ruby-lang.org/issues/8728#change-40871

Author: headius (Charles Nutter)
Status: Open
Priority: Low
Assignee:
Category: ext
Target version: next minor
ruby -v: 2.1.0-dev
Backport: 1.9.3: UNKNOWN, 2.0.0: UNKNOWN


There's a bug in strio_substr when reading a zero-length string from a
StringIO when the position is past the end of the internal string.

static VALUE
strio_substr(struct StringIO *ptr, long pos, long len)
{
    VALUE str = ptr->string;
    rb_encoding *enc = rb_enc_get(str);
    long rlen = RSTRING_LEN(str) - pos;

    if (len > rlen) len = rlen;
    if (len < 0) len = 0;
    return rb_enc_str_new(RSTRING_PTR(str)+pos, len, enc);
}

Logic in strio_read passes ptr->pos directly to this function:

...
    if (NIL_P(str)) {
  str = strio_substr(ptr, ptr->pos, len);
  if (binary) rb_enc_associate(str, rb_ascii8bit_encoding());
    }
...

Logic above will check if ptr->pos is >= the string length, but *only*
if the requested length is greater than zero:

...
      case 1:
  if (!NIL_P(argv[0])) {
      len = NUM2LONG(argv[0]);
      if (len < 0) {
    rb_raise(rb_eArgError, "negative length %ld given", len);
      }
      if (len > 0 && ptr->pos >= RSTRING_LEN(ptr->string)) {
    if (!NIL_P(str)) rb_str_resize(str, 0);
    return Qnil;
      }
      binary = 1;
      break;
  }
...

So the following code, which seeks way beyond the end of the StringIO
string, ends up calling strio_substr with the position as-is, resulting
in a RubyString pointing off into invalid memory:

sio = StringIO.new("")
sio.seek(10000000000)
str = sio.read(0)

It doesn't manifest as a crash, as far as I can tell, because the
resulting string is of length zero. There's shortcuts in the string
logic to never dereference the string's pointer if the length is zero.

However...the pointer is completely invalid. It seems like bad form to
reference invalid memory, even if you're pretty sure you won't
dereference it.

A simple fix would be to have strio_substr use some other ptr offset
when pos does not point at valid memory, but I'm not sure what offset
that should be. A better fix would probably be to have it always return
a new empty string when len == 0.

I have ported much of this logic into JRuby and today could not figure
out how pos gets validated before creating this string. As it turns out,
it does not...so I will be doing the empty string fix in JRuby.
This topic is locked and can not be replied to.