C++ の未定義動作によりクラッシュする。(NULL)->MemberFunction();

[ds14050]

以下のような構造を持つメンバ関数が複数存在する。

if (this) {
} else {
}

これに起因して -Og や -Os といった最適化オプションを付けてできあがった sakura.exe が起動直後にクラッシュする。

ソースがブログではあるがこれまでクラッシュしなかったのはたまたまであるらしい。>「C++ NULLポインタ経由のメンバ関数呼び出し - プログラミングの教科書を置いておくところ」

[berryzplus]

これが危険な話で、サクラエディタにはそういう「○コード」がたくさんあることは理解しています。

nullptrを返す関数の存在を許容しておきながら、関数が返したインスタンスのメソッドをノーチェックで呼び出すコードを許容している現状では、仕方ないんじゃないかなぁと思っています。

通常ならば両者の共存を許さない、という方向で環境整備を進めるんですが、すでにガチガチのスパゲッティなので「呼び出しても問題ない抜け道をつくる」しかなかったんだと思います。危険な状態にも見えるけど、意外と問題ないようになっているのかもです（楽観趣味）。

[ds14050]

おそらく NullObject パターンや Optional 型を使うように、いい感じに NULL を許容(無視)したかったんだと思います。

C++ で NullObject パターンはクラスごとに異なる NullObject クラスを用意して継承して、かえって大仰なことになりそうです。

std::optional は C++17 だそうです。

[m-tmatma]

以下の正規表現で検索できる。 if\s*\(\s*this\s*\)

[ds14050]

if (! this) {} else {} という、何かの嫌がらせかというようなコードが CPrintPreview.h に見つかりました。そこでこんなパターンにしてみました>if\s*\([^)]+(?=\bthis\b)。偽陽性は多いですが見逃しよりはましです。

しかしこれでも this ? A : B 型の(自分が好む)コードは見つけられません。

[ds14050]

これって原理的に無理なことをしようとしているわけではないんですよね。ただ未定義領域であるためにコンパイラに最適化を行う裁量が与えられていて、プログラマの利益にならない最適化のための最適化を行うコンパイラを使うとクラッシュするわけです。最適化を行わせなければ解決する問題です。

お世辞にもお行儀がいいとは言えませんが、大幅な書き直しをしなくてもこのパッチで -Os オプション付きでも起動直後にクラッシュすることがなくなります>nullpofix.txt

一応試してみましたが ~0 では結論が見えているためにコンパイラを出し抜くことはできませんでした。~1 が問題にならないと考えたのは、ポインタが(一般に)奇数アドレスに置かれることがないことを利用して Ruby の処理系がその 1 ビットに情報を埋め込んでいるという、大昔に聞いた話が元です。自分にはその話の確からしさを推定できるだけの知識もありません。

実際のところ &~1 が問題になるのは、有効な this ポインタが 0x1 という値を持っていたときにヌルポだと誤解してしまうという１点のみです。ありえないでしょう。そしてたぶんコンパイラはその可能性を棄却できない。

[ds14050]

この13件で全部だと思います。173件から目視で絞りました。CMyWnd.h が条件演算子タイプでした。対応して nullpofix.txt を更新しました。

□検索条件  "(?<!\\*)\\bthis(?!\\s*->| software| code)"
検索対象   *.* !*.svn-base
フォルダ   sakura_core\
    (サブフォルダも検索)
    (単語単位で探す)
    (英大文字小文字を区別する)
    (正規表現:bregonig.dll Ver.2.00 with hitend(). with Oniguruma 5.9.2)
    (文字コードセットの自動判別)
    (一致した行を出力)


