0
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

CWE-457とCWE-665を混同してレビューを通してしまった話

0
Posted at

CWE-457とCWE-665を混同してレビューを通してしまった話

C言語のコードレビューで「未初期化」バグを指摘するとき、CWE-457とCWE-665を同じものとして扱っていないだろうか。自分は去年まで両者をほぼ同一視していて、レビューコメントで誤爆したことがある。

この記事は、組み込み系やレガシーC資産のレビューを担当していて、cppcheckやclang-tidyの警告を「なんとなく」握り潰したり通したりしている人に向けて書いている。CWEの番号を正確に使い分けられると、レビューの説得力と検出精度が両方上がるという実感がある。

先に結論を書く。CWE-457は「初期化されていない変数の使用」で、変数が宣言された後に一度も値を設定されないまま読まれるケースを指す。CWE-665は「不適切な初期化」で、初期化そのものは行われているが、その初期化処理が状況によって不十分・不完全になるケースを指す。この2つは発生条件も検出方法も違うので、同じ「初期化漏れ」として片付けると見落としが生まれる。

なぜこの2つを混同するのか

自分がこの2つを混同していた理由は、どちらも「値が入っていない状態で使われる」という結果だけを見ていたからだ。原因側を見ていなかった。

CWE-457は静的解析ツールが得意な領域だ。変数の宣言から使用までのデータフローを追えば、どの経路でも代入されていない箇所は機械的に検出できる。cppcheckでもclang-tidyでも、単純なローカル変数の未初期化はほぼ拾ってくれる。

問題はCWE-665の方だ。こちらは「初期化コードは存在する」のが厄介なところで、条件分岐やエラーハンドリングの抜けによって、特定の実行パスでだけ初期化が飛ばされる。ツールがフラグを立てても、それが「そもそも初期化コードがない」CWE-457なのか、「初期化コードはあるが条件によって通らない」CWE-665なのかを、警告メッセージだけで判断するのは難しい。

自分は以前、あるプロジェクトで「未初期化変数あり」という警告を全部CWE-457として扱い、変数宣言時にゼロ初期化を機械的に追加するパッチを大量に書いたことがある。そのうち3割くらいは実はCWE-665で、初期化漏れの根本原因(エラー処理の分岐で早期returnしていて、後続の初期化コードに到達しない)が残ったままだった。表面上は警告が消えるので、レビューでは一度通ってしまった。

解決手順:分類してから対処する

ステップ1:警告が出た変数の初期化コードの有無を確認する

まず、その変数に対して初期化を意図したコードが存在するかどうかを見る。存在しなければCWE-457、存在すればCWE-665の可能性を疑う。

ここで注意したいのは、「初期化っぽい行がある」ことと「実際にその行を通る」ことは別問題だという点だ。ソース上にret = 0;という行があっても、その行に到達しない実行パスがあればCWE-665になる。

ステップ2:実行パスを手で追う

静的解析ツールの警告だけで判断せず、実際にその変数が使われるまでの実行パスを最低2〜3パターン書き出す。自分の経験では、以下のパターンで見落としが多い。

  • エラー処理での早期return(goto含む)が、初期化コードより前にある
  • switch文でdefaultケースがなく、想定外の値が来たときに初期化がスキップされる
  • 構造体の一部メンバだけを初期化していて、残りのメンバが未初期化のまま使われる

3つ目のパターンは特に見落としやすい。memsetで構造体全体をゼロクリアしたつもりが、実は構造体の途中に追加されたメンバがゼロクリアの範囲外になっていた、という経験が自分にはある。構造体定義とmemsetの範囲を毎回突き合わせる作業は、正直に言って地味に時間がかかる。

ステップ3:CWE番号をレビューコメントに明記する

レビューコメントを書くときは「未初期化っぽい」ではなく、CWE-457かCWE-665かを明記する。理由は、修正方針が変わるからだ。

CWE-457の修正は基本的に宣言時の初期化かデフォルト値の付与で済む。CWE-665の修正は、初期化処理そのものではなく、そこに到達しない実行パスの方を直す必要がある。ここを混同すると、CWE-665の変数に対して宣言時初期化だけ入れて「警告が消えたから直った」と誤認するケースが起きる。実際の不具合(特定条件下で初期化ロジックが動かない)は温存されたままになる。

