Fix SIGSEGV in CFileNameManager caused by stale share data pointer across test suites#2534
Fix SIGSEGV in CFileNameManager caused by stale share data pointer across test suites#2534berryzplus with Copilot wants to merge 14 commits into
Conversation
テストスイート単位でインスタンス生成するように変える。 メンバーアクセスがテストを書くのに都合が悪いので、一時的に変える。
TrayWndもウィンドウなので、メッセージ配送が主な役割。 現時点では「おかしなメッセージ」に耐性がなくてテスト不能なものが結構ある。
CFileNameManagerはTSingletonでシングルトンとして管理されており、 コンストラクタでGetDllShareData()を呼び出してm_pShareDataをキャッシュしていた。 テスト環境では複数のテストスイートで共有データが生成・破棄されるため、 テストスイートの切り替え時にm_pShareDataが無効なメモリを指すことがあった。 これにより、CSakuraEnvironmentTest.ExpandParameter_mixedでSIGSEGVが発生していた。 m_pShareDataメンバ変数を削除し、各メソッドでGetDllShareData()を直接呼び出す よう修正することで、常に有効な共有データを参照するようにした。
berryzplus
left a comment
There was a problem hiding this comment.
解析結果は正しいのだけど、直し方が元の設計に沿ってないようなので却下です。
仕様
CFileNameManager は生成した時点の共有メモリポインタをキャッシュして使う。
m_pShareDataは生成時以外変更しないため、使用時にはNULLチェックをしない。
共有メモリの生成状態に左右されるコードでありながら、
共有メモリの生成状態を考慮してないため、
オブジェクトの生存期間を「共有メモリの生成後、破棄される前まで」に限定してやる必要あり。
そうしないとあちこち変更する感じになり、できるだけ元コードを変更しない、の方針に反してしまう。
| @@ -26,7 +26,6 @@ class CFileNameManager : public TSingleton<CFileNameManager>{ | |||
| friend class TSingleton<CFileNameManager>; | |||
| CFileNameManager() | |||
There was a problem hiding this comment.
「このクラスがシングルトンであること」が誤り。
前提
シングルトンクラスは、状態を持ってはならない。
いつ生成しても不変なオブジェクトを何度も生成しないための仕組みがシングルトン。
2つ以上の状態を持つオブジェクトをシングルトンにしてはならない。
TSingleton テンプレート利用クラスのほとんどがこれにあたるので、本当は直さないといけない。
この修正の趣旨
状態変数 m_pShareData の代わりに GetDllShareDataPtr() を使う。
ダメ出しポイント
もともと、状態変数 m_pShareData は絶対 NULL にならない。
したがって、m_pShareData が NULL だった場合のガードは存在しない。
GetDllShareData() は NULL を返し得る関数 として定義している。
この関数の戻り値は deref する前にNULLチェックする必要がある。
つまり、こうなる。
仕様上NULLにならないからチェックしてないコード。
↓
仕様上NULLになり得るけど移行した結果チェックが漏れたコード。
|
この修正 a01decc を入れて対応したのでクローズ。 |
CSakuraEnvironmentTest.ExpandParameter_mixedcrashed with SIGSEGV inCFileNameManager::GetTransformFileNameFastin the mingw (Debug) CI job.CFileNameManageris aTSingletonthat cachedm_pShareData = &GetDllShareData()in its constructor. In the test environment,CShareDatais destroyed and recreated between test suites — whenTrayWndTestran beforeCSakuraEnvironmentTest, the singleton's cached pointer referred to unmapped memory.Fix
CFileNameManager.h/CFileNameManager.cpp: Removem_pShareDatamember. Replace allm_pShareData->accesses with directGetDllShareData()calls at each use site.This is safe in production because
CShareDatalives for the entire process lifetime; it only matters in tests where share data is cycled.Also included
Merge of
feature/improve_traywnd_testwhich introduced theTrayWndTesttests that exposed this bug, along with several SonarQube fixes inCControlTray.cpp/.hand test infrastructure updates (ShareDataTestSuite,EditorTestSuite,UiaTestSuite).