sakura_core\doc\layout\CLayout.h(70,39)  [UTF-8]: 	CLogicInt GetLogicLineNo() const{ if(this)return m_ptLogicPos.GetY2(); else return CLogicInt(-1); } //$$$高速化
sakura_core\doc\layout\CLayout.h(102,44)  [UTF-8]: 	const CDocLine* GetDocLineRef() const{ if(this)return m_pCDocLine; else return NULL; } //$$note:高速化
sakura_core\doc\logic\CDocLine.h(60,6)  [UTF-8]: 		if(this){
sakura_core\doc\logic\CDocLine.h(69,6)  [UTF-8]: 		if(this){
sakura_core\env\CAppNodeManager.h(46,31)  [UTF-8]: 	HWND GetSafeHwnd() const{ if(this)return m_hWnd; else return NULL; }
sakura_core\env\CAppNodeManager.h(48,28)  [UTF-8]: 	int GetSafeId() const{ if(this)return m_nId; else return 0; }
sakura_core\env\CAppNodeManager.h(137,59)  [UTF-8]: inline CAppNodeGroupHandle EditNode::GetGroup() const{ if(this)return m_nGroup; else return 0; }
sakura_core\env\CAppNodeManager.h(139,52)  [UTF-8]: inline bool EditNode::IsTopInGroup() const{ return this && (CAppNodeGroupHandle(m_nGroup).GetEditNodeAt(0) == this); }
sakura_core\mfclike\CMyWnd.h(40,35)  [UTF-8]: 	HWND GetSafeHwnd() const{ return this?m_hWnd:NULL; }
sakura_core\print\CPrintPreview.h(91,50)  [UTF-8]: 	HWND GetPrintPreviewBarHANDLE_Safe() const{ if(!this)return NULL; else return m_hwndPrintPreviewBar; } //!< thisがNULLでも実行できる版。2007.10.29 kobake
sakura_core\view\colors\CColorStrategy.h(163,51)  [UTF-8]: 	EColorIndexType GetStrategyColorSafe() const{ if(this)return GetStrategyColor(); else return COLORIDX_TEXT; }
sakura_core\view\colors\CColorStrategy.h(165,6)  [UTF-8]: 		if(this){
sakura_core\window\CEditWnd.cpp(4682,31)  [UTF-8]: 	if( bShowProgress && NULL != this ){
173 個が検索されました。

[ds14050]

取り込みの可能性が示されれば nullpofix.txt を PR にしますが、コードの構造を変えずに簡便にクラッシュを回避できるとはいえ「MinGW ビルドのために」「コードの可読性を落とす」「ワークアラウンド」であるために反対意見を予想しています。

[k-takata]

正しくは呼び出し元でチェックすべきなのでしょうが、一体どのくらい修正が必要になるんですかね？ 調べるには assert(this != NULL) を入れた上でデバッグモードで動かしてみるのか、それとも何か別の方法を使うのか…

[ds14050]

正しくは呼び出し元でチェックすべきなのでしょうが、一体どのくらい修正が必要になるんですかね？

途中までやりましたが大変に面倒でした。ワークアラウンドに気がついてしまってはもうやるつもりがありません。

調べるには assert(this != NULL) を入れた上でデバッグモードで動かしてみるのか

その assert と同等のチェックを(規格上は)無意味なものだと取り除く最適化がクラッシュの原因です。

(追記) すべての呼び出し元ではなく NULL になり得るフローだけを洗い出して修正する手がかりにしようということでしょうか。それも泥縄式のワークアラウンドです。

[k-takata]

assertはあくまで修正箇所を探すための方法の1つとして挙げただけです。 呼び出し元を全て修正するのではなく、実際にNULLで呼んでいる箇所を見つけて、そこだけ修正できればいいのですが。（全箇所修正必要だったとなると残念ですが。）

[berryzplus]

認識している限りこんなのがあるんです。

m_pNanika->GetNankaNoObj().DoAction()->GetResult()->reset();

登場するメンバ名、メソッド名は、実際に存在するメンバ名、メソッド名とは一切関係ありません。

NULLチェックが漏れてるからNULLチェックしよう！という素直な対応が大変しずらいコードがあるわけです。どうしたらいいかの答えは、結構長いこと考えているんですが、いまだに出ません。

[ds14050]

EditNode へのポインタを一時変数に保存するというのが機械的に行える、つまりはリファクタリング的な手法だと思います。

[k-takata]

#1372 で、MinGW でビルドした Release バイナリは起動できるようになったのですが、あくまで暫定対処なので、reopenしておきます。暫定対処である点は以下の通り。

• MinGW バイナリを実際に動かして、クラッシュが発生したところのみに NULL チェックを追加したので、試せてない条件でクラッシュが発生する可能性は十分に残っている。
• VC++ バイナリに影響が出ないようにするため、this の NULL チェックは残したままになっている。（該当箇所には TODO コメントを追加済み） 将来的には削除すべき。