pixiv insideは移転しました! ≫ https://inside.pixiv.blog/

nginxの開発コミュニティにパッチを送った時の話

あけましておめでとうございます。年末年始はひどい風邪でほぼ布団の上だったbokkoです。 しかし、布団の上でもプログラミングはできるので過去に自分が作ったnginxのモジュールのコードを直したり、 ngx_mrubyにPull-Requestを送ったりして過ごしていました。

そんな中・・・

年が明けてもなかなか風邪が治らず、朦朧とした意識の中でngx_mrubyに機能を追加してnginx本体と 一緒にコンパイル & Valgrindで実行していると以下のエラーが出力されました。(nginxの拡張モジュールはnginx本体のビルド時に静的に組み込まれます)

1
2
3
4
5
6
7
8
9
10
11
12
==21338== Conditional jump or move depends on uninitialised value(s)
==21338==    at 0x4407AC: ngx_http_log_set_log (ngx_http_log_module.c:1278)
==21338==    by 0x417AE5: ngx_conf_parse (ngx_conf_file.c:387)
==21338==    by 0x433862: ngx_http_core_server (ngx_http_core_module.c:2944)
==21338==    by 0x417AE5: ngx_conf_parse (ngx_conf_file.c:387)
==21338==    by 0x42F508: ngx_http_block (ngx_http.c:239)
==21338==    by 0x417AE5: ngx_conf_parse (ngx_conf_file.c:387)
==21338==    by 0x4151F8: ngx_init_cycle (ngx_cycle.c:268)
==21338==    by 0x408B9E: main (nginx.c:333)
==21338==
                         (以下中略)

当初は自分が作った拡張モジュールのバグか何かだろうと思って調べ始めたのですが、「git stash」で作業中のコードを全部退避させて元に戻しても直る気配がなく、 試しにnginx本体に含まれていない外部の拡張モジュールを全部ビルド対象から外してみても上記のエラーは相変わらず出力されていました。

1
2
$ cd nginx-1.3.10
$ ./configure --with-pcre

そして何よりValgrindが出力しているスタックトレースをよく見ると原因はnginx本体にありそうに見えます。

1
2
3
4
5
6
7
8
9
# スタックトレース内の関数名が全部nginx本体の関数なので原因はnginx本体にあると考えられる
==21338== Conditional jump or move depends on uninitialised value(s)
==21338==    at 0x4407AC: ngx_http_log_set_log (ngx_http_log_module.c:1278)
==21338==    by 0x417AE5: ngx_conf_parse (ngx_conf_file.c:387)
==21338==    by 0x433862: ngx_http_core_server (ngx_http_core_module.c:2944)
==21338==    by 0x417AE5: ngx_conf_parse (ngx_conf_file.c:387)
==21338==    by 0x42F508: ngx_http_block (ngx_http.c:239)
==21338==    by 0x417AE5: ngx_conf_parse (ngx_conf_file.c:387)
==21338==    by 0x4151F8: ngx_init_cycle (ngx_cycle.c:268)
==21338==    by 0x408B9E: main (nginx.c:333)
==21338==

 

ちなみにこの「Conditional jump or move depends on uninitialised value(s)」というのは簡単に言うと 初期化されていない変数を使用している際に出力されるエラーです。C言語では変数を初期化してない場合、 その変数が取り得る値は不定となり、思わぬ動作を引き起こす可能性があります(「必ず」ではないのが嫌なところです。。。)。 なので、C言語でプログラムを書く時はValgrind-safeにしろとよく言った・・・いえ、言われたものです。(遠い目)

さて、私は昨年の12月に結構な頻度でngx_mrubyに修正を加えてPull-Requestを送ったりしていたのですが、 その頃は上記のエラーを見た記憶はありませんでした。そこでつい最近手元のnginxのバージョンを更新したのを思い出しました。 nginx.orgのTOPページを見てみると昨年のクリスマスに最新版がリリースされています。

1
2
3
4
5
6
2012-12-25 
nginx-1.3.10 development version has been released. Merry Christmas!
2012-12-11
nginx-1.2.6 stable version has been released.
2012-11-27
nginx-1.3.9 development version has been released, with support for chunked transfer encoding while reading client request body.

私の手元では1.3.10を使っていたので試しに1.2.6あるいは1.3.9に戻してみたところ、このエラーは出なくなりました。 なのでこのバグは当時(2013/01/01)の最新版である1.3.10で紛れ込んだものと見て間違いなさそうです。

原因を探る

さて、前置きが長くなりました。Valgrindが出力したスタックトレースを元に原因を探っていきます。 もう一度スタックトレースを見てみましょう。

1
2
3
4
5
6
7
8
==21338== Conditional jump or move depends on uninitialised value(s)
==21338==    at 0x4407AC: ngx_http_log_set_log (ngx_http_log_module.c:1278)
==21338==    by 0x417AE5: ngx_conf_parse (ngx_conf_file.c:387)
==21338==    by 0x433862: ngx_http_core_server (ngx_http_core_module.c:2944)
==21338==    by 0x417AE5: ngx_conf_parse (ngx_conf_file.c:387)
==21338==    by 0x42F508: ngx_http_block (ngx_http.c:239)
==21338==    by 0x417AE5: ngx_conf_parse (ngx_conf_file.c:387)
==21338==    by 0x4151F8: ngx_init_cycle (ngx_cycle.c:268)
==21338==    by 0x408B9E: main (nginx.c:333)
==21338==

