mruby-passwd のサンプルからわかるmgem（あと、C言語）を書くときの罠

mruby advent calendar 8日目としてやっていかせていただきます。7日目はけいぞうくんのすごい記事でした。

keizobookman.hatenablog.com

さて先日、mrubyのgemをコーディングするスクリーンキャストを撮り、実際にコードを残しました。

udzura.hatenablog.jp

ですが、mrubyのお作法的な点、後そもそもの題材である getpwnam_r(3) とpasswd構造体の扱いについて不適切なところがあった点の2ヶ所で、補足しないといけないところが見つかりましたので、今回記事を書かせていただこうと思います。

まずは、先日の時点での最終形としたコードを眺めてみましょう。

先日のコード

なお、本記事の環境としてはLinux、x86_64 の Ubuntu Xenial、gcc 5.4.0-6ubuntu1~16.04.4 を利用しています。mrubyはこの日の時点のmasterとします。

このままではインスタンスのバックデータがGCされない問題

まず、このmgemから作ったmrubyバイナリを、以下のように valgrind にかけてみましょう。なんのことはない、1000回インスタンスを生成するだけのコードです。

$ valgrind --leak-check=full ./mruby/bin/mruby -e "1000.times {pwd = Passwd.new('vagrant'); pwd.uid}"

しかし、このプロセスのRSS(Resident set size)を観察すると、 もりもりと 肥大化していくのがわかります。

そして、以下のような definitely lost の警告が出ます。明らかにGCされていないようですね...

==13517== HEAP SUMMARY:
==13517==     in use at exit: 16,000 bytes in 1,000 blocks
==13517==   total heap usage: 5,544 allocs, 6,544 frees, 1,072,671 bytes allocated
==13517== 
==13517== 16,000 bytes in 1,000 blocks are definitely lost in loss record 1 of 1
==13517==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13517==    by 0x46121C: mrb_passwd_init (mrb_passwd.c:49)
==13517==    by 0x4335BC: mrb_funcall_with_block (vm.c:398)
==13517==    by 0x408A9F: mrb_instance_new (class.c:1347)
==13517==    by 0x430F74: mrb_context_run (vm.c:1126)
==13517==    by 0x435DCB: mrb_toplevel_run_keep (vm.c:2411)
==13517==    by 0x4362E3: load_exec (parse.y:5631)
==13517==    by 0x44577A: mrb_load_nstring_cxt (parse.y:5653)
==13517==    by 0x44577A: mrb_load_string_cxt (parse.y:5665)
==13517==    by 0x40233A: main (mruby.c:231)
==13517== 
==13517== LEAK SUMMARY:
==13517==    definitely lost: 16,000 bytes in 1,000 blocks
==13517==    indirectly lost: 0 bytes in 0 blocks
==13517==      possibly lost: 0 bytes in 0 blocks
==13517==    still reachable: 0 bytes in 0 blocks
==13517==         suppressed: 0 bytes in 0 blocks

これは、mrubyにおいては、C言語の構造体をデータとして持つようなクラスの場合、以下の記事にあるようにデータをGCでどう取り扱うかを明示しないといけないからです。

qiita.com

以下の宣言を mrb_mruby_passwd_example_gem_init の該当する箇所に追記すれば、GCをしてくれるようになります。

#include <mruby/class.h>
// ...
passwd = mrb_define_class(mrb, "Passwd", mrb->object_class);
MRB_SET_INSTANCE_TT(passwd, MRB_TT_DATA);

スタックにある変数を外に連れ出してしまった問題

続いて、valgrindはこういう警告も残しているかと思います。

==13727== Invalid read of size 4
==13727==    at 0x46119C: mrb_passwd_uid (mrb_passwd.c:78)
==13727==    by 0x430F74: mrb_context_run (vm.c:1126)
==13727==    by 0x435DCB: mrb_toplevel_run_keep (vm.c:2411)
==13727==    by 0x4362E3: load_exec (parse.y:5631)
==13727==    by 0x44577A: mrb_load_nstring_cxt (parse.y:5653)
==13727==    by 0x44577A: mrb_load_string_cxt (parse.y:5665)
==13727==    by 0x40233A: main (mruby.c:231)
==13727==  Address 0xffefffad0 is on thread 1's stack
==13727==  584 bytes below stack pointer
==13727== 
==13727== Invalid free() / delete / delete[] / realloc()
==13727==    at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13727==    by 0x461303: mrb_passwd_free (mrb_passwd.c:30)
==13727==    by 0x41050F: obj_free (gc.c:768)
==13727==    by 0x41050F: incremental_sweep_phase (gc.c:960)
==13727==    by 0x411EF5: incremental_gc (gc.c:1021)
==13727==    by 0x411EF5: incremental_gc_step (gc.c:1047)
==13727==    by 0x411EF5: mrb_incremental_gc (gc.c:1091)
==13727==    by 0x412A8C: mrb_obj_alloc (gc.c:483)
==13727==    by 0x408A5E: mrb_instance_alloc (class.c:1322)
==13727==    by 0x408A5E: mrb_instance_new (class.c:1346)
==13727==    by 0x430F74: mrb_context_run (vm.c:1126)
==13727==    by 0x435DCB: mrb_toplevel_run_keep (vm.c:2411)
==13727==    by 0x4362E3: load_exec (parse.y:5631)
==13727==    by 0x44577A: mrb_load_nstring_cxt (parse.y:5653)
==13727==    by 0x44577A: mrb_load_string_cxt (parse.y:5665)
==13727==    by 0x40233A: main (mruby.c:231)
==13727==  Address 0xffefffac0 is on thread 1's stack
==13727==  192 bytes below stack pointer

