Project

General

Profile

Actions

Bug #11386

closed

taint flag about rb_fstring()

Added by usa (Usaku NAKAMURA) over 9 years ago. Updated over 9 years ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-dev:49188]

Description

r51261以降、mswinのtest-allでfailureが出るようになった件を調査していて発見したのですが、
rb_fstring()にはtaintフラグを保存しないという問題があります。

原因は2つあって、

  1. sharedなStringオブジェクトを登録する際にtaintフラグをコピーしないバグがある
  2. 既に同じバイト列・エンコーディングで表現可能なStringオブジェクトが登録されている場合それを返すが、
    引数のtaintフラグと返すオブジェクトのtaintフラグの違いを評価していない

というものです。

前者は以下のパッチで直るのでどうでもいいんですが、後者はrb_fstring()の仕様がどうなのか、
という問題であると思います。
r51261より前のように、SymbolやRubyレベルでは見えないStringオブジェクトのみを扱っているのならば特に問題ではないのでr51261のような使い方を禁止するべきなのか、
rb_fstring()を変更してtaintフラグを適切に扱うようにすべきなのか、どちらでしょうか?

Index: string.c
===================================================================
--- string.c	(リビジョン 51334)
+++ string.c	(作業コピー)
@@ -245,8 +245,10 @@ fstr_update_callback(st_data_t *key, st_data_t *va
     }
     else {
 	if (STR_SHARED_P(str)) { /* str should not be shared */
-	    str = rb_enc_str_new(RSTRING_PTR(str), RSTRING_LEN(str), STR_ENC_GET(str));
-	    OBJ_FREEZE(str);
+	    VALUE newstr = rb_enc_str_new(RSTRING_PTR(str), RSTRING_LEN(str), STR_ENC_GET(str));
+	    OBJ_INFECT(newstr, str);
+	    OBJ_FREEZE(newstr);
+	    str = newstr;
 	}
 	else {
 	    str = rb_str_new_frozen(str);


Files

0001-string.c-keep-taintedness.patch (2.08 KB) 0001-string.c-keep-taintedness.patch nobu (Nobuyoshi Nakada), 07/22/2015 11:23 PM
0002-tests-for-fstring-taintedness.patch (2.61 KB) 0002-tests-for-fstring-taintedness.patch nobu (Nobuyoshi Nakada), 07/22/2015 11:23 PM
0003-string.c-bare-string-fstring.patch (2.26 KB) 0003-string.c-bare-string-fstring.patch nobu (Nobuyoshi Nakada), 07/23/2015 01:42 AM

Related issues 1 (0 open1 closed)

Related to Ruby master - Bug #12923: Accessing singleton_class of fstring cause assertion failureClosedmatz (Yukihiro Matsumoto)Actions

Updated by usa (Usaku NAKAMURA) over 9 years ago

  • Backport changed from 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: REQUIRED to 2.0.0: DONTNEED, 2.1: REQUIRED, 2.2: REQUIRED

Updated by usa (Usaku NAKAMURA) over 9 years ago

なるほど、とは思ったのですが、
「次にぼくは『特異メソッドを定義したStringオブジェクトをrb_fstring()に渡すと問題がある』と言うッ!」
という感じです。(そして次はインスタンス変数)

Updated by usa (Usaku NAKAMURA) over 9 years ago

むむ、早い。

で、このパッチだと、Stringのサブクラスのインスタンスだったり特異メソッドがあったりインスタンス変数を持ってたりするとfstringにならなくなってるわけですが、そもそもtaintフラグが立ってるものもfstringにならなくてもいいんじゃない? 的なことも思ったりします。

というわけで、やっぱり、「fstringって本質的にはなんなの?」という仕様の問題に立ち返る気がするのですが、これを説明できるのは誰でしょう?
最初は笹田さんかと思ってたんですが、実は元々これを持ってきたのはちゃりさむ?

Updated by nobu (Nobuyoshi Nakada) over 9 years ago

昨日笹田さんとも話した結果、「内容だけをfstringに保存する」という方針がいいのではないか、ということになりました。

https://github.com/nobu/ruby/tree/bug/fstring-extra

Updated by usa (Usaku NAKAMURA) over 9 years ago

やっぱそうなりますよねー。
ぼくもそれがいいと思います。

Actions #8

Updated by nobu (Nobuyoshi Nakada) over 9 years ago

  • Status changed from Open to Closed

Applied in changeset r51360.


string.c: pool only bare strings in fstring

  • string.c (fstr_update_callback): pool bare strings only.
  • string.c (rb_fstring): return the original string with sharing a
    fstring if it has extra attributes, not the fstring itself.
    [ruby-dev:49188] [Bug #11386]
Actions #9

Updated by naruse (Yui NARUSE) almost 8 years ago

  • Related to Bug #12923: Accessing singleton_class of fstring cause assertion failure added
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0