早期リターンとApple SLLバグ

2021.04.26

早期リターン（アーリーリターン）とは、リダーブルコードやその他の本で推奨されている書き方です。ガード節でnullチェックや範囲チェックして、想定外の値のときは、すぐにリターンします。else で続けません。

function foo()
{
    if ($a == null) {
        return 何か;
    }

    メインロジック
    return 何か;
}Code language: plaintext (plaintext)

早期リターンではない書き方です。elseで続けます。

function foo()
{
    if ($a == null) {
        return 何か;
    } else {
        メインロジック
        return 何か;
    }
}Code language: plaintext (plaintext)

入り口は一つ、出口は一つと習っていると、elseで書きがちですが、ガード節と早期リターンの組み合わせは確かに読みやすいと思います。

さらに、phpmdという静的解析ツールは「elseを使わないように」と警告してきます！

基本的にif文にelseは必要ないね。ifの条件を書き直してelseをなくそう、読みやすくなるよ。そのためには、アーリーリターンを使用すること。でも、コードを小さなメソッドに分けなければいけないだろうね。単純な割り当てなら、三項演算子を使うことができるよ。

else禁止、まじで？

いくらなんでもそれはないわ、何か意図があるはずよ

すると、phpmdの「elseを使わないように」警告は、オブジェクト指向エクササイズが由来ということがわかりました。

オブジェクト指向エクササイズは、ThoughtWorksアンソロジーという本（マーチンファウラーも著者の一人）の第5章で、Jeff Bay氏が提唱している９つのルールです。

本の「else禁止」の該当箇所を読むと、

if 〜 elseのネストはやめて、簡潔にしよう。
ネストが深くて追跡できない条件文やスクロールしなければ読めないcase文にはうんざりだよ。

else句を使わずに書き直す方法を考えてみよう、else禁止はいい練習になるよ。

早期リターン。（早期リターンは使いすぎるとわかりにくくなってしまうから注意してね）

三項演算子。

ポリモーフィズム。StrategyパターンやNullObjectパターンも試してみて。

if〜else〜以外の新しい方法を身に着けよう、そのために強制的にelse句禁止で書いたらどうかな、ということです。

「何がなんでもelse禁止、早期リターンしなさい」とは言っていないですよね。

特に注目したいのは、次の一文です。ぼくが書き足したのではなく、本に書いてあります。

早期リターンは使いすぎるとわかりにくくなってしまうから注意してね

早期リターンを使いすぎると、本来のロジックとは違うものになってしまう可能性があります。

次のような if〜elseif〜があります。

if (条件1) {
    何か処理
} elseif (条件2) {
    何か処理
} else {
    何か処理
}Code language: plaintext (plaintext)

早期リターンを使って、次のように書きかえます。

if (条件1) {
    何か処理
    return;
}

if (条件2) {
    何か処理
    return;
}

何か処理
return;Code language: plaintext (plaintext)

読みやすくなった気がするけど、何が悪いのかな？

将来、別の誰かが、if と if の間に、別の処理を書けてしまいます！

if (条件1) {
    何か処理
    return;
}

★新しい処理★

if (条件2) {
    何か処理
    return;
}

★新しい処理★

何か処理
return;Code language: plaintext (plaintext)

条件2が成立するとき、どのような処理をするのか、わかりにくくなってしまいました。

このように一つの if 〜 elseif と書くべきところを、複数のif で書いてしまったために起きた重大なバグがありました。

AppleのSSLバグ（CVE-2014-1266）です。gotoバグとして有名であり、wikipediaの「到達不能コード」に例として紹介されています。

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    OSStatus        err;
    ...
 
    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
        goto fail;
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
        goto fail;
        goto fail; // 常に実行される
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) // このif文が実行されることはない
        goto fail;
    ...
 
fail:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;
}

Code language: plaintext (plaintext)

波カッコをつけていたらどうでしょうか。

    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        goto fail;
        goto fail;
    }Code language: plaintext (plaintext)

2つめのgoto failは実行されることはなく、バグを防げました。

次のように波カッコをつけていたらどうでしょうか。

    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        goto fail;
    }
        goto fail;
Code language: plaintext (plaintext)

2つめのgoto failが変だと気づき、削除していれば、バグを防げました。

しかし、インデントが変と思って、インデントを修正して、残してしまうかもしれません。この場合は、バグを防げません。

    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        goto fail;
    }
    goto fail;
Code language: plaintext (plaintext)

つまり、波カッコをつけると、このバグを防ぐ可能性は高いですが、波カッコをつけただけでは、完全に防ぐことはありません。

if 〜 elseif で続けるとどうでしょうか。

    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) {
        goto fail;
    } else if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        goto fail;
    } else if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
        goto fail;
    }Code language: plaintext (plaintext)

2つめの goto fail の入り込むすきがありません。


（ロジックの流れだけから判断するなら）このバグは、gotoが原因というよりは、
（１）波カッコをつけていなかった
（２）「if 〜 elseif」ではなく、「複数の if」で書いていた
の２点が原因だと考えます。

話しがずれますが、このコードをよく見ると、メインの処理は複数分岐ではなく、逐次処理であることがわかります。

    SSLHashSHA1.update(&hashCtx, &serverRandom);
    SSLHashSHA1.update(&hashCtx, &signedParams);
    SSLHashSHA1.final(&hashCtx, &hashOut);
Code language: plaintext (plaintext)

このメイン処理にエラー処理を追加するとしたら、if 〜 elseif ではなく、複数のif で書いたとしてもしかたがないかなと思います。

話しを戻すと、

「if〜elseif」はswitch文に近い複数分岐、「複数のif」は逐次処理、意味が違います。

「複数の if」で書いてもさ、「if 〜 elseif」と同じ動きしているなら、読みやすいほうがいいんじゃないの？

同じ動きをしているのは、同じ動きをするように書いているからです。しかし「複数の if」はたった1行挿入しただけで、「if 〜 elseif」とは違う動きになってしまいます。

リーダブルコードで推奨しているのは、ガード節と早期リターンです。if〜elseifで書くべきところを早期リターンで書いてしまうのは、早期リターンを拡大解釈しすぎだと思います。

if〜elseif で書くべきところを早期リターンのほうがいいかなと思ったら、Apple SSLバグを思い出してください。

多くの人がリーダブルコードを読んでいるのはいいことだけど、経験者まで拡大解釈しているのは困ったことだわ

タイトルとURLをコピーしました