From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS4713 221.184.0.0/13 X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from neon.ruby-lang.org (neon.ruby-lang.org [221.186.184.75]) by dcvr.yhbt.net (Postfix) with ESMTP id 99D101F461 for ; Sun, 8 Sep 2019 23:03:15 +0000 (UTC) Received: from neon.ruby-lang.org (localhost [IPv6:::1]) by neon.ruby-lang.org (Postfix) with ESMTP id 1BA111209E2; Mon, 9 Sep 2019 08:03:00 +0900 (JST) Received: from o1678948x4.outbound-mail.sendgrid.net (o1678948x4.outbound-mail.sendgrid.net [167.89.48.4]) by neon.ruby-lang.org (Postfix) with ESMTPS id ACBED1209E2 for ; Mon, 9 Sep 2019 08:02:57 +0900 (JST) Received: by filter0047p3iad2.sendgrid.net with SMTP id filter0047p3iad2-26120-5D75889F-9 2019-09-08 23:02:55.190379915 +0000 UTC m=+455321.280911375 Received: from herokuapp.com (unknown [3.86.63.147]) by ismtpd0001p1iad1.sendgrid.net (SG) with ESMTP id b7TYQ_SrTXubuJkVPSebwg for ; Sun, 08 Sep 2019 23:02:55.119 +0000 (UTC) Date: Sun, 08 Sep 2019 23:02:55 +0000 (UTC) From: XrXr@users.noreply.github.com Message-ID: References: Mime-Version: 1.0 X-Redmine-MailingListIntegration-Message-Ids: 70403 X-Redmine-Project: ruby-trunk X-Redmine-Issue-Id: 16151 X-Redmine-Issue-Author: alanwu X-Redmine-Sender: alanwu X-Mailer: Redmine X-Redmine-Host: bugs.ruby-lang.org X-Redmine-Site: Ruby Issue Tracking System X-Auto-Response-Suppress: All Auto-Submitted: auto-generated X-SG-EID: =?us-ascii?Q?PWg67P6owy8ojUUZg1G=2FQM4Z0jTQ2XLCqLM8Y2L8tUvRFuUPM6+aKANjRFY5CJ?= =?us-ascii?Q?rIEWsF1bK1PpHOZcO8agKq+BdRSbQPLZB7I9y1e?= =?us-ascii?Q?1KA2EADZPEAuKf719o+DnYinkQQWmU37hver4Gu?= =?us-ascii?Q?enp+ryt3wCsjstDluHTkPTQJr7AjzU9Eio3CD6p?= =?us-ascii?Q?CqYy7PKGAY5SnbDU0VEUx9gS98e0G5p+ilg=3D=3D?= To: ruby-core@ruby-lang.org X-ML-Name: ruby-core X-Mail-Count: 94848 Subject: [ruby-core:94848] [Ruby master Bug#16151] [PATCH] Fix a class of fstring related use-after-free X-BeenThere: ruby-core@ruby-lang.org X-Mailman-Version: 2.1.15 Precedence: list Reply-To: Ruby developers List-Id: Ruby developers List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ruby-core-bounces@ruby-lang.org Sender: "ruby-core" Issue #16151 has been updated by alanwu (Alan Wu). That general approach sounds good to me, but please let me mention some caveats. ```diff @@ -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); } } ``` These two lines are important for #13085, where `STR_IS_SHARED_M` was first introduced. The following program generates a lot more garbage when I comment out these two lines: ```ruby # cat.rb buf = ''.b File.open(ARGV.first) do |f| while f.read(16384, buf) $stdout.write(buf) end end $stderr.puts(File.read("/proc/#{Process.pid}/status")) ``` Going with this approach while keeping the optimization introduced in #13085 might require a new flag. Sidenote, marking roots with a flag is a heuristic, since a string stops being a root once all the dependants are unreachable. We already have a more coarse heuristic: if a string is frozen, we know that it could be a shared root. Maybe that can be a viable alternative approach. ```diff diff --git a/string.c b/string.c index 05ce0ed8d6..86db4891fb 100644 --- a/string.c +++ b/string.c @@ -327,6 +327,8 @@ rb_fstring(VALUE str) fstr = register_fstring(str); if (!bare) { + /* A frozen string might be a shared root */ + if (FL_TEST_RAW(str, FL_FREEZE)) return str; str_replace_shared_without_enc(str, fstr); OBJ_FREEZE_RAW(str); return str; ``` Though, I think if we can get away with removing the buffer replacement code path, we should. ---------------------------------------- Bug #16151: [PATCH] Fix a class of fstring related use-after-free https://bugs.ruby-lang.org/issues/16151#change-81471 * 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/