2015-11-03

Linus Torvals、クソコードにブチギレ

Linux-Kernel Archive: Re: [GIT] Networking

Linus TorvalsがGCCの独自拡張を使った整数演算のオーバーフロー検知コードがあまりにクソすぎるためにブチギレしている。

On Wed, Oct 28, 2015 at 3:32 PM, David Miller <davem@xxxxxxxxxxxxx> wrote:

リリースサイクルのこの後半に入れるのはちょっと怖いと思われるかもしれないが、小規模なドライバーの修正をあちこちに施しただけだよ。

マジかよテメーら、こりゃクソだ。

コンフリクトはGCCの新しいクソヘッダーファイルのせいなんだが、俺がブチギレてるのはそこじゃなくてこいつがクソなせいだ。

net/ipv6/ip6_output.cの以前のコードはこれだ。

mtu -= hlen + sizeof(struct frag_hdr);

そして、こいつが新しい、「改良された」コードだ。最近流行りのコンパイラーマジックな組み込み機能を使っていて、その機能がない場合に備えて泥臭いラッパー関数を使ってる。

if (overflow_usub(mtu, hlen + sizeof(struct frag_hdr), &mtu) ||
mtu <= 7)
goto fail_toobig;

この上記のコードに対して、

(a) 読みやすい

(b) 効率的(しかもコンパイラーマジックまでサポートしてやがるぜ)

(c) とても安全

なんて考える奴は無能だしこの場にいる資格すらねぇ。

上記のコードはクソだ。しかもクソコードを生成しやがる。見た目がクソでしかも不必要なクソだ。

このコードは、簡単に、たった一つの、わかりやすい条件分岐だけで書けるし、しかもコンパイラーはもっとマシなコードを吐き出す上に、コードは見た目にマシでわかりやすい。なんでこうしないんだ。

if (mtu < hlen + sizeof(struct frag_hdr) + 8)
goto fail_toobig;
mtu -= hlen + sizeof(struct frag_hdr);

これは同じ行数だし、何をするか誰も知らんキチガイみたいなヘルパー関数使う必要がないし、実際に何をするか明白だ。

こっちの明白版の方が読むのも簡単だし理解するのも簡単だ。反論はあるか?

いやマジで、なんで2つも条件分岐を使うマヌケなコードと、ピカピカの真新しいコンパイラーサポートを必要とする非標準の関数を使って、まともに見えないコードでクソコードを吐かせる必要があるのか、誰か教えてくれよ。

他では使う必要がないピッカピカのこの関数は、単にコンパイラーがシコってるだけだろ。

ああ、それから、"hlen + xyz"という式がオーバーフローしたら、やっぱりオーバーフローは起こるだろ。だが、"overflow_usub()"コードにも同じ問題がある。というわけでその心配をしているのならば、そもそも最初から間違ってるってこった。

この手の完全にアホくさいクソが存在する理由がわからん。

いや、この完全にキチガイな上にrc7というこの時期にコンフリクトを起こすようなものを俺がpullするわけがねーし、読めないほどクソになる理由すらないんだが、どういうこった?

このコードは、新しい"overflow_usub()"コードを使いたいために設計されたかのように思える。関数を使う言い訳としてな。

頭イカれてる言い訳にすらならんぞ。

申し訳ございませんが、こちらと致しましてもこのようなよろしくないコードのために好ましくないインターフェースを追加するわけにはまいりません。

まあ、これがネットワークレイヤーの仲にいたならば、俺も気が付かなかっただろうよ。だが俺が気づいたからには、こいつは絶対pullしねぇ。いいかお前らみんなに知っていてもらいたいんだが、こういうコードは絶対受け付ける気がない。こういうコードが最近流行りのオーバーフロー検出を使っているから「安全」だとか「セキュア」だとか考える奴は、場違いにも程があるし、冗談にしても笑えねぇ。

このクソが成し遂げた唯一のことは、まともな人間が読めないコードを作り出しただけだ。

取り除け。俺はこんなクソをもう二度と見たくねぇ。

Linus。

5 comments:

  1. ネットワークレイヤーの仲

    ネットワークレイヤーの中

    # Linus 発言の翻訳、いつも楽しみにしています

    ReplyDelete
  2. 「俺がブチギレてるのはそこじゃなくてこいつがクソなせいだ」が自分には「俺がブチギレてるのはそこじゃなくてこのクソが完全にインチキな理由で書かれたからだ」じゃないかと思えるのですがいかがでしょう。

    ReplyDelete
  3. 少なくともoverflow_xxxを使ってる方はこのマクロをネストすればオーバーフローを完全に検知できるけど、Linusのは結局オーバーフローしちゃうのでは?

    後、カンマ演算子使わなければ最初の奴の方が遥かに意味は分かりやすいです。
    Linusのはオーバーフローする事に気付いてるのか気付いていないのか(要件としてオーバフローを認めているのか)コメント無しでは分かりません。

    まぁ元のコードは不完全なのは確かですね。

    ReplyDelete
  4. 出てくる変数のtypeはunsigned intで32か64かどちらにしても、hlen + sizeof(struct frag_hdr) + 8 < 2^32 を想定しているならばいいか。
    ip6_fragment()内の処理で、パケットサイズの大きさ程度だろうから。

    ReplyDelete
  5. カンマ演算子なんて使ってる?

    ReplyDelete

You can use some HTML elements, such as <b>, <i>, <a>, also, some characters need to be entity referenced such as <, > and & Your comment may need to be confirmed by blog author. Your comment will be published under GFDL 1.3 or later license with no Invariant Sections, no Front-Cover Texts, and no Back-Cover Texts.