「PHP 5.3.7に重大なバグ、strncatの罠にはまる」記事へのコメント

strncatの罠？

[Anonymous Coward]

リンクの解説読みましたけど、問題は

strncat(passwd, "$", 1);
を
strlcat(passwd, "$", 1);
に、変更したことのようですから、正確には「strlcatの罠」じゃないですかね？

strcat→strncatの時点ではバグってないわけで。

Re:

[ikotom (20155)]

まあ仰ることも間違いではないのですが、C言語界の背景というか
strncatのsize指定が「意外性最小原則にそぐわない」仕様だということで
数々のバグの発生源となっており、散々非難されてきた、という今までの経緯があるのです。

で、srtncat使用者はそこを注意して使用してたけど、後からそれをstrlcatに置き換えた人は
そんな罠があったと知らず、何も考えずそのまま関数だけ置き換えてしまったと。

ある意味、strncatというのは関数自体がバッドノウハウ的な存在なのですね。
まさにバッドノウハウ的対応が、たとえその場は良くても将来に問題を引き起こすという好例となってしまったということでしょう。

Re:strncatの罠？

[Anonymous Coward]

strncat の第三引き数は size じゃないぞー（型は size_t だったりするかもしれないが)。
第三引き数が size なのは strlcpy の方だぞ。

引き数の意味を間違えて「意外性」どうのこうのとか、知ったかぶりするのはやめて欲しい。
本人は悪意がないのかもしれないけど、まだぞろ誤解して、strcat() 使うなとか、
strncat() 使うなとか、頭の悪いコード規約作ろうとうするやつが多発するから。

今回のも罠でも何でもなくて strcat() で十分なのに、迷信に従って strncpy(), strlcpy() とかに書き換えた馬鹿がいるだけじゃないか。

＃ *passwd = '$'; passwd[1] = 0; <--- 本当にやりたっかったこと

Re:strncatの罠？

[ikotom (20155)]

>strncat の第三引き数は size じゃないぞー（型は size_t だったりするかもしれないが)。
ああ、ごめんなさい。あまり気にしないで書いてしまいましたが
第三引数が「連結する文字数」なので size でなく length OR count だとかいう意味のご指摘ですよね？

その辺にこだわる方もいらっしゃいますが私は型がsize_tなら何であれサイズと表しても
間違いではないと思う派です。
バッファサイズ、バイトサイズ、文字列サイズ。ということで。

C言語で size_t が出てくる時、いつもこのうちのどれなのかで悩むので
静的型付け大好き人間としては buf_size_t とか length_t とか標準で作って欲しいと密かに思っています。

Re:strncatの罠？

[Anonymous Coward]

> *passwd = '$'; passwd[1] = 0; <--- 本当にやりたっかったこと

蛇足の部分でミスった。

  passwd_end = passwd + strlen(passwd);
  *passwd_end = '$'; passwd_end[1] = 0;

と書かないと意味が通じないか。

Re:strncatの罠？

[Anonymous Coward]

そして、そんな些細なミスを誘発するような再発明をするくらいならstrcat(passwd, "$");でいいじゃん、と言うことで、振り出しに戻る。

Re:

[Anonymous Coward]

>そして、そんな些細なミスを誘発するような再発明をするくらいなら
>strcat(passwd, "$");でいいじゃん、と言うことで、振り出しに戻る。

戻りませんよ。

プログラムに限らず、人間の行う論理表現では
「明示的なある」を示せても「暗黙のない」は示せません
(コメントなど含め、また「”ない”と書いて”ある”」まで含まれます)。

