cargo crev でコードレビューをしてみたらバグを見付けた話など

依存とコードへの信頼

C 言語の時代から「他人が書いた他のライブラリに依存したコードを書く」という状況は少なからずあったわけだが、現代的な環境ではこれが殊更に加速している。 開発環境やツールチェインが外部ライブラリの簡単な利用や、ライブラリの簡単な公開をサポートしているためである。 そうすると、外部ライブラリの利用や公開への障壁が小さくなり、より小さな機能やマイナーな機能を提供するライブラリの利用や公開が加速し、結果としてプロジェクトの依存関係は膨らみがちになる。

依存ライブラリに深刻なバグや悪意あるコードが紛れ込むと自分のプロジェクトまでそのコードを抱えることになるため、できればこれは避けたい。 依存先プロジェクトが少なければ、依存先のライブラリを自分の目で確認することもできるかもしれない。 依存先プロジェクトが有名であれば、多くの人によって確認されたコードは信頼できるかもしれない。 しかし、そのどちらでもない場合、プロジェクトが依存しているコードは本当に安全であるといえるだろうか？

……という問題も、今に始まったことでなく昔から議論されていたのだろうが、問題を認識しつつ末端の趣味開発者として実際にできることはそんなに多くない。 セキュリティが重要なプロジェクトであれば、お金やマンパワーを投入してコードを監査 (audit) することもできるだろうが、趣味のプロジェクトでそこまでできる人もそういまい。 加えて、監査する主体にも社会的信用がなければ、見ず知らずの人が「このコードは信用できます！」などと言ったところで誰も信用しないだろう。 さらにはそういった現状も相俟って、「このコードは信用できます！」と主張する人自体がそもそも少ない。 そんなわけで、世間のコード、また我々が利用するコードの大部分は、安全であるということが明確に認められないままなんとなく利用されている。

つまり、コードを信頼するか決めるにあたって、以下のことが大きな障害となる。

• 安全性を確認すべきコードが多いこと
• 安全性を確認したという報告が少ないか、あるいは目に付かないこと
• コードの安全性を確認した (と主張する) 赤の他人の言葉を信用しかねること

他人への信頼と Web of Trust

上記の問題について、実のところ1番目と3番目については技術的な解決策が昔から知られている。 これは Web of Trust^[0] (信用の輪) と呼ばれるもので、実際に交流したことのない誰かの電子署名 ^[1] (あるいは PGP 公開鍵) が本当に本人のものであると信用すべきかという問題を解決する仕組みである。

大雑把に言うと、本人であることの信用関係をグラフ構造で表現し、グラフのエッジに管理能力の信頼度や本人性の信用度で重み付けをし、エッジを辿るたびに重みに応じて信用度を減衰させていくことで、ある署名が本当に本人のものであるかの信用度を算出するという、発想自体はシンプルな仕組みであり、このグラフこそが Web of Trust である。 これにより、「信頼できる友人の言ならまあ信用できるし、信頼できる友人が信頼しているという別の友人の言ならそこそこ信用できる」という自然な仕組みをコンピュータの世界に持ち込める。

また、この仕組みは「○○は万人が信用しているものとする」といったような天下り的な信用を必要とせず機能するため、多数の多様なユーザが存在する状況においてもうまく機能を発揮できる。

この Web of Trust の仕組みは本人性の確認以外にも利用でき、これをコードレビューのエコシステムへと持ち込んだのが cargo-crev である。

コードレビュー

cargo-crev について語る前に、コードレビューについて軽く説明しておく。

多くの場合、コードレビューとは、プロジェクトのメンバーやリポジトリの管理に責任や強い権限を持っている人が、プロジェクトに取り込むコードの品質や問題を確認するものである。 しかし OSS の場合、特に個人開発の小規模なプロジェクトでは正式なメンバーは一人か数人であることが多い。 この場合のコードレビューは、その数人が、それ以外の人 (つまりプロジェクトのメンバーではないがパッチを投稿してくれた人) のコードの品質を確認し、必要に応じて修正を求めたり自ら修正するというものになる。 こういったコードレビューは、プロジェクトのコードそのものというより、実際には「コードへの変更」をレビューするものである。

依存先プロジェクトのコードを信用できるかという文脈で考えるとき、コードの品質はプロジェクトの管理者やメンバー当人ばかりでなく、第三者の目による品質確認もあった方がよい。 何故なら、悪意ある開発者や問題に気付かない開発者は自分のコードは信用できると言い張るだろうが、多数の人々が確認をすれば、そのうちいくらかの人は問題に気付いて指摘できるかもしれないからである。 この場合、どちらかというとコードレビューは品質保持のための手順というよりは、既にあるコードに不正や問題がないか確認する監査に近いものである。 また、ここでの監査のようなコードレビューは、各々の小さな変更ではなく、今あるコード全体、特定のバージョンのスナップショットそのものについてのレビューが求められる。