コード例

以下は自分がよく見かけるCWE-665のパターンを単純化したものだ。

int process_data(int mode, int *result) {
    int status = -1;

    if (mode == MODE_A) {
        status = do_process_a(result);
    } else if (mode == MODE_B) {
        status = do_process_b(result);
    }
    /* MODE_C が来た場合、status も result も初期化されない */

    if (status == 0) {
        use_result(*result);
    }

    return status;
}

status-1で初期化されているのでCWE-457には該当しない。しかしresultはポインタの先の値が、MODE_AでもMODE_Bでもないmodeが来たときに未初期化のままuse_resultに渡る。これはCWE-665、初期化処理はあるが条件によって不十分になるパターンだ。

対処としては、else節で明示的にデフォルトを設定するか、想定外のmodeをエラーとして早期returnする。

int process_data(int mode, int *result) {
    int status = -1;

    if (mode == MODE_A) {
        status = do_process_a(result);
    } else if (mode == MODE_B) {
        status = do_process_b(result);
    } else {
        return -1; /* 想定外modeは明示的にエラー */
    }

    if (status == 0) {
        use_result(*result);
    }

    return status;
}

この修正で重要なのは、警告を消すことではなく、実行パスの穴を塞ぐことだ。cppcheckの警告表示上は同じ「未初期化の可能性」でも、直す場所がまったく違う。

この分類作業を毎回手でやるのはそこそこしんどくて、自分は最終的にCWE-457とCWE-665を区別して検出するツールを自作した(CSAF)。市販だとCoverityやKlocworkあたりが有名だが、ライセンス費用が個人や小規模チームには重い。自分は自分の手元のレビュー作業を楽にする用途で作った。

落とし穴と対処

一番踏んだ落とし穴は、警告数を減らすこと自体を目的にしてしまったことだ。CWE-665の変数にとりあえず宣言時初期化を入れると、警告は消えるがロジックの穴は残る。レビューで「警告ゼロになったのでLGTM」を出してしまい、後日別の担当者が同じ箇所で本番に近い環境でのテスト中に不具合を踏んだ、という経緯があった(NDAの関係で案件の詳細は書けないが、初期化まわりの話としてはよくある部類だと思う)。

もう一つの落とし穴は、構造体メンバの部分初期化だ。memset(&s, 0, sizeof(s))を構造体定義の変更前に書いていて、後からメンバを追加したときにmemsetの範囲を見直さなかったケースがあった。sizeofを使っていれば範囲自体は自動的に追従するはずなのだが、コピー元とコピー先で型が違っていて実質的に範囲がずれていた、というオチだった。これはCWE-665というよりコードの保守性の問題だが、根っこは同じで「初期化コードがあるから安全」という思い込みが原因になっている。

対処として自分がやっているのは、レビュー時に「この変数、初期化コードがあるか」だけでなく「その初期化コードを通らない実行パスがあるか」を必ずセットで聞くことだ。この一言を挟むだけで、CWE-665由来の見落としはかなり減った実感がある。

関連ツール

CWE-457とCWE-665の取り違えは、自分の観測範囲では組み込み・車載・医療系のレビューで特に頻発している印象がある。この2つを含めてCWE-467(sizeof on pointer)やCWE-476(NULLポインタ参照)まで機械的に区別して検出するツールとして、CSAFを作った。完全ローカルで動くので、社外に出せないコードベースでも使える。

CSAF

まとめ

CWE-457は初期化コードそのものが存在しないケース、CWE-665は初期化コードはあるが実行パスによって機能しないケースだ。この違いを意識するだけで、レビューコメントの精度も修正方針の的確さも変わってくる。

警告が消えたかどうかではなく、実行パスの穴が塞がったかどうかで判断する習慣をつけると、初期化まわりの不具合はかなり減らせるという実感がある。


著者:ぽん(@pon_freelance
C言語実務23年、組み込み/制御系。
副業で技術記事販売と自作ツール販売をやっている。

書いているもの:

  • CSAF - C言語の業務・監査レベル安全性監査フレームワーク(Booth ¥4,980)
    (その他:(なし))
0
0
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
0
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?