Bug #1137: I can modify literals

• Status: Closed
• Assignee: -
• Target version: -

[mame (Yusuke Endoh)]

=begin
遠藤です。

ObjectSpace を使うと種々のリテラルを書き換えることができてしまうようですが、
仕様でしょうか。

def foo
"foobarbaz"
end

ObjectSpace.each_object(String) do |s|
s.replace("evil") if /foobarbaz/ =~ s && !s.frozen?
end

p foo #=> "evil"

def bar
ls -l
end

ObjectSpace.each_object(String) do |s|
s.replace("echo rm -rf /") if /ls -l/ =~ s && !s.frozen?
end

p bar #=> "rm -rf /\n"

バグだとして、リテラルを freeze するパッチを書きましたが、IRC では

• freeze で解決するのが正しいやり方なのか
• freeze しても finalizer が付け替えできるのではないか

という感じの指摘がありました。どう直すのがよいでしょう。

Index: compile.c

--- compile.c (revision 22217)
+++ compile.c (working copy)
@@ -2120,7 +2120,7 @@
int cnt = 1;

  debugp_param("nd_lit", lit);

• ADD_INSN1(ret, nd_line(node), putobject, node->nd_lit);

• ADD_INSN1(ret, nd_line(node), putobject, rb_obj_freeze(node->nd_lit));

  while (list) {
  COMPILE(ret, "each string", list->nd_head);
  @@ -2236,6 +2236,7 @@
  rb_ary_push(ary, node->nd_head->nd_lit);
  node = node->nd_next;
  }

•  rb_obj_freeze(ary);
  
   iseq_add_mark_object_compile_time(iseq, ary);
   ADD_INSN1(ret, nd_line(node_root), duparray, ary);
  