この監査という行為に監査者自身への信用が必要であることは先述の通りだが、コードレビューを行った多数の第三者をどの程度信用するかという問題は Web of Trust で解決できそうである。

crev, cargo-crev はどう問題を解決するか

crev とは、多数の第三者による草の根的なコードレビューをうまく共有し、また Web of Trust のモデルに基いてそれらのコードレビューへの信用をうまく活用できるようにしようという仕組みである。 この仕組み自体は、特定の言語やストレージや中央サーバなどには非依存で、今のところ Rust 言語用の cargo-crev は既に利用可能なレベルで作られており、 JavaScript 用の npm-crev は開発途上らしい。

cargo-crev は、以下のようなモデルで動作する。

• 各レビュアーは、自分の「他レビュアーへの信頼・信用情報」と「自分 (もしかすると他人も？) のコードレビュー結果」を各々の決めた場所で公開する。
  • 信頼・信用情報とコードレビュー結果は電子署名が付与されており、 WoT の仕組みによって偽造が困難に、また検出可能になっている。
• コードレビュー情報の利用者は、自分の信頼・信用するレビュアーを足掛かりに、信頼するレビュアーが信頼する別のレビュアーなど様々な人々によるレビュー情報を取得し、ローカルで信用度情報を算出し利用する。
  • ユーザが誰を信用しているかによって、ローカルで算出されたプロジェクトの信用度は異なるということである。

これにより、次のような結果が期待できる。

• 自分が信頼・信用した人の目を通してコードが信用できるかを、ある程度主観的かつ自動化して判断できる。
  • ここでいう自動化とは、「あなたはこれを信用していますか？」といちいち尋ねる必要がないという意味であって、人の脳を介さずにコードを機械的に検査するという意味では断じてない。
  • 信頼の連鎖はポジティブなものだけでなく、たとえば「私が信頼している人が信頼しないことにした人によるレビュー」は、信頼度が低いものとして結果の算出に利用される。 そういった意味でも、何を信頼・信用するかをユーザ個々人に合わせて決めることができる。
• 自分のコードレビュー結果を、偽造・改竄される心配なく、手軽に、かつ利用しやすい形で、広く公開できる。
  • 信頼されるレビューの積み重ねが、さらにレビュアーの信頼へと繋がっていくことで、レビュアー自身のブランディングとしても機能する。 逆に、信頼できないレビューや雑なレビューが多いと、もちろん人々からは信頼されなくなっていく。

何をレビューするか

最初に考えるのがこれである。 もしあなたが何か既にコードを書いたことがあるなら、そのコードが依存しているライブラリを (間接的な依存も含めて) 列挙し、その中から小さなプロジェクトでまだレビューされていない (あるいはレビューの少ない) ものを適当に選んでみるのが良い。 何を選んでも良いのだが、いきなり大きなコードと向き合うよりは、小さなもので慣らしていくのが良いだろう。

もし自分が貢献したことのある他人のプロジェクトがあるなら、それをレビューするのも良いだろう。 場合によっては自分が書いた以外の部分まで目を通したこともあるだろうから、コードへの理解は初めて読む人よりは進んでいるはずである。 使ったことのある機能や弄ったことのある部分を端緒として理解を広げていくと、どこから始めるか考える手間が省ける分やりやすいかもしれない。

貢献したことがなくとも、 GitHub で star したことがあるプロジェクトやその依存についてチェックしていくのも良い。 大事なのは、完全に利他的なボランティアとしてやるのではなく、「将来の自分がより安全なコードを使うための投資」として自分の利益を念頭にコードを読むことである。 レビューが集まった結果として利用者全体を利することはあるだろうが、開発にせよレビューにせよバグ報告にせよ、自分自身の利益を度外視した活動は持続しないものである。 せっかく関わるなら、自分に縁のありそうなものから選ぶのがモチベーションも保ちやすくて良いだろう。

悪意あるコード片はあるか？

レビューするうえで、これがまず、何よりも見落としたくないものである。 通常のバグであれば発現は条件次第だろうし、場合によってはユーザ側での回避策もとれる。 また、必ずしも実害が出るとも限らない、ガチャのようなものである。

しかし、悪意あるコードは過去・未来に渡るプロジェクト自体の信用を疑わせるものであり、実害の発生した可能性は極めて高く、場合によっては既に実行された結果起きたとについても調査が必要になるかもしれない。 ゆえに、この観点で疑わしいことを放置しては、コードの信用を論ずることはできず、そもそもコードレビューの意味がない。

以下で挙げる観点は悪意の発見のみに使うものではないが、資するところは大きいであろうと思われるものである。

コードの品質は良いか？

軽微な機能向上