ここで、strcatでは
「バッファのサイズ溢れとか危険だけどその意識は”ある”のか”ない”のか」
を示すことができません。
ですので、そこで人間が明示的に「(意識は)ある」を保証するためには、
適切なコード(strcatからstrncatにす

Re:

[Anonymous Coward]

でもstrncat(passwd, "$", 1);には「私はバカです」の表明以外の意味はないよね。
少なくとも溢れないようにしたいですとか溢れなくしましたという意思は一切表現されてない。
意識があろうがなかろうが、バッファ溢れするコードは溢れるし、溢れないコードは溢れません。
こんな簡単なケースなら機械的に判断できるはずだし、静的解析を謳うんならそれくらい解析して欲しいものです。

Re:

[Anonymous Coward]

いやいや、蛇足に対するネタですよ。

ところで、(#2008738)や(#2008747)のACとは別の方ですよね。それらのコメントではstrcat()でいいよという話ですが、話が全く変わってます。

そもそも、文字列定数を連結するのに、strcat()の代わりにstrncat()を使うのは、引数の計算・入力ミスよる危険性が加わるだけで良いことがありません。

また、strcat()を使っているからと言って、文字列長を意識していないと言うことはないと思います。それが意識されていないようでは、コード全体を信用できません。

Re:

[Anonymous Coward]

このタレコミについてるコメント全体に言えることだけど、
strncat(passwd, "$", 1);
自体は全く問題ない。
strlcat(passwd, "$", 1);
が間違ってるだけ。

そもそも全体テストが通らない状態でstrcatをstrncatに書き換える意義は殆ど無いんだが、strncatに変えた事自体は問題なかった。
その後、一番の大馬鹿者がstrncatをstrlcatに差し替えたのが問題だった。

strlcatの存在を知っている奴がなぜstrncatとstrlcatでは動作が違い引数も違うということを意識しなかったのか…理解に苦しむ。

# くらいの事情は編集追記の「徳丸浩氏がブログにて詳細に解説している [tokumaru.org]」を読めば理解できるぞ

Re:

[Anonymous Coward]

一番の大馬鹿は、そのエンバグに対してfailしている単体テストを無視したことです。
何も考えずにstrncatをstrlcatに単純に書き換えてしまう凡ミスはむしろ当然に有り得るヒューマンエラーです。理解に苦しむとか言ってるあなたもきっと同レベルのことをやらかしますよ。
自分はやらかさないとか言う人が単体テスト無視するんですよ……。

Re:

[Anonymous Coward]

＞テストを無視
通らないテストがゴロゴロいる上にその管理(XFAIL化して現時点で通らないことを明示する作業)もできていないプロジェクト(#2008727 [srad.jp])だから、そこはもう手遅れですよ。
腐りかけのプロジェクトでテスト工程が腐るのは問題ではありますが、理解は出来ます。

が、標準化されているstrncatをわざわざ非標準であるstrlcatに置き換えた人間は、strncatよりstrlcatの方が適した場面が存在することや、引数の意味が違うことも理解しているはずです。
そうでなければ置き換える理由がありません。
にもかかわらず、strlcatの使い方を間違えているものだから理解出来ないと言っているのです。

# でもコミットログ見るとstrcat→strncatの [php.net]

Re:

[Anonymous Coward]

> strncat(passwd, "$", 1);
> 自体は全く問題ない。
大問題です。そういう動けばいい、アラートが出なければいい、というコーディングがすべての元凶です。

Re:

[Anonymous Coward]

>大問題です。
>そういう動けばいい、アラートが出なければいい、
>というコーディングがすべての元凶です。

「動けばいい」のはstrcatのほうですよ。
それが理解できませんか？

記述をより明確で確実なものにしていく、という観点で
strcatをstrncatにしたことは確実に意味と価値があります。
これが理解できないのであれば、
申し訳ありませんが他人とのコミュニケーションを数年経験してみてはどうでしょうか。

Re:

[Anonymous Coward]

本気で言っているのなら、あなたはgreentea氏以下の最悪のバカだとしかいいようがありません。

> strcatをstrncatにしたことは確実に意味と価値があります。

strncatにするときに第三引数をきちんと計算して渡せば、それは大いに意味があります。
でも、今回は違うでしょ？

Re:

[Anonymous Coward]

このstrncatはpasswdの残り容量を全く気にすることなく末尾に"$"(の先頭1文字(笑))を追加してするだけです。
passwdはstaticな配列なので、sizeof(passwd)-strlen(passwd)で残り容量を計算してやればマシになります(静的解析ツールの警告とやらもそういう意図のものでしょう)。このケースでは計算するまでもなく安全ですけれど、それはstrcatでも同じです。
こういう間違ってるのか、一見間違えているように見えるけど意図通りなのか判然としないコードは明確でも確実でもありません。むしろ後でそのコード読んで修正しようとする人に地雷を埋めているだけです。今回は自分でその地雷踏んでしまったようですが。
これが理解できないのであれば、
申し訳ありませんが他人とのコミュニケーションしながらコード書く実務を数年経験してみてはどうでしょうか。

Re:

[Anonymous Coward]

＞このstrncatはpasswdの残り容量を全く気にすることなく
strncatの引数をバッファ残量と解釈するか、そのstrncat呼び出しで追加するフィールドの最大長と解釈するかの問題ですね。
バッファ長の管理を意図したならstrlcatのような仕様になるのは明らかなので、strncatの引数は追記するフィールド長の限界を示す意味合いのほうが強いってのはよくよく考えれば判るとは思いますが。

フィールド長を管理する思想ならば、可変長文字列の追記時にはstrncatで余裕を確保して固定長文字列の追加はstrcatで済ます事になりますが、全てstrncatで処理しても(文字列長の手計算が)冗長なだけで問題

Re:

[Anonymous Coward]

まだわかってないバカがいるよ。

Re:

[Anonymous Coward]

＞まだわかってないバカ
「,1」
より
「,sizeof(passwd)-strlen(passwd)」
の方がはるかに危険な地雷って理解できないのか…。
今回はともかく全般的に言って、設計思想的にも踏みやすさ的にも爆発再現性的にも盲目的なsizeof-strlenは危ないんだけど。
strncatにはsizeof-strlenって何処かで聞いて思考停止しちゃったのカナ？

そもそも「strlcatとstrncatの引数の意味を理解していなかった」のが地雷なのであって、「,1」が地雷だったわけじゃないし。

Re:

[Anonymous Coward]

本当にプログラマにはバカしかいないことが良く分かるな。
「,sizeof(passwd)-strlen(passwd)」は何がやりたいかよく分かるし、間違ってることも一目瞭然。「記述をより明確で確実なものにしていく」ってのはこういうことを言うんだよ。
「,1」は何がやりたいのか分からないし、もしかしたら合ってるかもしれないけど、ホントにそれが書きたかったことなのかどうか分からないヒドいコードなの。確かなのは、こいつの書いたコードはここだけじゃなくて全部気をつけた方がいいってことだけ。

Re:

[Anonymous Coward]

わかっていないのはもう君しかいないようだから説明しないけど(アホのgreenteaですら理解して黙ったようだし)、

バーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーーカ

> 「,1」が地雷だったわけじゃないし。

普通に考えればたしかに踏む奴もアホですが、踏んだ後では紛れもない地雷でした。しかも自爆なのでなんの言い訳もできませんね。

Re:

[Anonymous Coward]

＞記述をより明確で確実なものにしていく
「strncat(A,B,sizeof(A)-strlen(A))」なんて冗長なテンプレが明瞭だなんてよく言えたもんだ。
sizeof-strlenは「溢れたら切り捨てる(文字列を黙って破損させる)」という記述であり、バッファ長の正確な見積もりをしていないって自己申告なんだよ。
バッファ長のことを何も考えてない奴がstrncatで定数を使うなんて余程のことがない限り起きないし、「,1」の意図は明らか。
＞「,1」は何がやりたいのか分からないし
盲目的に「,1」としただけならかなりの阿呆だし論外だが、君はその論外の方だと自己申告しているのかい？

＞確かなのは、こ

Re:

[Anonymous Coward]

ACは同一人物ではないから、技術的に何も知らないのにただ人をバカにしたい奴が混じっている可能性を無視しないように。そういうのは相手にするな。

その上で、strncat(A, B, sizeof(A) - strlen(A))は意図は分かるけど誤り。strncat()の3番目の引数は文字数だから、正しくは、strncat(A, B, sizeof(A) - strlen(A) - 1)。でも、意図はstrlcat(A, B, sizeof(A))のようなことであることは想像がつく。

一方、strncat(passwd, "$", 1)は意図が判然としない。1文字だけを追加したいのか、1文字しか追加してはいけないのか、追加する文字列の文字数を入れただけなのか、分からない。strcat(passwd, "$")からの変更だから、追加する文字数を入れたのだろう。

意図を分かるように書くと、strncat(passwd, "$", strlen("$"))。だったら、strcat(passwd, "$")の方が良い。

Re:

[Anonymous Coward]

＞strncat(A, B, sizeof(A) - strlen(A))は意図は分かるけど誤り
面倒だったので気にしてませんでしたが、ゼロ終端文字列の場合確かにそうですね。
ゼロ終端しない文字列って今はどの程度使われているのやら。

＞だったら、strcat(passwd, "$")の方が良い。
strcatより、strncatに「,"$",1」や「,"$",strlen("$")」や「,"$",sizeof("$")」をつけたほうが、「バッファ長を意識はしました」って意思表示にはなるかと思います。
strlenだとその長さが定数である意思表示が若干だが曖昧になりますし、このケースでは結局一文字に過ぎないので、目算して「,"$",1」でも有りなんじゃないかと。

本気で「バッファ長を意識した」って意思表示するなら、バッファ確保の時に領域を足し合わせで表現してその定数を使うべきなんでしょうけれど、このへんは手間と意思表示効果のトレードオフなんで、まぁケースバイケースじゃないかと。

Re:

[Anonymous Coward]

余計なちゃちゃいれ

＞ *passwd_end = '$'; passwd_end[1] = 0;

char 型の配列に『数値の』0を代入ですか？

『キャラクター値である文字列終端記号』である事を示すために '\0' の方がよくありませんか？

＃同様にポインタとして NULL と書くべきところを 0 と書いているのも大量に見るな。

Re:

[Anonymous Coward]

〜の方がよい理由、〜と書くべき理由(もしくはそうする必要のない理由)をきちんと説明できますか？
# それができないくせにコードに手を入れようとするから、こういうトラブルが生まれるってことも理解してます？