Invalid read/Invalid free() について考察してみます。

そもそも、元のコードは、 struct passwd * を mrb_passwd_init で宣言した変数内に格納しています。この変数は、このままではスタックに存在することになります。

static mrb_value mrb_passwd_init(mrb_state *mrb, mrb_value self)
{
  char *name;
  struct passwd pwd;
  struct passwd *res; // <- スタックに配置される
  size_t bufsize = 16384;
  char buf[bufsize];

  //...

  if(getpwnam_r(name, &pwd, buf, bufsize, &res) < 0)
    mrb_sys_fail(mrb, "getpwnam_r failed");
  else if(!res) {
    mrb_sys_fail(mrb, "entry not found");
  }

}

スタック領域とヒープ領域の詳細については省略します(この辺りの説明のイメージを持っておいていただければと)が、今回のように mrb_value を経由して、構造体をインスタンスのバックデータとして関数の外に連れ出すような場合に、そのデータがスタックに存在するのは問題があります。

なので、 malloc ファミリと呼ばれる関数群を利用して、そのような用途の変数を配置するヒープ領域にメモリを確保する方法が推奨されます。

一方で、 getpwnam_r は第５引数で struct passwd ** というポインタのポインタを取り、そこに取得した構造体のポインタを格納するという定義ですので、このような変数を上手にmallocするのは少し難しいです。

そこで、普通にmallocした構造体の先に、必要なメンバーを代入でコピーする、あるいは memcpy(3) を使う、という方法で、データをスタックから逃がしてあげることができるようです。

  data = (mrb_passwd *)mrb_malloc(mrb, sizeof(mrb_passwd));
  data->pwd = malloc(sizeof(struct passwd));
　// ...

  /* data->pwd->pw_name  = res->pw_name;
     data->pwd->pw_uid   = res->pw_uid;
     data->pwd->pw_gid   = res->pw_gid;
     data->pwd->pw_gecos = res->pw_gecos;
     data->pwd->pw_dir   = res->pw_dir;
     data->pwd->pw_shell = res->pw_shell; */
  /* ... or */
  memcpy(data->pwd, res, sizeof(struct passwd));

ちなみにCRubyの Etc モジュールでは、 struct passwdのデータから新規にRuby側で構造体を作成するという方法を取っています。

このような対応ののち、valgrindの（少なくともメモリリークの）警告はなくなりました！

$ valgrind --leak-check=full ./mruby/bin/mruby -e "1000.times {pwd = Passwd.new('vagrant'); pwd.uid}"
==14069== Memcheck, a memory error detector
==14069== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==14069== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==14069== Command: ./mruby/bin/mruby -e 1000.times\ {pwd\ =\ Passwd.new('vagrant');\ pwd.uid}
==14069== 
==14069== 
==14069== HEAP SUMMARY:
==14069==     in use at exit: 0 bytes in 0 blocks
==14069==   total heap usage: 5,544 allocs, 5,544 frees, 1,104,671 bytes allocated
==14069== 
==14069== All heap blocks were freed -- no leaks are possible
==14069== 
==14069== For counts of detected and suppressed errors, rerun with: -v
==14069== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

前回の記事で、 コンパイラさんの言うことを聞こう という内容を書いたような気がします。

これはもちろん基本的なことだとは思いますが、一方で valgrind さん、あるいは gdb さん、 SystemTap さんといった強いマサカリを投げてくる強い味方の意見もしっかりと活用できるようにすると、mruby(C言語...?)初心者を脱出できるのではないかと思います。...脱出したいですね...。

合わせて読みたい

blog.cybozu.io

ということで、mruby要素は半分ぐらいですがえいやと8日目として登録してしまおうと思います...（ええんかいな）

