From: nobu@...
Date: 2019-09-08T07:01:47+00:00
Subject: [ruby-core:94838] [Ruby master Bug#16151] [PATCH] Fix a class of fstring related use-after-free

Issue #16151 has been updated by nobu (Nobuyoshi Nakada).


Probably, `STR_IS_SHARED_M` string must not get its buffer replaced.
And I don't think that memory leaks, resulted by setting `STR_NOFREE` regardless the original string, is acceptable.

Although I'm not sure why it is restricted to `klass == 0` strings, this is a rough patch.

```diff
diff --git a/string.c b/string.c
index 05ce0ed8d6..2a99d10f6f 100644
--- a/string.c
+++ b/string.c
@@ -155,8 +155,7 @@ VALUE rb_cSymbol;
     if (!FL_TEST(str, STR_FAKESTR)) { \
 	RB_OBJ_WRITE((str), &RSTRING(str)->as.heap.aux.shared, (shared_str)); \
 	FL_SET((str), STR_SHARED); \
-	if (RBASIC_CLASS((shared_str)) == 0) /* for CoW-friendliness */ \
-	    FL_SET_RAW((shared_str), STR_IS_SHARED_M); \
+        FL_SET_RAW((shared_str), STR_IS_SHARED_M); \
     } \
 } while (0)
 
@@ -327,6 +326,7 @@ rb_fstring(VALUE str)
     fstr = register_fstring(str);
 
     if (!bare) {
+        if (FL_TEST_RAW(str, STR_IS_SHARED_M)) return str;
 	str_replace_shared_without_enc(str, fstr);
 	OBJ_FREEZE_RAW(str);
 	return str;
@@ -1177,6 +1177,9 @@ str_replace_shared_without_enc(VALUE str2, VALUE str)
             /* TODO: check if str2 is a shared root */
             char *ptr2 = STR_HEAP_PTR(str2);
             if (ptr2 != ptr) {
+                if (FL_TEST_RAW(str2, STR_IS_SHARED_M)) {
+                    rb_fatal("about to replace a shared root");
+                }
                 ruby_sized_xfree(ptr2, STR_HEAP_SIZE(str2));
             }
         }
@@ -1287,8 +1290,7 @@ str_new_frozen(VALUE klass, VALUE orig)
 		RSTRING(str)->as.heap.len -= ofs + rest;
 	    }
 	    else {
-		if (RBASIC_CLASS(shared) == 0)
-		    FL_SET_RAW(shared, STR_IS_SHARED_M);
+                FL_SET_RAW(shared, STR_IS_SHARED_M);
 		return shared;
 	    }
 	}
@@ -1308,8 +1310,6 @@ str_new_frozen(VALUE klass, VALUE orig)
 	    RBASIC(str)->flags |= RBASIC(orig)->flags & STR_NOFREE;
 	    RBASIC(orig)->flags &= ~STR_NOFREE;
 	    STR_SET_SHARED(orig, str);
-	    if (klass == 0)
-		FL_UNSET_RAW(str, STR_IS_SHARED_M);
 	}
     }
 
```

P.S.

```ruby
  def test_shared_string_safety
    -('a' * 30).force_encoding(Encoding::ASCII)
    str = ('a' * 30).force_encoding(Encoding::ASCII).taint
    frozen_str = Bug::String.rb_str_new_frozen(str)
    assert_fstring(frozen_str) {|s| assert_equal(str, s)}
    GC.start  ###<-- I needed this line to reproduce the failure.
    assert_equal('a' * 30, str, "[Bug #16151]")
  end
```

----------------------------------------
Bug #16151: [PATCH] Fix a class of fstring related use-after-free
https://bugs.ruby-lang.org/issues/16151#change-81458

* Author: alanwu (Alan Wu)
* Status: Open
* Priority: Normal
* Assignee: 
* Target version: 
* ruby -v: ruby 2.7.0dev (2019-09-07T18:26:35Z master e9bc8b35c6) [x86_64-linux]
* Backport: 2.5: UNKNOWN, 2.6: UNKNOWN
----------------------------------------
Pull request: https://github.com/ruby/ruby/pull/2435

## The bug

Run the following against master(e9bc8b3) to observe use-after-free:

```ruby
-('a' * 30).force_encoding(Encoding::ASCII)
a = ('a' * 30).force_encoding(Encoding::ASCII).taint

t = Thread.new{}
t.name = a
eval('', binding, t.name)
p a
```

```ruby
-('a' * 30).force_encoding(Encoding::ASCII)
a = ('a' * 30).force_encoding(Encoding::ASCII).taint

require 'ripper'
ripper = Ripper.new("", a)
eval('', binding, ripper.filename)
p a
```

There may be other cases in the standard library or in the wild.

## Background

When a string has both `STR_NOEMBED` and `STR_SHARED` set, it relies on a
different string for its buffer. I will refer to strings that are depended upon
as "shared roots". Shared roots are frozen and have the `STR_SHARED` unset.
This is a bit unintuitive to me. A name for `STR_SHARED` that makes more sense
to me would be `STR_BUFFER_ELSEWHERE`.

## What went wrong

It is not safe to free the buffer of a shared root while it has dependants. The
root and its dependants use the same buffer. As such, it is only safe to free
the shared buffer when all users are unreachable on the heap.

## The Fix

`rb_fstring` has a code path that frees and replaces the buffer of its input.
Using this code path on the shared root of dependant strings sets up
use-after-free. This patch removes the problematic code path as no tests
require said buffer replacement functionality. Additionally, there has been
three other issues that steam from this particular code path. See #15926,
#15916 and #16136

---

I used @mame's commit in #16136 as the starting point for this investigation.
Thank you!



-- 
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:ruby-core-request@ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>