開発ツール(QEMU)への貢献(後半) 〜自作OSのいまと昔 [第4回]

これまでのおさらい

前回の記事では、QEMUのVFAT機能にバグがあり、そしてその原因がメモリ破壊である、というところまでを突き止めました。
しかし、バグを発生させる直接の要因がわかっただけでは、そのバグを修正することはできません。今回はさらにもう一歩踏み込んで、どのような仕組みでメモリ破壊が発生したのかを突き止めると共に、それに基づいて修正パッチを投稿するところまでの道のりを紹介します。

lldbのwatchpoint機能

メモリ破壊系のバグらしき挙動を発見した際には、どのプログラムがそのアドレスのデータを書き換えたのか特定できれば、多くの場合原因が判明します。これを特定するために使える機能として、デバッガのwatchpointという機能があります。これは、特定のアドレスに対しての読み書きアクセスが発生した場合に、プログラムの動作を止めてデバッガで調査できる機能です。
この機能は通常、ハードウエアの支援、つまりCPUの機能を用いて実現されています。x86_64 アーキテクチャでこれがどのように実現されているのか興味がある人は、Intel SDM Vol.3 を "Debug Register"や"Debug Exception"で検索してみるとよいでしょう。（CPUの機能として実装されているため、自作OSのデバッグにも応用可能です。実際に、筆者が昔書いていた自作OSでは、デバッグ例外周りを実装することで、OS自身のメモリ管理機構のバグを見つけることができました）

さて、このwatchpoint機能は、lldbの場合、以下のようにして使うことができます。

(lldb) watchpoint set expression – <監視対象となるアドレスの値を生成する式>

このように、監視対象としたいアドレスの値、もしくはそれを生成する式を指定すればよいだけです。

では早速、前回特定したメモリ破壊が発生しているアドレス&child->bsをデバッガで調べて指定してみましょう……と言いたいところなのですが、残念ながらこの値は実行ごとに変化してしまうため、これではうまくいきません。おそらくchildが指しているBdrvChild構造体というのは、動的に確保されるものなのでしょう。

ということで、まずはBdrvChild構造体が、ソースコード上のどこで確保されるのかを突き止めましょう。そこでブレークポイントをしかければ、バグが起きるよりも前にwatchpointを設定でき、誰がメモリ破壊を引き起こしたのか突き止めることができるはずです。

BdrvChildはどこから来るのか

早速ソースコードを読んでいきますが、ここで気をつけなければならないのは、QEMUのソースコードは広大である、ということです。まじめに最初から最後まで読んでいては、気力が持ちません。
そこで、まずはEXC_BAD_ACCESSが発生した時の関数とスタックトレースを読んでみましょう。そうすれば、childがどこからやってくる値に依存しているのかわかるはずです。

