From: masaki.suketa@...
Date: 2015-01-08T15:58:05+00:00
Subject: [ruby-dev:48803] [ruby-trunk - Bug #10697] WIN32OLE: WIN32OLE_RECORD を使用したスクリプト終了時にruby.exe がクラッシュすることがある

Issue #10697 has been updated by Masaki Suketa.

Assignee changed from cruby-windows to Masaki Suketa

ありがとうございます。
現象自体は確認しており、メモリーの二重開放まではわかっていたのですが、
COMサーバ側で開放しているとは思いませんでした。
ちょっと時間が取れてなくてパッチの方は詳しく見ていないのですが、別のアプローチの
VT_BYREF|VT_RECORDで渡したら、こちらでも現象は起こらなくなったみたいです。
わざわざ、VT_VARIANT | VT_BYREF で渡しているのは参照渡しにしたいからなので、
参照渡しをするのなら、VT_RECORD|VT_BYREFでいいような気がしてきました。
IRecordInfo::RecordCreateで確保することは最初に実装するときに試してみたんですが
なんかうまくいかなくて、ALLOC_N()の確保に切り替えた記憶があります。
(記憶があやふやで正確なところが思いだせない。)
時間が取れるようになったら、もう少し調べてみます。





----------------------------------------
Bug #10697: WIN32OLE: WIN32OLE_RECORD を使用したスクリプト終了時にruby.exe がクラッシュすることがある
https://bugs.ruby-lang.org/issues/10697#change-50864

* Author: Takashi Sawanaka
* Status: Open
* Priority: Normal
* Assignee: Masaki Suketa
* Category: platform/windows
* Target version: current: 2.2.0
* ruby -v: ruby 2.3.0dev (2015-01-03 trunk 49122) [i386-mswin32_120]
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
以下のスクリプトまたは、Ruby のソース内のテスト `test/win32ole/test_win32ole_record.rb` を実行すると数回に一度程度の確率で ruby プロセスの終了時にSEGVが発生します。

```ruby
require 'win32ole'
obj = WIN32OLE.new('RbComTest.ComSrvTest')
book = WIN32OLE_RECORD.new('Book', obj)
obj.getBookByRefBook(book)
```
* ※1 上記スクリプトで使用している `RbComTest.ComSrvTest` は、 `test/win32ole/test_win32ole_record.rb` ファイル内に記述されている VB.NET COM server を ビルドして作成しています。 私は、https://github.com/sdottaka/mruby-win32ole/tree/master/test/RbComTest のように作成して VS2013 でビルドしました。
* ※2 実行環境は Windows 8.1 64bit版。 Visual Studio 2013 で ruby を32bitビルドしています。

異常発生時、VisualStudioで見たコールスタックは以下の通りです。

```
 	ntdll.dll!7764ebd8()	Unknown
 	[Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]	
 	ntdll.dll!775aa68c()	Unknown
 	oleaut32.dll!7706df93()	Unknown
 	msvcr120.dll!585aecfa()	Unknown
>	win32ole.so!olerecord_free(void * ptr) Line 220	C
 	msvcr120-ruby230.dll!finalize_list(rb_objspace * objspace, unsigned long zombie) Line 2476	C
 	msvcr120-ruby230.dll!rb_objspace_call_finalizer(rb_objspace * objspace) Line 2617	C
 	msvcr120-ruby230.dll!rb_gc_call_finalizer_at_exit() Line 2549	C
 	msvcr120-ruby230.dll!ruby_cleanup(volatile int ex) Line 233	C
 	msvcr120-ruby230.dll!ruby_run_node(void * n) Line 310	C
 	ruby.exe!main(int argc, char * * argv) Line 36	C
 	ruby.exe!__tmainCRTStartup() Line 626	C
 	kernel32.dll!7599919f()	Unknown
 	ntdll.dll!775c0bbb()	Unknown
 	ntdll.dll!775c0b91()	Unknown
```

調査してみたところ、二重free が起きているように思われました。

上記スクリプトの4行目の`obj.getBookByRefBook(book)`が呼ばれると
win32ole.c の `ole_invoke()` 関数内の `IDispatch::Invoke()` に引数として渡している
`VT_RECORD|VT_BYREF`の`VARIANT`型引数(`book`引数に相当)のメンバ `pvRecord` が示すメモリが 
COMサーバー側で解放＆再確保され、アドレスが書き換わってしまっています。(アドレスが変わらないこともあります)

この `pvRecord` の値は、`WIN32OLE_RECORD` 内で確保して保持しているメモリアドレス（`struct olerecorddata::pdata`)ですが、
上記 Invoke の呼び出しで解放と再確保されてアドレスが変わっているのに気付かず、 `WIN32OLE_RECORD` オブジェクト
の解放時に古いメモリアドレスでfreeしようとして2重free が起きるというように見えました。

