delta-keeperの日記： ポインタ二重解放

今日、自分の書いたコードをテストしていたところ・・・
ちょっとまずいコードがあったので共有させて頂きたく。

(1) fpの二重解放
----
FILE *fp

if( NULL != (fp = fopen(path, "rb")) ) {
            /* ファイルの内容を読み込む */
}

fclose( fp );
----
fopenで失敗した場合、二重解放になるケース。
どっかのお兄さんがfclose()はどのルートでも実行すればいい・・・なんて言うのでやってみたら、SEGV出して死亡しました。
嘘はだめだょ(怒)

(2) opensslのEVP関数
----
EVP_CIPHER_CTX *test;

if( NULL != (test = EVP_CIPHER_CTX_new()) ) {
      /* 暗号処理 */
}

EVP_CIPHER_CTX_free( test );
----
これも同じパターン。
こっちはSIGILLになります。
デバッグ中に見つかって本当に良かった笑

さっき調べたらこんなのがあった。
https://www.jpcert.or.jp/sc-rules/c-mem31-c.html

おーこわいこわい。

# 疑似コードが腐っていたので修正しました。

エラー処理は?

[Ryo.F (3896)]

if( fopen( path, fp, "wb" ) ) {

このfopenって、ANSIのやつじゃないよね?
引数が三つあるし、初期化されてもいないし、なんら実体を指していないfpを渡して、何か意味がある?
この後、「ファイルの内容を読み込む」のに、"wb"ってのも謎だし。

ANSI的には、

FILE *fp;
if( fp = fopen( path, "rb" ) ) {
  /* ファイルの内容を読み込む */
  fclose( fp );
}

って書く場面だよね。

それに、これなら、fopenに失敗したときにfcloseすることはない。
初等教科書にも、こーゆーコードが載ってるんじゃないかな?

実用的なコードとしては、fopen できなかったときのエラー処理が無いのもおかしくね?

FILE *fp;
if( !fp = fopen( path, "wb" ) ) {
  /* エラー処理 */
} else {
  /* ファイルの内容を読み込む */
  fclose( fp );
}

エラー処理で、returnとかexitとかできるんだったら、もうちょっとすっきりしたコードになるね。

Re:エラー処理は?

[delta-keeper (31927)]

あら、なんか色々書き間違えてましたw
書いてくれたANSI準拠のコードが正解です。

Re:エラー処理は?

[ktmizugaki (46208)]

openssl の方のこのコードでは、二重解放以前に、test を初期化せずに暗号処理に使用してしまいます。

EVP_CIPHER_CTX *test;
if( EVP_CIPHER_CTX_new ) {
            /* 暗号処理 */
}
EVP_CIPHER_CTX_free( test );

ちゃんと書くと、下の感じだと思いますが、手元の環境では test が NULL でもクラッシュしないですね。(Debian9, openssl 1.1.0)

EVP_CIPHER_CTX *test;
if( (test = EVP_CIPHER_CTX_new()) != NULL ) {
            /* 暗号処理 */
}
EVP_CIPHER_CTX_free( test );

// fpの方も、二重解放というより NULL ポインター参照なのではなかろうか。

--
svn-init() {
  svnadmin create .svnrepo
  svn checkout file://$PWD/.svnrepo .
}

Re:エラー処理は?

[delta-keeper (31927)]

すいません・・・なんか疲れているようですw

ライブラリのバージョン的には同じ1.1系なんですが、クラッシュはしないけどSIGILLになりませんか?(GDB経由で実行すると出てくると思います)

私のところもDebian系のディストリなのですが、上記のコードを実行するだけでは正常終了してしまいます。

いつでもfclose

[M-FalconSky (8868)]

> どっかのお兄さんがfclose()はどのルートでも実行すればいい

あくまでfpがあるときは、漏れなくcloseしようですもんね。

# Cのポインタは、1.解放漏れ、2.2重解放(解放時の変数のNULL初期化漏れとかその判定漏れ) 3.NULLでのアクセス/クローズ の3つはチェックできてないとまずいってところかなあ...

--
M-FalconSky (暑いか寒い)

Re:いつでもfclose

[delta-keeper (31927)]

その通りです。
解放対象がNULLかどうか、コールされたAPI側で判定して欲しいです。。。

Re:いつでもfclose

[ktmizugaki (46208)]

free は NULL 渡してもいいので、つい fclose にも NULL を渡したくなってしまいますね。
EVP_CIPHER_CTX_free の方は、簡単にググった感じでは NULL を渡して良さそうです。

--
svn-init() {
  svnadmin create .svnrepo
  svn checkout file://$PWD/.svnrepo .
}

Re:いつでもfclose

[delta-keeper (31927)]

> free は NULL 渡してもいい
これですね。たぶん私にアドバイスしたお兄さんも、そのつもりで話をしたのでしょう。

Re:いつでもfclose

[Ryo.F (3896)]

んー、それはどうだろう?
クローズのときにNULLが渡されるのって、異常系が実装されていない正しくないロジックだから、と考えれば、異常終了するのが望ましい。
…と考えることもできるんじゃないかと。

どうしても、ってことなら、

fclose(fp);

を

fp && fclose(fp);

と書けば済む話だしね。

# Powershellのusing、なぜC#相当じゃないんだー!!

Re:いつでもfclose

[delta-keeper (31927)]

> クローズのときにNULLが渡される
というと、free(NULL)とfclose(NULL)で差を設けている理由はなんでしょうかね・・・

> fp && fclose(fp);
この書き方おもしろいですねｗ
ワンライナーで実行する・しないが選べるとは・・・

Re:いつでもfclose

[Ryo.F (3896)]

というと、free(NULL)とfclose(NULL)で差を設けている理由はなんでしょうかね・・・

不明としか言いようがないですね。

ただ、mallocに失敗して、NULLが返ってきて、そのエラー処理をしていなければ、freeするより前に異常終了しますよね。
NULLチェックをしているなら、正常系にのみfreeを書けば済むし。

この書き方おもしろいですねｗ

Cだと割とよくある書き方だと思いますよ。
こーゆーのを知らないと、例えば、

if(func1() || func2()) {

みたいなコードを書いたときに、func2が実行されない! みたいなことで悩むことになります。

なので、中級の教科書には載ってると思います。

Re:いつでもfclose

[delta-keeper (31927)]

> if(func1() || func2()) {

あんまり見ない書き方ですねw
関数の戻り値評価は、おおよそ一関数実行の後にすぐやるので複数条件の並ぶ
if文はそうそう出てこないです。
また、gdbを使うことが多いので戻り値は一時変数に取ることが多いです。

Re:いつでもfclose

[Ryo.F (3896)]

あんまり見ない書き方ですねw

見たことが無いのは、個人の経験なので仕方ないですね。
# 職業プログラマなら、草生やしてる場合じゃないと思いますが。

しかし、中級以上の教科書では必ず解説されているので、手元の教科書を読み返してみてください。
もちろん、ぐぐってもいいです。いくらでも出てきます。

関数の戻り値評価は

関数でなくても、

flag && --i > 0

とかでも同じ問題が発生します。

また、

name = id || "Anonymous Coward";

とかもよく見る書き方です。

Cでなくても、JavaでもJavaScriptでもRubyでもPerlでも同じことができます。

Re:いつでもfclose

[delta-keeper (31927)]

業界柄、使用が禁止されているということですよw
以下のリンクの(5)の通りでございます。。。

http://www.softech.co.jp/mm_070801_firm.htm#rule12-4

ひとまずありがとうございました!

Re:いつでもfclose

[Ryo.F (3896)]

業界柄、使用が禁止されているということですよw

それはよく理解できます。
もっと緩いソフトウェア開発の現場でも、レベルの低いプログラマ(というか、コーダというか)が混じっている場合(残念ながらよくあることです)には、そーゆーコーディング規約を定めることはよくあります。
つまり、レベルの低いメンバが理解していないだろうハマり所にハマらないように、最初から禁止しておく、ってことです。

言い換えれば、レベルの高い低いを決めるのは、その理由を知ってるか知らないかだ、ってことです。

fcloseやfreeもそうで、上から言われたからやってる、ってのじゃ、職業プログラマとしてはちょっと…となるわけです。
どれも学生さんが使う教科書にも載ってるわけですし。

Re:いつでもfclose

[delta-keeper (31927)]

> 言い換えれば、レベルの高い低いを決めるのは、その理由を知ってるか知らないかだ、ってことです。
緩い現場の話でのルール適用なら、私もそうだと思いますw
コードレビューする側に十分なスキルがある現場は理想的ですね。

> どれも学生さんが使う教科書にも載ってるわけですし。
そうですね!
くれぐれも気をつけたいと思います。

Re:いつでもfclose

[ktmizugaki (46208)]

> name = id || "Anonymous Coward";
これは、C と Java では動かないです。

--
svn-init() {
  svnadmin create .svnrepo
  svn checkout file://$PWD/.svnrepo .
}

Re:いつでもfclose

[Ryo.F (3896)]

ああ、確かにそうですね。
ご指摘ありがとうございます。