• Preallocate storage using iterator's size hint by lo48576 · Pull Request #8 · Robbepop/string-interner
  • イテレータからコンテナに値を入れるとき、事前に長さが判明していたら事前にメモリを確保できるというだけの話。
• Implement `std::iter::Extend` for `StringInterner` by lo48576 · Pull Request #13 · Robbepop/string-interner
  • FromIterator が実装されているのだから、 std::iter::Extend も実装されているのが自然だよね、というだけの話。

説明するほどのことでもない簡単なものである。 前者の size hint を使えそうだなというのは、まあ眺めてると浮かび上がってきたやつで、慣れである。 後者の Extend が欲しいというのは、前者の変更の直後にあったループで「この形は頻出しそう」という気持ちになったとき、コンテナにまとめて要素を追加する一般的な API が存在することを思い出したので書いたもの。 いずれも、このライブラリについて大して知らずとも思い付くことはできるだろうが、私の場合はこのライブラリを使ったことがあったので StringInterner がコンテナとして振る舞うという直観を既に持っており、ひらめきが近かったというのはあるだろう。

こんなものはちょっと書いてやれば済む話であって、さっさとパッチを投げてしまうのが一番手っ取り早い。 こうして改善しておけば、得をするのは次のリリースを自分のプロジェクトで使う自分自身ということになるので、躊躇う理由はない。

unsafe と向き合う

この crate は unsafe なコードを含んでおり、これはパフォーマンス関係で割と重要な要素でもあった (具体的には、メモリ消費量を半分近くにできる)。 よって、 unsafe を完全に排除するのではなく、安全性を説明し確信する方向で読んでいくことにした。

          fn from_usize(val: usize) -> Self {
    assert!(val < u32::MAX as usize);
    Sym(unsafe { NonZeroU32::new_unchecked((val + 1) as u32) })
}
        

          fn from_usize(val: usize) -> Self {
    assert!(
        val < u32::MAX as usize,
        "Symbol value {} is too large and not supported by `string_interner::Sym` type",
        val
    );
    Sym(NonZeroU32::new((val + 1) as u32).unwrap_or_else(|| {
        unreachable!("Should never fail because `val + 1` is nonzero and `<= u32::MAX`")
    }))
}
        

だから何なのか

まあここ数日の進捗みたいなものを雑に書いただけなので、だから何と言われても教訓を引き出すようなことでもないが。 コードレビューについてはえらくいろいろ考えている風なことを書いている割には、実際やっていることが「眺めてたら光って見える」みたいな雑なアレであるということを察してもらえれば、日記じみたものを書いた甲斐があったというものである。

crev で試みているコードレビューの在り方とは、 OSS の文脈でよく言われる「十分な目の数があれば〜」と似たようなもので、数がそれなりに大事になってくる。 品質も高いに越したことはないが、そこは信頼の連鎖でうまく計算を調整することで、多少品質の低いレビューがいくらかあってもどうにかなる。 よって、無理せず手の届く範囲でやるのがまず大事ということである。

自動化は推奨されない

crev では、コードレビューそのものの自動化、メトリクスなどによる客観化、天下り的な仕組みなどは推奨されていない ^[12] 。 コードの品質や信用などは主観的なものであって、ゆえにそれらはあくまで人間の目と各自の判断によって決定されるべきという立場である。

そのようなわけで、「lint を通らなかったから OK」とか「メトリクスがいい感じだったので OK」とかそういうレビューはしない方がいい。 (そんな評価はそれこそツールですれば済む話で、数を集める必要はない。) というか、しているとレビュアー自身が信用されなくなっていくと思われる。

カッチリ決められていない基準

今のところレビューに記述できる情報はかなり大雑把で (まあ細かく決めても使い道に困るし無駄に複雑になるだけなのでこれで良いと思うが)、大概の細かい情報は comment セクションに書くということになっている。

それ自体はまあ良いのだが、「場合によってはまずいバグがあるけど大抵のユーザとユースケースでは問題なさそう」のような状況で、 rating として neutral ("secure but with flaws") を選ぶか negative ("severe flaws and not ok for production usage") を選ぶかで悩む。 「妙な使い方をするとやばいことになるバグはあるが、踏まなければ問題なく動く」というのは未定義動作に近いものを感じるので、とりあえず厳し目の評価をしておくべきだろうか……

ユーザ増えてくれ

最近 (に限ったことでもないが) の Rust 界隈だと unsafe に無頓着なの怖いし可能なら避けようよ的な話 がちょくちょく盛り上がり、また unsafe を気にするツールや crate なども作られており、 unsafe の危険性をどうにかしていく流れが感じられる。 こういった状況で、コンパイラが機械的に検査できない部分を確認する人間の目の数が増えるというのは大変心強いものであり、まあ早い話が慣れてきた人はどんどんレビューしてみてほしいという話だ。