問題の箇所はngx_http_log_module.cの1278行目ということがわかります。

1
2
3
4
// nginx-1.3.10/src/http/module/ngx_http_log_module.c
1278         if (log->file->data) {
1279             buffer = log->file->data;
1280

このlog->file->dataという変数が初期化されてないということのようです。 それぞれの変数の型を調べていくとlog->fileがstruct ngx_open_file_sのポインタ、 そのメンバ変数であるdataがvoidポインタとして定義されていることがわかります。

1
2
3
4
5
6
7
8
// nginx-1.3.10/src/core/ngx_conf_file.h
90 struct ngx_open_file_s {
91     ngx_fd_t              fd;
92     ngx_str_t             name;
93
94     void                (*flush)(ngx_open_file_t *file, ngx_log_t *log);
95     void                 *data;
96 };

さて、そうなるとlog->file(struct ngx_open_file_sのポインタ)とそのメンバ変数を初期化している箇所が怪しそうです。 該当箇所よりもちょっと前に戻ってみるとlog->fileはngx_conf_open_file関数を使って初期化していることがわかりました。

1
2
3
4
5
// nginx-1.3.10/src/http/module/ngx_http_log_module.c
1131         log->file = ngx_conf_open_file(cf->cycle, &value[1]);
1132         if (log->file == NULL) {
1133             return NGX_CONF_ERROR;
1134         }

この関数の実装は以下のようになっています。

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
// nginx-1.3.10/src/core/ngx_conf_file.c
891 ngx_open_file_t *
892 ngx_conf_open_file(ngx_cycle_t *cycle, ngx_str_t *name)
893 {
894     ngx_str_t         full;
895     ngx_uint_t        i;
896     ngx_list_part_t  *part;
897     ngx_open_file_t  *file;
898
899 #if (NGX_SUPPRESS_WARN)
900     ngx_str_null(&full);
901 #endif
902
903     if (name->len) {
904         full = *name;
905
906         if (ngx_conf_full_name(cycle, &full, 0) != NGX_OK) {
907             return NULL;
908         }
909
910         part = &cycle->open_files.part;
911         file = part->elts;
912
913         for (i = 0; /* void */ ; i++) {
914
915             if (i >= part->nelts) {
916                 if (part->next == NULL) {
917                     break;
918                 }
919                 part = part->next;
920                 file = part->elts;
921                 i = 0;
922             }
923
924             if (full.len != file[i].name.len) {
925                 continue;
926             }
927
928             if (ngx_strcmp(full.data, file[i].name.data) == 0) {
929                 return &file[i];
930             }
931         }
932     }
933
934     file = ngx_list_push(&cycle->open_files);
935     if (file == NULL) {
936         return NULL;
937     }
938
939     if (name->len) {
940         file->fd = NGX_INVALID_FILE;
941         file->name = full;
942
943     } else {
944         file->fd = ngx_stderr;
945         file->name = *name;
946     }
947
948     file->flush = NULL;
949
950     return file;
951 }

ちょっと長いですが、よく読んでみるとstruct ngx_open_file_s(ngx_open_file_t)のメンバ変数dataを初期化していないことがわかります。 なのでこの関数内でvoidポインタのdata変数をNULLで初期化してやるとよさそうです。以下がそのdiffになります。

1
2
3
4
5
6
7
8
9
diff -ur orig/nginx-1.3.10/src/core/ngx_conf_file.c nginx-1.3.10/src/core/ngx_conf_file.c
--- orig/nginx-1.3.10/src/core/ngx_conf_file.c  2012-12-24 00:36:52.000000000 +0900
+++ nginx-1.3.10/src/core/ngx_conf_file.c       2013-01-01 17:35:06.991854337 +0900
@@ -946,6 +946,7 @@
     }
     file->flush = NULL;
+    file->data  = NULL;
     return file;
 }

そしてこのパッチで今までValgrindが出力していた未初期化エラーは出なくなりました。 私の環境では確認できなかったのですが、設定ファイルの書き方次第ではこのバグが原因で セグメンテーション違反を引き起こすことがあるようです。Nginx 1.3.10 Segfault

nginxの開発チームにパッチを送る

さて、せっかく修正パッチを書いたのでnginxの開発チームにバグレポートとともに送りたいところです。 公式のWikiにあるNginx Communityのページを見ると、

1
2
Bug reporting and feature requests
Bugs and feature requests should be submitted via Trac(http://trac.nginx.orgへのリンク).

とあるのでとりあえずこのTrac上でレポートを書いて送れば良さそうです。 実際送ったレポート(チケット)はこちらになります。

コミットそしてリリース

この1行パッチは先日の2013/01/10にリリースされたnginx-1.3.11にそのまま取り込まれました。 nginxにはプライベートでも仕事でも非常にお世話になっているので、こういった形で貢献できるのは ソフトウェアエンジニアとしてとても嬉しいです。

最後に

以上、nginxの開発コミュニティにパッチを送った時の話でした。 nginxに限らず自分以外のエンジニアが開発しているプロダクトにバグレポートやパッチを送るのは敷居が高いように感じる方も多いと思いますが、 勇気を出して最初の一歩を踏み出してみましょう。

(※)nginxは2条項BSDライセンスで公開されているオープンソースソフトウェアです。本記事で掲載しているnginxのソースコードの利用についてもそのライセンスに準ずるものとします。