Feb 08, 2010
evalが危険でそれ以外の方法が安全だと思ってる人へ
先日、ソース表示タブのアップデート版をAMOにアップロードしたところ、公開申請が却下されました。「不必要なeval()が多すぎる。拡張機能におけるeval()の5つの間違った使い方（原文：Five wrong reasons to use eval() in an extension）をよく読んで出直してこい。（大意）」という感じのコメントが添えられていました。
また、ツリー型タブのアップデート版も別の人から同じコメント付きで公開申請を蹴られました。
全文＋コメントを翻訳するくらいには当該エントリを読んだ僕ですから、今更読み返すこともないのですが、こう何度もこのエントリを引き合いに出して申請を蹴られると正直腹に据えかねる物がありますので、自分の考えをちょっとまとめておきたいと思います。
evalの使い方に「正しい」も「間違っている」もない
まず何より明らかにしておきたいのは、言葉尻を捉えて反論するのもどうかと言われそうですけれども、evalの使い方には「正しい」も「間違っている」も無いということです。
例えばHTMLの仕様書には、em要素は強調を意味するとか、この要素はこの要素の中には記述できないとかの、色々な定義が書かれています。その定義から外れた使い方をすれば、確かにそれはinvalidな（間違っている）HTMLと言っていいでしょう。
では、evalはどうか。ECMAScriptの仕様書（リンク先のページには3rd Editionと書いてあるけどPDFは5th Editionです）の15.1.2.1 eval (x)
の項には、evalの仕様としては「与えられた引数をスクリプトとして実行する」という、挙動についての取り決めだけが書かれています。「何のためになら使ってよい」という風な記述は一切ありません。当たり前といえば当たり前ですが。
そこにあるのはただ、「その仕様の通りに動作した結果、何が起こるのか？」という結果についてのリスクの高低だけです。多大なリスクを伴う使い方、全くリスクを伴わない使い方、色々な使い方が考えられますが、動作する限りはそれらはすべて「仕様通りの正しい使い方」です。なので当該エントリのタイトルも本当は「evalの5つの危険な使い方」などとするのが妥当だと僕は思っています。
こういう場合にこういうデメリットがあること考えられる、だからこのような対策が必要だ、あるいはメリットとデメリットを天秤にかけないといけない、というのがリスク管理の考え方です。「間違っているから駄目だ」と思考停止するのは、天秤にかける事すら否定するということです。
例えばSSLを使っていない普通のHTTP接続で閲覧しているWebページの内容は、「第3者によって改竄されている可能性」を完全には否定できません。だからといって、そのページに書かれている情報は一切全く信用してはいけない、ニュースだろうが料理のレシピだろうが一切アテにしてはいけない、という考え方が健康的と言えるでしょうか？ また、SSLは接続している先のWebサイトが本当にその運営者が運営するものであるかどうかということは保証してくれますが、運営者が善人か悪人かということまでは保証してくれません。相手が善人であると保証されないのだからネットで買い物はしたくない、カップラーメンもマンガの本もホッチキスの針も、詐欺に遭うのが怖いからネット経由では絶対何も買わない、という考え方は妥当でしょうか？ そこまで行くともはやインターネット<ruby><rb>恐怖症</rb><rp>（</rp><rt class="読み">フォビア</rt><rp>）</rp></ruby>でしょう。
前出のエントリで「この使い方は正しい」とされている以外の使い方がされているevalを、個々のケースごとのリスクの高低を無視して一律で否定するのは、僕には単なるeval<ruby><rb>恐怖症</rb><rp>（</rp><rt class="読み">フォビア</rt><rp>）</rp></ruby>としか思えません。
そのevalには本当に、Webからやってきたコードが混入する恐れがあるのか？
前出のエントリで挙げられている5つのケースのうち、1番目（JSON形式のデータのパース）、2番目（プロパティ名が動的に変化する時に、そのオブジェクトのプロパティにアクセスするため）、4番目（HTMLやXULにインラインで記述されたイベントハンドラの実行）の3つのケースでは、Webからやってきた信頼されていないコードを特権付きで実行してしまう可能性があることに触れています。これはWebアプリケーションにおいてのSQLインジェクションに対する注意喚起と同様の物と言えます。
この観点でevalの使い方に神経質になることは大事です。僕も、evalを使うすべての人は、この点に気をつけなくてはならないと思います。この理由で公開申請が却下されたのであれば、その判断は実に妥当だと思いますし、反論の余地は全くありません。
では、今回のケースはどうでしょうか。
ソース表示タブやツリー型タブでは、evalはFirefox本体の関数を書き換えるためにだけ利用しており、そこにWebから取得したコードが混入するとは考えにくいです。もしこの理由で申請が却下されたのであれば、それは、レビュアーがきちんとコードを読まずに、単に機械的チェックにおけるevalの警告だけを見て判断を下したという可能性が出てきます。これは言い換えると、「evalが機械的チェックで検出されたからNG、検出されなかったからOK」という風な安直な判断を下すようになっている可能性があるということです。
もしそうなのであれば、そのようなレビュー姿勢は宜しくないものだと自分には思えます。そういう機械的なチェックをくぐり抜けるための抜け道はいくらでもあります。難読化されたコードなどはその典型ですし、難読化されていない「読みやすそうなコード」でも、ぱっと見分からないようにごまかすのは簡単です。もし機械的なチェックの結果だけを見て判断を下しているのであれば、そういうケースによる危険なコードの混入の可能性は否定できないことになります（そういう場合、時間をかけてでも・判断が遅くなってでもきちんと精査するべきでしょう）。そういうケースに対しても対処できるようにというのが、わざわざ人の手でレビューすることになっている理由の1つであると自分は信じています。
evalによる関数の書き換えのリスクは、それ以外の方法のリスクと比べてそんなに高いのか？
ソース表示タブやツリー型タブで使われているevalはすべて、前出のエントリで挙げられている5つのケースのうち5番目のケース、「Firefox内で使われている関数の内容を書き換えるため」に該当するものです。前出のエントリでは、この使い方を以下の理由から非難しています。
- 書き換える対象の関数の内容が変化した時に、ブラウザ自体がまともに動作しなくなってしまう恐れがある。
- せっかく修正されたセキュリティ上の問題を、再び持ち込んでしまう恐れがある。
そして、代わりに推奨するやり方として以下のような「無理のないやり方」を挙げています。
- 関数を別の関数で置き換えた上で、置き換え後の関数内で元の関数を
func.apply(this, arguments)で呼ぶ。 Object.watch()を使って、特定の変数やプロパティの値が変化したタイミングで処理を行う。- そういう「無理のないやり方」では実現できないことをしようとしているのであれば、もう素直に諦める。
換言すると、前出のエントリの筆者は「このような無理のないやり方の方が、evalによる関数の置き換えに比べて、前述のような事態が発生するリスクが少ない」と述べていると言えます。
僕には、これはアンフェアな言い方と思えます。リスクの多寡で問題を語るのであれば、ここに挙げられていないリスクについても考慮するべきでしょう。そうすることで初めて、僕のような人が「evalによる関数の書き換え」という手法を選択している理由が明らかになります。僕だって、考えなしにevalを使っているわけではありません。メリットとデメリットを天秤にかけた上で、その方がリスクが小さいと考えてevalを使っているのですから。
リスク：Firefox自身の機能や他のアドオンと競合する可能性について
アドオンを「機能を実現する方法」の観点で見ると、以下のように分類できます。
- Firefoxや他のアドオンが提供するAPIに則ったアドオン。ツールバーへのボタン追加、サイドバーパネルの追加、nsIContentPolicyによるコンテンツフィルタリング（AdBlock Plus）、Jetpack Featureなど。
- Firefoxや他のアドオンがAPIを提供していない箇所に改変を加えるアドオン。
- XBLを使って機能を加えるアドオン。
- 新規にXBLを適用するアドオン。
- 既存のXBLを上書きするアドオン。Tab Mix Plusなど。
- JavaScriptの関数のはたらきを変更して機能を加えるアドオン。
- 関数を置き換えるアドオン。
Object.watch()等で本来の処理に割り込むアドオン。- 関数をevalで書き換えるアドオン。
- CSSでXUL要素の見た目を変えるアドオン。Personas、テーマなど。
- XBLを使って機能を加えるアドオン。
1つのアドオンが複数の分類にまたがる場合も多いのですが、このうち「他のアドオンとの競合の可能性」という点で本当に低リスクであると言い切れるのは、1の「APIに則ったアドオン」の枠から外れないものだけです。それ以外はどんなアドオンであっても、Firefox自体の挙動や他のアドオンに何らかの影響を与える可能性があります。テーマですら例外ではありません。
前述の「無理のないやり方」の説明では、さもそのようなやり方を採用することによって競合のリスクが全面的に低減されるかのような書き方がなされていますが、場合によってはこの方が却って競合してしまう場合すらあります。実際、Firefox 3.6上でサイドバーの開閉状態の変化を捕捉するためにIDがsidebar-boxである要素のhiddenプロパティに対してObject.watch()で監視を行おうとしたところ、XUL要素の挙動が破壊されて、サイドバーの開閉自体が行われなくなってしまいました。（これは結局、DOMAttrModifiedイベントを監視してhidden属性の値の変化を見るという方法で回避することにしました。）
別の例として、「無理のないやり方」で関数を丸ごと置き換えられてしまうと、evalで元の関数を書き換えようとしていた他のアドオンが、関数を書き換えられないせいで期待通りに動作しなくなってしまうということも考えられます。……「そんな邪道なやり方をしている方が悪いのだ」という非難は無しですよ？ 他のアドオンに与える影響を考えていないのは、お互い様なのですから。
また、これはリスクとは別の話ですが、「evalによって特定の箇所に特定のコードを注入することによって関数の定義内容を変えてしまわなければ実現できない機能」というものもあります（前出のエントリに対するコメントでも、Dorando氏が例を示しています）。そのような「無理なやり方」でやらなければならない事はするべきではない、というのが前出のエントリの著者の結論のようですが、後述する物も含めたリスクの点で明らかな優位性を主張できないのであれば、「無理なやり方はするべきでない、なぜならevalの間違った使い方だからだ」「evalの間違った使い方はするべきでない、なぜならそれは無理なやり方だからだ」というトートロジーに過ぎません。
リスク：Firefoxのバージョンが変わると予期しない結果になる可能性について
この点のリスクは、evalによる関数の書き換えでも、「無理のないやり方」と呼ばれている関数の置き換えでも、同様に存在します。前出のエントリでは「置き換え対象の関数の内容が変わったらどうするんだ」ということを指摘していますが、それを言えば、関数の名前が変われば「無理のないやり方」も破綻します。セキュリティアップデートでは関数の名前やそれぞれの働きが変わることはない、というのはただの思い込みです。そんなルールはどこにもありません。
また、「置き換えた関数A」→「置き換えた関数B」の順で処理が行われることを期待してコードを書いてる場合に、「置き換えた関数A」だけが実際には置き換えられなかったとすると、「置き換えた関数B」が動作する上での前提が崩れますので、その時どんなトラブルが起こるかは予想できません。これはFirefoxのバージョンが変わった時だけでなく、他のアドオンと同時に利用した時の競合についても全く同じ事が言えます。
いずれの場合にせよ、Firefox本体や他のアドオンのバージョンアップに対して、改変を加える箇所のソースコードをその都度調査し直さなければならないという、メンテナンスのコストの問題は、どちらにも共通して存在しています。「evalと関数置き換えのどちらの方が元のコードの変化に強いのか」ということを論じてもあまり意味はなく、どちらの手法を使うにせよ、Firefoxのバージョンアップに際してきちんと動作を検証しているかどうかや、前提が崩れた時のためのフェイルセーフがきちんと考えられているかどうかの方が、リスクに対する備えとしては重要且つ効果的なのではないか？ というのが僕の考えです。
Firefoxのアドオンやテーマの正体は「動的に適用されるパッチ」です。ソースコードに対して、そこら辺に転がってる野良パッチをあててビルドする、というのはLinuxを使ってる人にはよくある光景ではないかと思いますが、「パッチが書かれた時のバージョンと違うバージョンのソースにパッチを当てること」がいかに危険かは、言うまでもないでしょう。Firefoxのアドオンを「アドオン作者によって動作確認されたバージョン」以外のバージョンにインストールすることは、それと全く同じ事です。
リスク：メンテナンス性、ソースの可読性が低下する可能性について
evalによる関数書き換えの場合、ソース中には「書き換える対象の関数名」「書き換えたい前のコード片」「書き換え後のコード片」だけが書かれることになります。
このコードを解読するためには「書き換える対象の関数」の内容を参照する必要があるため、その内容が頭に入っていない場合（大抵はそうでしょう）、解読が難しくなりがちです。その代わり、ソースコードの量自体は必要最低限になります。また、書き換える対象がロジックに絡まない「widthをheightに書き換える」という風な部分なのであれば、コードの読みやすさという点でもさほど問題は無いと言えます。このあたりは、「evalで関数を書き換える時の、互換性や後々のメンテナンス性を高く保つためのノウハウ」が重要になってくる所です。
関数の置き換えを行う場合も、ソースコードは結局は断片的な物になります。「前処理の関数」「元の関数をラップする関数」「後処理の関数」「元の関数の中で特定のプロパティに変更が加えられた事を捕捉して処理を割り込ませる関数」などが細切れに定義されますし、また、元の関数の処理結果の受け取り方法として「戻り値」以外の情報を使うのであれば、結局は元の関数の内容が頭に入っていないといけないことになります。
元の関数の中に変更を加えなければどうにもならない機能をどうにかして実現したい場合（前出のエントリのコメントで指摘されているようなケースなど）、こちらの手法にこだわるのであれば、「元の関数に必要な変更を施した後の関数」をアドオンの中で定義しておく以外に手はありません。そうなると、Firefoxや他のアドオンのコードと全く同じコードがそのアドオンの中に大量に含まれるようになってしまいます。これは、以下のような問題に繋がります。
- それぞれのライセンスが衝突してしまうために、コードを丸ごと引用できないという事態が起こり得る。その機能を実現できない、という苦渋の決断を迫られる。
- 前出のエントリ内でも指摘されている「せっかくFirefox（や他のアドオン）の側でセキュリティ上の問題が修正されても、関数を古いバージョンのそれに基づいた改変版に丸ごと置き換えてしまうことで、再びセキュリティ上の問題が持ち込まれてしまう」問題が起こり得る。
- 単純にソースの行数が増加する事それ自体も問題となる。
リスク：公開申請時のレビューが通りにくくなる可能性について
これは「メンテナンス性、ソースの可読性が低下する可能性」にも共通する話です。AMOのエディターは最終的にはコードレビューによってそのアドオンの安全性を精査する必要があり、コードがトリッキーなものであればあるほど、また、コードの行数が増えれば増えるほど、コードレビューは大変な物になります。その意味で、関数の部分的な書き換えも関数全体の置き換えも、コードレビューの手間が増えるという点では一長一短です。
もっとも、今回の事例のように「evalを使っていること」を理由に公開申請が却下されることが相次ぐのであれば、eval無しでトリッキーで可読性の低い分量の多いソースコードの方が、比較的レビューが通りやすいという事になるのかもしれませんけれどもね。その場合、これもやはり「レビューを通してもらうためにはevalを使わない方がいい、なぜなら、evalを使っているとレビューが通らないからだ」という、何の説明にもなっていないトートロジーになる訳ですが。
まとめ
上記のような理由で、僕はソース表示タブやツリー型タブの中で使われているevalが不必要な物ばかりであるとは考えていません（むしろどれも必要だからそうしているというのが自分の立場です）し、それらのevalの個々の用法が「これを読んで出直せ」と提示されたエントリの中で書かれている「代替案」に比べて目立ってリスキーであるとも思っていませんし、その「代替案」で代替できるとも思えません。
「evalの方が危険だ」と言っている人のうち何割の人が、本当にMozillaのコードを読んだことがあるのか、読み続けてきたのか、APIが用意されていない箇所でブラウザの機能を拡張する事に取り組んできたのか、継続的にFirefoxのバージョンアップに追従し続けたことがあるのか、他のアドオンとの競合を解消する方法を模索し続けてきたのか、僕には疑問に思えます。
XPCOMコンポーネントのIDL定義で「FROZEN」というフラグが立っている物以外のすべてのAPIは、いくらでも変更される可能性があります。FROZENになっているAPIは、Preferenceの読み書き、W3CのDOM関連など、全体から見れば本当にごく一部だけです。ブラウザ内のJavaScriptのレイヤで定義された関数に至っては、それが変更されない保証などどこにもありません。「Firefoxのバージョンに依存しないAPIが提供される」と思われていたFUELですら廃止される可能性が出てきている、Mozillaはそんな世界なのです。それで「evalの方が危険だ、同じ名前の関数で置き換える方がマシだ」と言うのがどれほど馬鹿げた話か。
ですので僕にとっては、evalの使用を理由として公開申請が却下された今回の出来事に対しては、不当な判断を下されたという思いが強いです。
wikieditish message: Ready to edit this entry.