最後に、本記事であまり筋が良くない箇所や、間違い、励ましの言葉等ありました場合は是非教えていただければと思っております。

9日目は id:rrreeeyyy さんです。各文字は3文字ずつです。

追記

@udzura 指摘漏れてたんですけど、resがスタックというよりは、pwdとそのメンバをスタックに確保したbuffer領域から切り出して使っているからヒープで取り回す時は問題が生じる、という感じですね。だからコードのコメントはおかしいかも
— 松本亮介 / まつもとりー (@matsumotory) 2016年12月7日

matsumotoryさんの指摘を受け、以下のようなコードで各ポインタの情報をプリントしてみます。

  printf("&pwd = %p\n", &pwd);
  printf("res  = %p\n", res);
  printf("buf  = %p\n", buf);
  printf("pwd.pw_name = %p\n", pwd.pw_name);

  memcpy(data->pwd, res, sizeof(struct passwd));

  printf("data->pwd = %p\n", data->pwd);
  printf("data->pwd->pw_name = %p\n", data->pwd->pw_name);

すると... スタックで確保した buf のポインタがそのまま残ってしまうことがわかります。valgrind的にも、例えば data->pwd->pw_shell にアクセスしようとするとちゃんと警告が出てくる。

&pwd = 0xffefffac0
res  = 0xffefffac0 // <= 同じポインタ
buf  = 0xffefff6a0
pwd.pw_name = 0xffefff6a0 // <= buf の先頭
data->pwd = 0x55a2af0
data->pwd->pw_name = 0xffefff6a0 // <= memcpy に影響されず、 buf の先頭

なので、 getpwnam_r の定義は、以下のような意味なのです。

**result には、成功すると pwd のポインタが格納される。失敗するとNULLになる。
pwd が使う文字列は buf に置かれる。

つまりスタックを完全にきれいにするには、以下のようにするのが正しい。bufもヒープに置くので、こちらもちゃんとfreeしないといけないのがポイントです。

typedef struct mrb_passwd {
  struct passwd *pwd;
  char *buf; // <= 増える
} mrb_passwd;

static void mrb_passwd_free(mrb_state *mrb, void *p)
{
  mrb_passwd *pwd = (mrb_passwd *)p;
  free(pwd->pwd);
  free(pwd->buf); // <= ここ
  mrb_free(mrb, pwd);
}

static const struct mrb_data_type mrb_passwd_data_type = {
  "mrb_passwd", mrb_passwd_free,
};

static mrb_value mrb_passwd_init(mrb_state *mrb, mrb_value self)
{
  mrb_passwd *data;
  char *name;
  struct passwd pwd;
  size_t bufsize;
  bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
  if (bufsize == -1)
    bufsize = 16384;
  char *buf = mrb_malloc(bufsize); // <= ヒープから確保
  struct passwd *res;

  data = (mrb_passwd *)DATA_PTR(self);
  if (data) {
    mrb_passwd_free(mrb, data);
  }
  DATA_TYPE(self) = &mrb_passwd_data_type;
  DATA_PTR(self) = NULL;

  mrb_get_args(mrb, "z", &name);
  data = (mrb_passwd *)mrb_malloc(mrb, sizeof(mrb_passwd));
  data->pwd = mrb_malloc(sizeof(struct passwd));

  if(getpwnam_r(name, &pwd, buf, bufsize, &res) < 0)
    mrb_sys_fail(mrb, "getpwnam failed");
  else if(!res) {
    mrb_sys_fail(mrb, "entry not found");
  }

  memcpy(data->pwd, res, sizeof(struct passwd));
  data->buf = buf;

  DATA_PTR(self) = data;

  return self;
}

$ ./mruby/bin/mruby -e "pwd = Passwd.new('vagrant'); p pwd.shell"
&pwd = 0x7ffe1d1ddd80
res  = 0x7ffe1d1ddd80
buf  = 0x8a66d0 // ヒープ
pwd.pw_name = 0x8a66d0
data->pwd = 0x8712e0 // 全部ヒープにある
data->pwd->pw_name = 0x8a66d0

（ちなみに mrb_malloc は、普通であればmalloc相当を呼ぶだけになるはずなので、可搬性を考慮しなければどちらでもいいようですが、逆に何もこだわりがなければ mrb_* 付きのほうがよさそう。参考サイト）

ローファイ日記

出てくるコード片、ぼくが書いたものは断りがない場合 MIT License としています http://udzura.mit-license.org/

mruby-passwd のサンプルからわかるmgem（あと、C言語）を書くときの罠

このままではインスタンスのバックデータがGCされない問題

スタックにある変数を外に連れ出してしまった問題

合わせて読みたい

追記