余計なものだとは思いますが、ご参考までに修正しようとして挫折した途中のパッチを添付しています。
このパッチでは、

1. 上記のように `WIN32OLE_RECORD`が確保する `struct olerecorddata::pdata` のメモリがCOMサーバー側で解放されることがあるため、
  メモリアロケータをCOMサーバー側のデアロケータと同じ種類のものにしないとまずそうです。
  このため、`ole_rec2variant()`関数内で行っているメモリ確保を`ALLOC_N()`ではなく、`IRecordInfo::RecordCreate()`に変更しています。
2. `struct olerecorddata::pdata`のメモリは`WIN32OLE_RECORD`オブジェクトを`VARIANT`型に変換するときにのみ必要なのと、
  上記のように書き換えられてしまうため`WIN32OLE_RECORD`内で保持する必要がないと考え、
  `struct olerecorddata::pdata` は削除し、変換後のVARIANT型データをクリアするときに
  `IRecordInfo::RecordDestroy()`で解放するようにしています。
  (再確保後のメモリアドレスをpdata に再代入するという方法もあるかもしれません)
3.  2. で解放漏れが発生しないようするのと、COMサーバーが戻り値としてVT_RECORDタイプのVARIANTデータを返してきたとき、
    この`VARIANT::pvRecord` は WIN32OLE側で解放する責任がありそうなため、
    `VariantClear()` する箇所の大部分を VT_RECORD タイプのVARIANTデータ ならば
   `IRecordInfo::RecordDestroy()` で解放する新設関数 `ole_variant_clear()` に置き換えています。

なお、このパッチでは、`test/win32ole/test_win32ole_record.rb` のテストは通るのですが、以下のスクリプトを
実行するとメモリ使用量が増加し続けてしまいます。

```ruby
require 'win32ole' unless Module.const_defined?(:WIN32OLE)
def test1
	obj = WIN32OLE.new('RbComTest.ComSrvTest')
	rec = WIN32OLE_RECORD.new('Book', obj)
	rec.title = "Ruby Book"
	rec.cost = 60
	book = obj.getVer2BookByValBook(rec)
end

def test2
	obj = WIN32OLE.new('RbComTest.ComSrvTest')
	book = WIN32OLE_RECORD.new('Book', obj)
	obj.getBookByRefBook(book)
end

10000000.times do
	test1
	test2
	GC.start
end
```

また、良いかどうかわからないのですが、別のアプローチとして、`IDispatch::Invoke` に渡す`VARIANT`型のデータを
`VT_VARIANT|VT_BYREF` ではなく、 `VT_RECORD|VT_BYREF` にすると `pvRecord` が
再確保されない雰囲気があるので、これに頼って以下のように`VT_RECORD`の時だけ特別扱いする
方法もあるかもしれません。


```C
--- win32ole-53d9cb7-left.c	Mon Jan 05 23:13:20 2015
+++ win32ole.c	Mon Jan 05 23:14:32 2015
@@ -2665,8 +2665,13 @@
                 ole_variant2variant(param, &op.dp.rgvarg[n]);
             } else {
                 ole_val2variant(param, &realargs[n]);
-                V_VT(&op.dp.rgvarg[n]) = VT_VARIANT | VT_BYREF;
-                V_VARIANTREF(&op.dp.rgvarg[n]) = &realargs[n];
+                if (realargs[n].vt == VT_RECORD) {
+                    op.dp.rgvarg[n] = realargs[n];
+                    V_VT(&op.dp.rgvarg[n]) = VT_RECORD | VT_BYREF;
+                } else {
+                    V_VT(&op.dp.rgvarg[n]) = VT_VARIANT | VT_BYREF;
+                    V_VARIANTREF(&op.dp.rgvarg[n]) = &realargs[n];
+                }
             }
         }
     }
```
  


---Files--------------------------------
patch.txt (9.79 KB)


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