frame #0: ... qemu-system-x86_64`bdrv_unset_inherits_from(..., child=0x00007f8058600e40) at block.c:2530:20
2527 {
2528 BdrvChild *c;
2529
-> 2530 if (child->bs->inherits_from == root) {
2531 /*
2532 * Remove inherits_from only when the last reference between root and
2533 * child->bs goes away.

最初の行をみてみると、引数としてchildが渡されていることがわかります。
では、呼び出し元はどこからこの値を取ってきたのでしょうか？今度はbtコマンド(back traceの略)を使って、スタックトレースを見てみましょう。

(lldb) bt
* thread #3, stop reason = EXC_BAD_ACCESS (code=1, address=0x1178)
* frame #0: 0x0000000105e623d7 qemu-system-x86_64`bdrv_unset_inherits_from(root=0x00007f8d4207f000, child=0x00007f8d407006e0) at block.c:2530:20
frame #1: 0x0000000105e623ad qemu-system-x86_64`bdrv_unref_child(parent=0x00007f8d4207f000, child=0x00007f8d407006e0) at block.c:2556:5
frame #2: 0x0000000105e6ebb0 qemu-system-x86_64`bdrv_close(bs=0x00007f8d4207f000) at block.c:4069:9
...

上から順に呼び出し元を辿っていくと、bdrv_close の引数には childが存在しないことに気付きます。ということは、この関数の中でchildをどこからか引っ張ってきているはずです。
ということで、bdrv_closeの処理をみてみましょう。上のスタックトレースによれば、block.cの4069行目あたりにありそうです。

static void bdrv_close(BlockDriverState *bs)
{
  BdrvAioNotifier *ban, *ban_next;
  BdrvChild *child, *next;
  ...
  QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
    bdrv_unref_child(bs, child); // <<<< line 4069: here!
  }
  ...
}

childという変数が最初に使用されるのは、QLIST_FOREACH_SAFE という謎のマクロの行です。その次の行で bdrv_unref_child に対してchildが渡されていることがわかります。

この謎マクロの名前をgit grep 'define QLIST_FOREACH_SAFE'みたいにして調べてみると、include/qemu/queue.hに宣言が見つかりました。

#define QLIST_FOREACH_SAFE(var, head, field, next_var) \
  for ((var) = ((head)->lh_first); \ 
  (var) && ((next_var) = ((var)->field.le_next), 1); \ 
  (var) = (next_var))

少し複雑ですが、じっくりとみると、どうもlinked listの各要素に対してfor文を回しているようです。
上のコードでは、リストの起点はBlockDriverState構造体のchildrenというメンバになっています。
ついでにリストの要素であるBdrvChildの宣言もみてみましょう。これはinclude/block/block_int.hにあるようです。

struct BdrvChild {
BlockDriverState *bs;
char *name;
const BdrvChildRole *role;
void *opaque;
...
QLIST_ENTRY(BdrvChild) next;
QLIST_ENTRY(BdrvChild) next_parent;
};

ほうほう、nextというメンバがあることから、確かにBdrvChildはリンクリストになっているようです。しかもBdrvChildのメンバには、先ほどリストへの起点を保持していたBlockDriverState型のメンバがあります。そう考えると、これら2つの構造体は、全体として再帰的な木構造になっているのではないか？と想像がつきます。

この想像は、先ほどのスタックトレースからも裏付けることができます。

スタックトレースの下の方を見てみると、bdrv_closeという関数が再帰的に呼び出されています。しかもbdrv_deleteやbdrv_unref_childという関数があることを考えると、これらのBlockDriverStateやBdrvChildは動的に確保されたものであると考えられます。おそらく、qemuの終了処理時に、確保したリソースをそれぞれ閉じて解放する処理が入っていて、その一環としてこの処理があるのでしょう。

ではBdrvChildを確保する処理も見つけてしまいましょう。動的に確保される構造体の確保には、大抵の場合それ専用の関数が存在していますから、行頭にBdrvChildがくるようなソースコードの行をリストアップすれば見つけられそうです。

$ git grep -n '^BdrvChild'
block.c:2403:BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
block.c:2474:BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
block.c:2798:BdrvChild *bdrv_open_child(const char *filename,
...

3つの関数定義が見つかりました。それぞれの実装を読んでみると、bdrv_open_childはbdrv_attach_childを、bdrv_attach_childはbdrv_root_attach_childをそれぞれ内部で呼んでいることがわかり、実際にメモリ確保を行う関数(ここではmallocのラッパであるg_new)を呼び出しているのはbdrv_root_attach_childであることがわかります。その実装は以下のような感じです。

BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, /* snip */)
{
  BdrvChild *child;
  ...
  child = g_new(BdrvChild, 1); // block.c:2422
  ...
  return child;
}

ここにブレークポイントを仕掛ければ、動的に確保される全てのBdrvChildのアドレスがわかりそうです。しかし、毎回ブレークポイントで止まった時にp childと打ち込むのも面倒なので、自動化してしまいましょう。

今度は、以下のようなファイルを作成して、lldb_cmd.txtという名前で保存します。

process launch --tty -s -- -drive format=raw,file=fat:rw:mnt
process handle -s false -p true SIGINT
process handle -s false -p true SIGUSR2
process handle -s false -p true SIGHUP
process handle -s false -p true SIGCONT
breakpoint set --file block.c --line 2423 --auto-continue true --command 'p child'
c

そして以下のようにしてlldbを起動すれば、自動でqemuを起動して上記コマンドを入力してブレークポイントを設定し、そのうえブレークポイントに当たったらそこでchildの値を自動で表示してくれます。その結果がこれです。

$ lldb qemu-system-x86_64 -s lldb_cmd.txt
...
(BdrvChild *) $0 = 0x0000000104800420
(BdrvChild *) $1 = 0x0000000102200c70
(BdrvChild *) $2 = 0x0000000102102f30
(BdrvChild *) $3 = 0x0000000102102d50
(BdrvChild *) $4 = 0x0000000104d001e0
(BdrvChild *) $5 = 0x00000001022014b0
(BdrvChild *) $6 = 0x00000001027003f0
(BdrvChild *) $7 = 0x0000000104d002b0

おお、ぞろぞろと出ていますね。では、ここでCtrl-CでQEMUを終了させ、EXC_BAD_ACCESSが発生した際のchildの値を見てみましょう。

Process 41291 stopped
* thread #2, stop reason = EXC_BAD_ACCESS (code=1, address=0x1178)
frame #0: 0x00000001005053d7 qemu-system-x86_64`bdrv_unset_inherits_from(root=0x0000000103081c00, child=0x0000000102102d50) at block.c:2530:20
2527 {
2528 BdrvChild *c;
2529
-> 2530 if (child->bs->inherits_from == root) {
2531 /*
2532 * Remove inherits_from only when the last reference between root and
2533 * child->bs goes away.
Target 0: (qemu-system-x86_64) stopped.

child=0x0000000102102d50は……ありました！確かに(BdrvChild *) $3 = 0x0000000102102d50ということで、ここで確保されているようですね。

せっかくなので、このアドレスがどのコードパスで確保されたものなのかも突き止めておきましょう。ブレークした際に実行するコマンドに、スタックトレースを表示する`bt`を付け足せば、どの関数がBdrvChildを確保する操作を発行したのかわかるはずです。

breakpoint set --file block.c --line 2423 --auto-continue true --command 'p child' --command 'bt'

lldb_cmd.txtのbreakpointを設定する行に、上記のように--command 'bt' を付け足して、再度同じことをやってみます。

(lldb) p child
(BdrvChild *) $3 = 0x0000000104200000
(lldb) bt
thread #2, stop reason = breakpoint 1.1
frame #0: ... qemu-system-x86_64`bdrv_root_attach_child( ... ) at block.c:2423:6
frame #1: ... qemu-system-x86_64`bdrv_attach_child( ... ) at block.c:2489:13
frame #2: ... qemu-system-x86_64`bdrv_open_child( ... ) at block.c:2812:12
frame #3: ... qemu-system-x86_64`enable_write_target( ... ) at vvfat.c:3194:15
frame #4: ... qemu-system-x86_64`vvfat_open( ... ) at vvfat.c:1252:19
...
Process 41487 stopped
* thread #2, stop reason = EXC_BAD_ACCESS (code=1, address=0x1178)
frame #0: 0x00000001005053d7 qemu-system-x86_64`bdrv_unset_inherits_from(root=0x0000000102876200, child=0x0000000104200000) at block.c:2530:20
2527 {
2528 BdrvChild *c;
2529
-> 2530 if (child->bs->inherits_from == root) {
2531 /*
2532 * Remove inherits_from only when the last reference between root and
2533 * child->bs goes away.
Target 0: (qemu-system-x86_64) stopped.

このスタックトレースをみるに、どうもvvfat.cにあるenable_write_targetという関数がbdrv_open_childを呼び出した結果、後々エラーを引き起こすBdrvChildが確保されるようです。
このvvfatという名称、見るからに怪しいですね……というのも、今回のバグが発生する条件は、QEMUの引数に-drive format=raw,file=fat:rw:mntと渡したときだけとわかっているからです。おそらくvvfat.cというファイルは、この仮想FATドライブ機能に関わる部分でしょう。早速ソースコードを見てみましょう。

static int enable_write_target(BlockDriverState *bs, Error **errp)
{
BDRVVVFATState *s = bs->opaque;
...
s->qcow = bdrv_open_child(s->qcow_filename, options, "write-target", bs,
&child_vvfat_qcow, false, errp); // block/vvfat.c:3195
...
}

たしかにここで確保しているようですね。では、先ほどのbreakpointを修正して、こちらを表示するようにしてみてもよさそうです。

process launch --tty -s -- -drive format=raw,file=fat:rw:mnt
process handle -s false -p true SIGINT
process handle -s false -p true SIGUSR2
process handle -s false -p true SIGHUP
process handle -s false -p true SIGCONT
breakpoint set --file block/vvfat.c --line 3196 --auto-continue true --command 'p s->qcow'
c

実行結果は以下のような感じです。

$ lldb qemu-system-x86_64 -s lldb_cmd.txt
(BdrvChild *) $0 = 0x00000001021277f0
(BdrvChild *) $1 = 0x00000001021277f0
(BdrvChild *) $2 = 0x00000001021277f0
...
Process 42415 stopped
* thread #2, stop reason = EXC_BAD_ACCESS (code=1, address=0x1178)
frame #0: 0x00000001005053d7 qemu-system-x86_64`bdrv_unset_inherits_from(root=0x0000000103072c00, child=0x00000001021277f0) at block.c:2530:20
2527 {
2528 BdrvChild *c;
2529
-> 2530 if (child->bs->inherits_from == root) {
2531 /*
2532 * Remove inherits_from only when the last reference between root and
2533 * child->bs goes away.
Target 0: (qemu-system-x86_64) stopped.

何回かこのコードパスを通っているようですが、明らかにここで使われているBdrvChildが、Ctrl-Cでの終了時に壊れているようです。

ここまでにわかったことをまとめると、以下のような感じでしょうか。

- block.cの2530行目、bdrv_unset_inherits_fromという関数の中で
- child->bsにあたるメモリの内容が何らかの理由で破壊されているため
- child->bs->inherits_fromへのアクセスがEXC_BAD_ACCESSを引き起こしている
- これを引き起こすchildはblock/vvfat.cのenable_write_targetという関数内で確保されているs->qcowと同一

だんだん核心に迫ってきた感じがありますね。もう少しです。

誰がBdrvChildを上書きしたのか

では先ほど説明したwatchpointの機能を使って、child->bsへの書き込みアクセスを捕捉してみましょうか。先ほどまでのbreakpointで実行していたコマンドを、watchpointの設定コマンドに置き換えて、さらにbreakpointが1回だけ発火するようにしておきましょう。watchpointは、デフォルトでは対象のアドレスへの書き込みアクセスのみを捕まえてくれます。

process launch --tty -s -- -drive format=raw,file=fat:rw:mnt
process handle -s false -p true SIGINT
process handle -s false -p true SIGUSR2
process handle -s false -p true SIGHUP
process handle -s false -p true SIGCONT
breakpoint set --file block/vvfat.c --line 3196 --auto-continue true --command 'watchpoint set expression &s->qcow->bs --one-shot true'
c

では実行してみましょう。実行すると、書き込み(w)に対するwatchpointを作成したとlldbが言ってくれます。

$ lldb qemu-system-x86_64 -s lldb_cmd.txt
...
(lldb) watchpoint set expression – &s->qcow->bs
Watchpoint created: Watchpoint 1: addr = 0x104000000 size = 8 state = enabled type = w

ではここで、Ctrl-CでQEMUを終了させてみましょう。すると、早速書き込みを検出して止まってくれます。

* thread #2, stop reason = watchpoint 1
frame #0: 0x000000010050a746 qemu-system-x86_64`bdrv_replace_child_noperm(child=0x0000000104000000, new_bs=0x0000000000000000) at block.c:2308:9
2305
2306 child->bs = new_bs;
2307
-> 2308 if (new_bs) {
2309 QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
2310
2311 /*

一応btも取ってみましょうか。

* thread #2, stop reason = watchpoint 1
* frame #0: 0x000000010050a746 qemu-system-x86_64`bdrv_replace_child_noperm(child=0x0000000104000000, new_bs=0x0000000000000000) at block.c:2308:9
frame #1: 0x0000000100504f66 qemu-system-x86_64`bdrv_replace_child(child=0x0000000104000000, new_bs=0x0000000000000000) at block.c:2354:5
frame #2: 0x000000010050535f qemu-system-x86_64`bdrv_detach_child(child=0x0000000104000000) at block.c:2507:5
frame #3: 0x00000001005052e0 qemu-system-x86_64`bdrv_root_unref_child(child=0x0000000104000000) at block.c:2518:5
frame #4: 0x00000001005053b6 qemu-system-x86_64`bdrv_unref_child(parent=0x0000000103077c00, child=0x0000000104000000) at block.c:2557:5
frame #5: 0x0000000100532533 qemu-system-x86_64`write_target_close(bs=0x000000010307be00) at vvfat.c:3129:5
frame #6: 0x0000000100511b4c qemu-system-x86_64`bdrv_close(bs=0x000000010307be00) at block.c:4063:13
frame #7: 0x000000010050dac0 qemu-system-x86_64`bdrv_delete(bs=0x000000010307be00) at block.c:4303:5
frame #8: 0x0000000100503d19 qemu-system-x86_64`bdrv_unref(bs=0x000000010307be00) at block.c:5670:9
frame #9: 0x00000001005052e9 qemu-system-x86_64`bdrv_root_unref_child(child=0x000000010246ddc0) at block.c:2519:5
frame #10: 0x00000001005053b6 qemu-system-x86_64`bdrv_unref_child(parent=0x0000000103077c00, child=0x000000010246ddc0) at block.c:2557:5
frame #11: 0x0000000100511bb0 qemu-system-x86_64`bdrv_close(bs=0x0000000103077c00) at block.c:4069:9
...

いいですね。ではconitnueっと。おや、またhitしました。

Watchpoint 1 hit:
old value: 0x0000000000000000
new value: 0x0000000000000000
Process 43254 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = watchpoint 1
frame #0: 0x00007fff73931f99 libsystem_malloc.dylib`tiny_free_list_add_ptr + 753
libsystem_malloc.dylib`tiny_free_list_add_ptr:
-> 0x7fff73931f99 <+753>: movq 0x278(%rdi), %rax
0x7fff73931fa0 <+760>: xorq %r11, %rax
0x7fff73931fa3 <+763>: movq %rax, %rcx
0x7fff73931fa6 <+766>: shrq $0x8, %rcx
Target 0: (qemu-system-x86_64) stopped.

今度はmallocがアクセスしたようです。まあfreeされた領域なんでmallocが触ることもあるでしょう。次いきましょう。

* thread #2, stop reason = EXC_BAD_ACCESS (code=1, address=0x1178)
    frame #0: 0x00000001005053d7 qemu-system-x86_64`bdrv_unset_inherits_from(root=0x0000000103077c00, child=0x0000000104000000) at block.c:2530:20
   2527 {
   2528     BdrvChild *c;
   2529
-> 2530     if (child->bs->inherits_from == root) {
   2531         /*
   2532          * Remove inherits_from only when the last reference between root and
   2533          * child->bs goes away.
Target 0: (qemu-system-x86_64) stopped.
(lldb) bt
* thread #2, stop reason = EXC_BAD_ACCESS (code=1, address=0x1178)
  * frame #0: 0x00000001005053d7 qemu-system-x86_64`bdrv_unset_inherits_from(root=0x0000000103077c00, child=0x0000000104000000) at block.c:2530:20
    frame #1: 0x00000001005053ad qemu-system-x86_64`bdrv_unref_child(parent=0x0000000103077c00, child=0x0000000104000000) at block.c:2556:5
    frame #2: 0x0000000100511bb0 qemu-system-x86_64`bdrv_close(bs=0x0000000103077c00) at block.c:4069:9
...

おっ、ついにEXC_BAD_ACCESSが発生しましたね。ところで、このときのchildのアドレスに着目して、先ほどのスタックトレースと今回のスタックトレースをみてみると、奇妙なことに気付きます。

それは、なぜ同じアドレスにあるBdrvChildに対して2回もbdrv_unref_childを読んでいるのか、という点です。言い換えれば、なぜか問題のBdrvChildは2回解放されようとしています。これはDouble Free（二重解放）と言われる類のバグで、明らかにおかしな挙動です。一度解放されたメモリ領域は、次に誰に再利用されてどんなデータが書き込まれようが自由ですから、メモリ破壊が起こっても何の不思議はありません。

というわけで、実はメモリ破壊自体が根本の原因ではなく、そもそも一度解放されたメモリ領域に触ろうとしていることが問題だった、ということがわかりました。

Double freeの原因

そうとわかれば、あとはなぜBdrvChildが二重解放されるに至ったのかを特定できればよさそうです。

さらにソースコードを読みこんでみたところ、以下のような仕組みで二重解放が発生しているということがわかりました。

まず最初に、青色のBlockDriverStateを解放するためにbdrv_closeが呼ばれます。するとbdrv_closeは、その子要素（黄色や赤のchild）に対しても順に解放を行います。これは、QLIST_FOREACH_SAFEというマクロを用いて、Linked List（緑色の点線で囲まれた領域）の要素を順に辿ってゆき、それぞれに対してbdrv_unref_childを呼び出すことで行われます。bdrv_unref_childは、引数で渡されたBdrvChildをLinked Listから取り除いてから、解放を行います。したがって、たとえ他のリストに入っているBdrvChildに対してこれを呼び出しても、リストから外されるので安全……だったはずでした。

しかし、この処理は常に安全というわけではなかったのです。
各BlockDriverStateでは、それぞれに固有の解放処理を設定することができます。先ほどの図で怪しいと言っていた緑色のwrite_target_closeという関数がまさに、VVFAT固有の解放処理を行う関数です。この関数も、BlockDriverStateの解放に伴って呼び出されます。そして、bs->opaque->qcowというオブジェクトを解放しようとしますが、ここで問題が生じます。なんと、ここで解放されようとするBdrvChildは、呼び出し元のbdrv_close関数で次に解放が行われるはずのBdrvChildだったのです。

え？でもBdrvChildは解放に伴ってリストから削除されるから、それは問題にならないんじゃないの？と思われるかもしれません。しかし、QLIST_FOREACH_SAFEのマクロに落とし穴があったのです。

#define QLIST_FOREACH_SAFE(var, head, field, next_var) \
  for ((var) = ((head)->lh_first); \
  (var) && ((next_var) = ((var)->field.le_next), 1);
  \ (var) = (next_var))

このマクロは、for文の中で現在のオブジェクトが解放されたとしても次の要素を安全に辿れるように、for文の本体に差し掛かるよりも前に、next_varという変数に次の要素のアドレスを保存しています。これが、SAFEという名前の理由だったわけです。しかし、もしそのnext_varが指すオブジェクトもfor文の中で解放されていたらどうなるでしょうか？……はい、それがまさに今回起きたことでした。SAFEだけど、SAFEじゃなかったんですね……！

解決策&パッチを書いて投稿する

というわけで、どちらかの解放処理をやめてあげればこのバグは解決しそうです。
ソースコードを読み進めたところ、BlockDriverStateを新たに確保する際は、自動的に親のBlockDriveStateの子として登録されるようになっていたので、前者の処理、つまりwrite_target_closeの処理を消せばよいはずであると私は判断しました。実際、該当の処理を消したところ、このバグは再現しなくなり、Ctrl-CでQEMUを止めても、うっとうしいエラーメッセージのウィンドウが出てくることはなくなりました。めでたしめでたし！

……さて、ここでこのバグ追跡物語を終えることもできるのですが、せっかくここまでして突き止めたバグとその修正を、私の手元だけに留めておくのはもったいないですよね？　もしかすると、世界中の誰かが、同じ問題に直面して困っているかもしれません。それに、QEMUを含む様々なオープンソースソフトウエア(OSS)は、こうやってユーザー自身がバグを発見して報告したり、その修正を直接行うことで、発展・維持されてきました。前回の記事でも述べたように、私たちがOS自作をする際も、さまざまなオープンソースソフトウエアを使っているわけですが、それができるのも、こうしたユーザー兼開発者のコミュニティが努力してきた結果です。できることなら、少しくらい恩返しをしてみたいですよね？

ということで、この修正を本家QEMUのほうでも取り込んでもらいましょう！

具体的な方法についてはQEMUのWikiに書いてある説明を読んでいただくことにして、個人的に気をつけるべきだと思っている点をいくつか挙げておきます。

1. コードのフォーマットや書き方を、既存のコードや指定されたコーディング規約に合わせる
2. パッチがどのようなものなのか十分に理解できるコミットメッセージ を書く
3. パッチがレビューされて取り込まれるまでは時間がかかるのでじっくり待つ
4. レビュアーも人間だということを忘れない

以下、それぞれについて具体的に説明します。

コーディング規約を守る

コーディング規約の存在は、趣味のひとりプロジェクトと、大人数で開発するOSSの間にある大きな違いのひとつです。コードなんて動けばそれでいいだろ！と思うかもしれませんが、たくさんの人が関わりながら開発する際には、コーディング規約を守ることで読みやすさやメンテナンス性を維持し、誰もが開発に参加しやすい環境を維持していく必要があります。
現代的なプロジェクトだと、ソースコードのフォーマット自体はclang-formatのようなフォーマッタで自動的に揃えることができる場合もありますが、QEMUのように古くからあるプロジェクトだと、何も考えずにこういったフォーマッタをかけると、変更していないソースコードの箇所までフォーマットされてしまうことがあるので注意が必要です。QEMUのContribute/SubmitAPatchを参照すると、パッチのフォーマットやスペルチェックを行うスクリプトの使い方が説明されていますので、こういった説明をよく読んで、規約に沿ったコードを書きましょう。

わかりやすいコミットメッセージを書く

わかりやすいコミットメッセージを書くことは、レビューする人だけでなく、パッチを送る人自身にも利益があります。というのも、わかりやすいコミットメッセージが書かれていれば、それだけスムーズにレビューが進み、短い時間でパッチを取り込んでもらえる可能性が高まるからです。
では、わかりやすいコミットメッセージとは一体なんでしょうか？　それは、このパッチが「何」をするもので、「なぜ」書かれたのか、という二点が押さえられているようなものだと私は考えています。例えば今回のパッチは「メモリの2重解放の結果発生するUse-after free」を回避するために「余計な解放処理を一つ消す」ということをしたので、これらの情報を、QEMUのことは知っていても、この問題には気づいていなかった人に分かるよう、背景も交えて説明する文章を書きました（GitHubから見ることができます）。英語で書かなくてはいけないので少したいへんですが、すでにマージされた他のコミットのメッセージを参考にしたりすると、それっぽい文章が書きやすいと思います。
また、パッチのタイトル（コミットメッセージの一行目）については、頭に変更したコンポーネントの名前をつけるなどの形式化がされていることがあるので、これも他のコミットメッセージやドキュメントを参考にするとよいコミットメッセージが書けるようになると思います。

レビューをじっくり待つ

レビューをじっくり待つのは、4番目とも関連していますが、わりと大切です。こういったOSSでは、レビュアーにも様々な人がいて、仕事としてこのプロジェクトに関わっている人もいれば、趣味で関わっている人ももちろんたくさんいます。それに、4番目に書いた通りレビュアーも人間ですから、体調を崩したり、休暇中だったり、うっかり見過ごして忘れてしまうというのもよく発生します。（実際に、年末にパッチを書いて送ったら、レビュアーが年末年始の休暇に入ってしまって、年明け後に「遅くなってごめんね！」と返事がきたときもありました）
ですから、パッチを投稿してからすぐ返事がなくても、またその返事が肯定的ではなくても、そんなに落ち込むことはありません。もし返事があまりにもこない場合は、レビュアーに"ping"してみるのもひとつの手です。（どのくらい待てばいいのかというのはかなり難しいところですが、例えばQEMUだとWikiに1-2週間待っても返事がなければpingしてね、と書いてありました）

レビュアーも人間だということを忘れない

「レビュアーも人間だということを忘れない」というのは、OSSにコントリビュートする上で一番大切といっても過言ではないかもしれません。
時に（というか頻繁に）、がんばって書いたパッチに対して、レビュアーから修正点の指摘を受けたり、そもそも方法が間違っている、という旨のコメントがつくことがあります。このような時、特にまだパッチを送ったりすることに慣れていない人は、どうすればいいか不安になったり、なんでだめなんだ！なぜわかってくれないんだ！と、もどかしい気持ちになることもあると思います（私もあります）。そういうとき、カッとなって適当にコメントを返したり、攻撃的なことを言ったりしたら、レビュアーの人はどう思うでしょうか？　もしくは、あきらめて返信するのをやめてしまったら？　それはもっと悲しいことです。
もちろん、無理をしてまでOSSに貢献する必要はまったくありませんが、レビュアーや他の開発者も、きっとパッチをはじめて送る人と全く同じように「ちょっとだけでもいいからこのOSSをよくしたい」と思っているはずです。ですから、そういったことが起きた時には、一晩眠ってから、すっきりとした頭でまた気分がよくなったときに作業を再開してみるといいかもしれません。

……とまあ、少し心構え的なことを書いてしまいましたが、こんなことを考えつつ私がメーリングリストにパッチを送ったところ、4日後くらいに取り込んだよーというメールがレビュワーから来て、その4日後くらいにメインツリーに取り込まれました。わーい！

おわりに

今回は少し自作OSから脇道にそれて、自作OSにかかわるOSSであるQEMUのデバッグをする道のりや、パッチを投稿する際の話をしました。OSはすべてのアプリケーションの下にいる縁の下の力持ちですが、そのOS自体も、様々なソフトウエアに支えられていることを意識していただけたら幸いです。皆さんも、もし気が向いたら&&何らかのツールのバグを自作OSを開発している最中に見つけたら、ぜひパッチを投げてみてください！