@@ -2708,7 +2709,7 @@

  if (estr != 0) {
if (needstr != Qfalse) {

•  VALUE str = rb_str_new2(estr);
  

•  VALUE str = rb_obj_freeze(rb_str_new2(estr));
   ADD_INSN1(ret, nd_line(node), putstring, str);
   iseq_add_mark_object_compile_time(iseq, str);
  

  }
  @@ -4353,7 +4354,7 @@
  case NODE_STR:{
  debugp_param("nd_lit", node->nd_lit);
  if (!poped) {

•  ADD_INSN1(ret, nd_line(node), putstring, node->nd_lit);
  

•  ADD_INSN1(ret, nd_line(node), putstring, rb_obj_freeze(node->nd_lit));
  

  }
  break;
  }
  @@ -4367,7 +4368,7 @@
  }
  case NODE_XSTR:{
  ADD_CALL_RECEIVER(ret, nd_line(node));

• ADD_INSN1(ret, nd_line(node), putobject, node->nd_lit);

• ADD_INSN1(ret, nd_line(node), putobject, rb_obj_freeze(node->nd_lit));
  ADD_CALL(ret, nd_line(node), ID2SYM(idBackquote), INT2FIX(1));

  if (poped) {

--
Yusuke ENDOH [e-mail]
=end

[ko1 (Koichi Sasada)]

=begin
　ささだです．

Yusuke ENDOH wrote::

ObjectSpace を使うと種々のリテラルを書き換えることができてしまうようですが、
仕様でしょうか。

　他にも，Regexp などの Immutable と言われてるオブジェクトに勝手に特異メ
ソッドなりを追加できちゃう，とかもあります．Regexp なんかは，リテラルで
毎回同じオブジェクトを返すので，仕様的にはどう解決したものだか（これはそ
のままとするのが仕様かもしれない）．影響としてぱっと思いつくのは，恐くな
いんですが Marshal をさせないようにするような嫌がらせです．

def xyzzy
p Marshal.dump(/xyzzy/)
end

ObjectSpace.each_object(Regexp){|e|
if /xyzzy/ =~ e.to_s
unless e.frozen?
begin
def e.foo
end
rescue TypeError
p e
end
e.instance_eval{
@A (A A) = 1
}
end
end
}

xyzzy #=> singleton can't be dumped (TypeError)

　なお，いただいたパッチについては，以下のようなほうが確実かなぁ，なんて
思いました．

Index: compile.c

--- compile.c (リビジョン 22118)
+++ compile.c (作業コピー)
@@ -398,6 +398,13 @@
iseq_add_mark_object(rb_iseq_t *iseq, VALUE v)
{
if (!SPECIAL_CONST_P(v)) {

• switch (BUILTIN_TYPE(v)) {
• case T_STRING:
• case T_ARRAY:

•  dp(v);
  

•  rb_obj_freeze(v);
  

• }
• rb_ary_push(iseq->mark_ary, v);
  }
  return COMPILE_OK;

--
// SASADA Koichi at atdot dot net

=end

[ko1 (Koichi Sasada)]

=begin
　ささだです．

SASADA Koichi wrote::

Index: compile.c

--- compile.c (リビジョン 22118)
+++ compile.c (作業コピー)
@@ -398,6 +398,13 @@
iseq_add_mark_object(rb_iseq_t *iseq, VALUE v)
{
if (!SPECIAL_CONST_P(v)) {

• switch (BUILTIN_TYPE(v)) {

•  case T_STRING:
  

•  case T_ARRAY:
  

•    dp(v);
  

　この行，消し忘れました．

•    rb_obj_freeze(v);
  

• }
• rb_ary_push(iseq->mark_ary, v);
  }
  return COMPILE_OK;

--
// SASADA Koichi at atdot dot net

=end

[usa (Usaku NAKAMURA)]

=begin
こんにちは、なかむら(う)です。

In message "[ruby-dev:37959] [Bug:trunk] I can modify literals"
on Feb.11,2009 02:16:05, [e-mail] wrote:

バグだとして、リテラルを freeze するパッチを書きましたが、IRC では

• freeze で解決するのが正しいやり方なのか
• freeze しても finalizer が付け替えできるのではないか

という感じの指摘がありました。どう直すのがよいでしょう。

元の問題も気になりますが、frozenなオブジェクトに対してfinalizer
を付け替えできてしまうことが気になります。
実際にはObjectSpaceの操作であることや、finalizerからは元のオ
ブジェクトそのものの操作ができるわけではないことなどが理由な
のではないかとは思いますが、付け替えの操作自体はオブジェクト
の内部フラグを変更しているので(あれ、取り除くとき変更してない
な)、禁止してもいいのではないかという気もします。

Index: gc.c

--- gc.c (revision 22185)
+++ gc.c (working copy)
@@ -2261,9 +2261,11 @@ static VALUE
undefine_final(VALUE os, VALUE obj)
{
rb_objspace_t *objspace = &rb_objspace;

• if (OBJ_FROZEN(obj)) rb_error_frozen("object");
  if (finalizer_table) {
  st_delete(finalizer_table, (st_data_t*)&obj, 0);
  }
• FL_UNSET(obj, FL_FINALIZE);
  return obj;
  }

@@ -2283,6 +2285,7 @@ define_final(int argc, VALUE *argv, VALU
VALUE obj, block, table;

  rb_scan_args(argc, argv, "11", &obj, &block);

• if (OBJ_FROZEN(obj)) rb_error_frozen("object");
  if (argc == 1) {
  block = rb_block_proc();
  }
  
  それでは。
  --
  U.Nakamura [e-mail]

=end

[matz (Yukihiro Matsumoto)]

=begin
まつもと ゆきひろです

In message "Re: [ruby-dev:37964] Re: [Bug:trunk] I can modify literals"
on Wed, 11 Feb 2009 16:11:06 +0900, "U.Nakamura" [e-mail] writes:

|元の問題も気になりますが、frozenなオブジェクトに対してfinalizer
|を付け替えできてしまうことが気になります。
|実際にはObjectSpaceの操作であることや、finalizerからは元のオ
|ブジェクトそのものの操作ができるわけではないことなどが理由な
|のではないかとは思いますが、付け替えの操作自体はオブジェクト
|の内部フラグを変更しているので(あれ、取り除くとき変更してない
|な)、禁止してもいいのではないかという気もします。

そうでしょうね。コミットしてください。元の問題の方は...うー
ん、どうしよう。

=end

[matz (Yukihiro Matsumoto)]

=begin
まつもと ゆきひろです

In message "Re: [ruby-dev:37959] [Bug:trunk] I can modify literals"
on Wed, 11 Feb 2009 02:16:05 +0900, Yusuke ENDOH [e-mail] writes:

|ObjectSpace を使うと種々のリテラルを書き換えることができてしまうようですが、
|仕様でしょうか。

うーん、ObjectSpaceってのはかなり裏技なんで、それを使って
「まずいこと」ができた場合、それを仕様と呼ぶのもバグと呼ぶの
も抵抗があります。

|バグだとして、リテラルを freeze するパッチを書きましたが、IRC では
|
| - freeze で解決するのが正しいやり方なのか
| - freeze しても finalizer が付け替えできるのではないか
|
|という感じの指摘がありました。どう直すのがよいでしょう。

後者はうささんに直してもらいました。前者については

• ObjectSpaceは裏技。やる方が悪い(からバグじゃない)
• 禁止できるものなら禁止した方がよい(からバグ)

の両方の考え方ができると思います。

....

いろいろ考えましたが、過去にもObjectSpaceの「まずいこと」に
手当てしてきた経緯がありますし、バグとみなすことにします。で、
その場合の修正方法ですが、freezeで構わないと思います。

パッチをコミットしてください。

=end

[nobu (Nobuyoshi Nakada)]

=begin
なかだです。

At Thu, 12 Feb 2009 10:34:51 +0900,
Yukihiro Matsumoto wrote in [ruby-dev:37969]:

• ObjectSpaceは裏技。やる方が悪い(からバグじゃない)
• 禁止できるものなら禁止した方がよい(からバグ)

の両方の考え方ができると思います。

....

いろいろ考えましたが、過去にもObjectSpaceの「まずいこと」に
手当てしてきた経緯がありますし、バグとみなすことにします。で、
その場合の修正方法ですが、freezeで構わないと思います。

freeze以前に、ObjectSpaceから不可視であるべきではないかと思いま
す。また、隠しオブジェクトから通常のStringを作るために
rb_str_replace()を使っていますが、これはintern.hで公開してもかま
わないんではないでしょうか。

Index: compile.c

--- compile.c (revision 22249)
+++ compile.c (working copy)
@@ -296,4 +296,5 @@ PRINTF_ARGS(void ruby_debug_printf(const
(name##body_.last = &name##body_.anchor, name = &name##body_)

+#define hide_obj(obj) (void)(RBASIC(obj)->klass = 0)

#include "optinsn.inc"
@@ -2231,5 +2232,5 @@ compile_array_(rb_iseq_t *iseq, LINK_ANC
if (opt_p == Qtrue) {
if (!poped) {

•  VALUE ary = rb_ary_new();
  

•  VALUE ary = rb_ary_tmp_new(len);
   node = node_root;
   while (node) {
  

@@ -2710,4 +2711,5 @@ defined_expr(rb_iseq_t *iseq, LINK_ANCHO
if (needstr != Qfalse) {
VALUE str = rb_str_new2(estr);

•  hide_obj(str);
   ADD_INSN1(ret, nd_line(node), putstring, str);
   iseq_add_mark_object_compile_time(iseq, str);
  

@@ -4354,4 +4356,5 @@ iseq_compile_each(rb_iseq_t *iseq, LINK_
debugp_param("nd_lit", node->nd_lit);
if (!poped) {

•  hide_obj(node->nd_lit);
   ADD_INSN1(ret, nd_line(node), putstring, node->nd_lit);
  

  }
  Index: string.c
  ===================================================================
  --- string.c (revision 22249)
  +++ string.c (working copy)
  @@ -816,6 +816,4 @@ rb_obj_as_string(VALUE obj)
  }

-static VALUE rb_str_replace(VALUE, VALUE);

VALUE
rb_str_dup(VALUE str)
@@ -3723,5 +3721,5 @@ rb_str_gsub(int argc, VALUE *argv, VALUE
*/

-static VALUE
+VALUE
rb_str_replace(VALUE str, VALUE str2)
{
Index: insns.def

--- insns.def (revision 22249)
+++ insns.def (working copy)
@@ -374,5 +374,6 @@ putstring
(VALUE val)
{

• val = rb_str_new3(str);

• VALUE rb_str_replace(VALUE, VALUE);
• val = rb_str_replace(rb_str_new(0, 0), str);
  }

@@ -461,5 +462,5 @@ duparray
(VALUE val)
{

• val = rb_ary_dup(ary);

• val = rb_ary_replace(rb_ary_new2(0), ary);
  }

Index: include/ruby/intern.h

--- include/ruby/intern.h (revision 22249)
+++ include/ruby/intern.h (working copy)
@@ -618,4 +618,5 @@ VALUE rb_str_equal(VALUE str1, VALUE str
VALUE rb_str_drop_bytes(VALUE, long);
void rb_str_update(VALUE, long, long, VALUE);
+VALUE rb_str_replace(VALUE, VALUE);
VALUE rb_str_inspect(VALUE);
VALUE rb_str_dump(VALUE);

--
--- 僕の前にBugはない。
--- 僕の後ろにBugはできる。
中田 伸悦

=end

[matz (Yukihiro Matsumoto)]

=begin
まつもと ゆきひろです

In message "Re: [ruby-dev:37971] Re: [Bug:trunk] I can modify literals"
on Thu, 12 Feb 2009 11:35:16 +0900, Nobuyoshi Nakada [e-mail] writes:

|freeze以前に、ObjectSpaceから不可視であるべきではないかと思いま
|す。また、隠しオブジェクトから通常のStringを作るために
|rb_str_replace()を使っていますが、これはintern.hで公開してもかま
|わないんではないでしょうか。

うーん、そうかあ。んじゃあ、とりあえず不可視にしておいてくだ
さい。それはそれとしてfreezeしても良いような気もしますが。

=end

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed
• % Done changed from 0 to 100

=begin
Applied in changeset r22255.
=end

[ko1 (Koichi Sasada)]

=begin
　ささだです．

Yukihiro Matsumoto wrote::

|freeze以前に、ObjectSpaceから不可視であるべきではないかと思いま
|す。また、隠しオブジェクトから通常のStringを作るために
|rb_str_replace()を使っていますが、これはintern.hで公開してもかま
|わないんではないでしょうか。

うーん、そうかあ。んじゃあ、とりあえず不可視にしておいてくだ
さい。それはそれとしてfreezeしても良いような気もしますが。

　さらに freeze する理由はなんでしょうか．しない理由もないと思いますが，
する理由もないような．

--
// SASADA Koichi at atdot dot net

=end

[ko1 (Koichi Sasada)]

=begin
　ささだです．

　rb_replace を使うんじゃなくて，新しい API を作るのはどうでしょう．
rb_from() という名前は，後で後悔しそうな気はします．もっと泥臭い名前の
方がいいかも．

Index: array.c

--- array.c (リビジョン 22272)
+++ array.c (作業コピー)
@@ -1468,6 +1468,12 @@ rb_ary_dup(VALUE ary)
return dup;
}

+VALUE
+rb_ary_new_from(VALUE ary)
+{

• return rb_ary_new4(RARRAY_LEN(ary), RARRAY_PTR(ary));
  +}

extern VALUE rb_output_fs;

static VALUE
Index: insns.def

--- insns.def (リビジョン 22272)
+++ insns.def (作業コピー)
@@ -373,7 +373,7 @@ putstring
()
(VALUE val)
{

• val = rb_str_replace(rb_str_new(0, 0), str);

• val = rb_str_new_from(str);
  }

/**
@@ -460,7 +460,7 @@ duparray
()
(VALUE val)
{

• val = rb_ary_replace(rb_ary_new2(0), ary);

• val = rb_ary_new_from(ary);
  }

/**
Index: vm_core.h

--- vm_core.h (リビジョン 22272)
+++ vm_core.h (作業コピー)
@@ -598,6 +598,9 @@ NOINLINE(void rb_gc_save_machine_context

#define sysstack_error GET_VM()->special_exceptions[ruby_error_sysstack]

+VALUE rb_str_new_from(VALUE str);
+VALUE rb_ary_new_from(VALUE ary);
+
/* for thread */

#if RUBY_VM_THREAD_MODEL == 2
Index: iseq.c

--- iseq.c (リビジョン 22272)
+++ iseq.c (作業コピー)
@@ -704,7 +704,7 @@ insn_operand_intern(rb_iseq_t *iseq,
if (hidden_obj_p(op)) {
switch (BUILTIN_TYPE(op)) {
case T_STRING:

•  op = rb_str_replace(rb_str_new(0, 0), op);
  

•  op = rb_str_new_from(op);
   break;
     case T_ARRAY:
   op = rb_ary_replace(rb_ary_new2(0), op);
  

Index: string.c

--- string.c (リビジョン 22272)
+++ string.c (作業コピー)
@@ -829,6 +829,11 @@ rb_str_dup(VALUE str)
return str_duplicate(rb_obj_class(str), str);
}

+VALUE
+rb_str_new_from(VALUE str)
+{

• return rb_str_replace(str_alloc(rb_cString), str);
  +}

/*

• call-seq:

--
// SASADA Koichi at atdot dot net

=end

[ko1 (Koichi Sasada)]

=begin
SASADA Koichi wrote::

Yukihiro Matsumoto wrote::

|freeze以前に、ObjectSpaceから不可視であるべきではないかと思いま
|す。また、隠しオブジェクトから通常のStringを作るために
|rb_str_replace()を使っていますが、これはintern.hで公開してもかま
|わないんではないでしょうか。

うーん、そうかあ。んじゃあ、とりあえず不可視にしておいてくだ
さい。それはそれとしてfreezeしても良いような気もしますが。

　さらに freeze する理由はなんでしょうか．しない理由もないと思いますが，
する理由もないような．

　あと，逆に freeze すれば不可視にする必要もないかなーとも思いますが，ど
んなもんですかね（不可視にするべきオブジェクトもあったけれども）．実行コ
スト的には freeze のほうが軽そうですが．

--
// SASADA Koichi at atdot dot net

=end

[matz (Yukihiro Matsumoto)]

=begin
まつもと ゆきひろです

In message "Re: [ruby-dev:37985] Re: [Bug:trunk] I can modify literals"
on Fri, 13 Feb 2009 06:36:47 +0900, SASADA Koichi [e-mail] writes:

|> 　さらに freeze する理由はなんでしょうか．しない理由もないと思いますが，
|> する理由もないような．
|
|　あと，逆に freeze すれば不可視にする必要もないかなーとも思いますが，ど
|んなもんですかね（不可視にするべきオブジェクトもあったけれども）．実行コ
|スト的には freeze のほうが軽そうですが．

まず不可視にする方が「かっこいい」のでそっちの方を選びたいと
ころです。最初から思いつくべきだったかもしれません。freezeす
るのは、まあ間違いを早めに見つけられたらいいんじゃない、程度
の理由です。

=end