コンボボックスのサブクラス化に関連するクラッシュバグの修正 #1481

kengoide · 2020-12-09T14:28:04Z

PR の目的

「ファイルを開く」および「名前を付けて保存」ダイアログにて、「最近のファイル」・「最近のフォルダ」コンボボックスにフォーカスがある状態で Ctrl+Backspace を押すとクラッシュします。
#1470 の機能追加による劣化バグで、本PRはその修正が目的です。

カテゴリ

不具合修正

PR の背景

問題の原因ですが、コンボボックスをサブクラス化する処理が CBS_DROPDOWNLIST （編集できないコンボボックス）を考慮していなかったことです。
SetComboBoxDeleter 関連のコードは、API関数 GetComboBoxInfo を通じてコンボボックス内部の Edit のハンドルを入手できることを前提としています。編集可能なコンボボックスでは期待通りにEditを取得できますが、CBS_DROPDOWNLIST スタイルのコンボボックスに対して同じことをすると結果は ComboBox のハンドルになってしまいます。このケースを考慮できていませんでした。

sakura/sakura_core/dlg/CDialog.cpp

Lines 813 to 816 in 526f512

    
           COMBOBOXINFO info = { sizeof(COMBOBOXINFO) }; 
        
           if (!::GetComboBoxInfo(hwndCtl, &info)) 
        
           	return; 
        
           ::SetWindowSubclass(info.hwndItem, SubEditProc, 0, (DWORD_PTR)pRecent);

hwndCtl が CBS_DROPDOWNLIST スタイルのコンボボックスだった場合、info.hwndItem に ComboBox が入った状態で返ってくるため、Edit をサブクラス化するつもりで ComboBox をサブクラス化することになります。

sakura/sakura_core/dlg/CDialog.cpp

Lines 767 to 770 in 526f512

    
           LRESULT CALLBACK SubEditProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam, 
        
           	                         UINT_PTR uIdSubclass, DWORD_PTR dwRefData) 
        
           { 
        
           	HWND hwndCombo = GetParent(hwnd);

ここで Edit の親（＝ComboBox）を取得しようとして ComboBox の親（＝ダイアログ）を取得してます。

sakura/sakura_core/dlg/CDialog.cpp

Lines 781 to 785 in 526f512

    
           case WM_CHAR: 
        
           	// ASCII 削除文字。Ctrl + Backspace が入力された。 
        
           	if (wParam == 0x7f) { 
        
           		DWORD selStart, selEnd; 
        
           		Combo_GetEditSel(hwndCombo, selStart, selEnd);

hwndCombo はダイアログです。CB_GETEDITSELには反応してくれません。selStartとselEndは未初期化のままです。

sakura/sakura_core/dlg/CDialog.cpp

Lines 793 to 798 in 526f512

    
           // 単語削除する 
        
           const int length = ::GetWindowTextLength(hwndCombo); 
        
           auto text = std::make_unique<wchar_t[]>(length + 1); 
        
           ::GetWindowText(hwndCombo, text.get(), length + 1); 
        
           const int pos = DeletePreviousWord(text.get(), length, selStart);

length は 0、textはL'\0'が入った長さ1の配列となります。selStartが偶然0になっていなければDeletePreviousWordの中で Access Violation が発生します😢

仕様・動作説明

問題に対する対処として、CBS_DROPDOWNLIST スタイルのコンボボックスに SetComboBoxDeleter を適用しないことにします。該当するのは「ファイルを開く」および「名前を付けて保存」ダイアログ下部のそれです。
副作用として履歴削除が機能しなくなる…と思いきや、実は別バグ（後述）により CBS_DROPDOWNLIST スタイルのコンボボックスでは元から機能していなかったため、機能面では退行しないものと思われます。

PR の影響範囲

以下のダイアログに存在したクラッシュバグを修正します。

ファイルを開く・名前を付けて保存（Vistaスタイルを除く）

以下の画面に存在するコンボボックスに関係するコードを変更します。

検索・置換
Grep・Grep置換
外部コマンド実行
ツールバーの検索ボックス

テスト内容

「共通設定」→「編集」→「Vistaスタイルのファイルダイアログ」にチェックが入っていない状態で、開く・保存ダイアログに対して以下のテストが必要です
- 「最近のファイル」・「最近のフォルダ」をクリックするなどしてフォーカスを移動させる
- Ctrl + Backspace を押下する
- プログラムがクラッシュしないことを確認する
他の画面の該当コンボボックス（上記）に対して、履歴削除・単語削除がそれぞれ引き続き機能することの確認が必要です。

参考資料

履歴削除の別バグ

sakura/sakura_core/dlg/CDialog.cpp

Lines 725 to 738 in 526f512

    
           // コンボボックスのキャレット位置を取得 
        
           DWORD dwSelStart = 0; 
        
           DWORD dwSelEnd = 0; 
        
           Combo_GetEditSel( hwndCombo, dwSelStart, dwSelEnd ); 
        
           // アイテムテキストとエディットテキストが異なる、またはエディットが全選択でなかった場合 
        
           if ( cItemText != cEditText 
        
           	|| 0 < dwSelStart 
        
           	|| dwSelEnd < (DWORD)cEditText.GetStringLength() 
        
           	) 
        
           { 
        
           	// 履歴削除をスキップする 
        
           	return; 
        
           }

CBS_DROPDOWNLIST スタイルのコンボボックスは CB_GETEDITSEL が送られてきても何もしないようで、dwSelStartとdwSelEndは常に0のまま変わりません。cEditTextは最短でも長さ1（null文字の分）であるため、dwSelEnd < (DWORD)cEditText.GetStringLength()の条件式により、履歴削除が実行されることはありません。
本 PR では元から動いていなかったことを理由として機能を削除しますが、対処方針として適切かどうかというと微妙だと思っています。後から追加PRとして修正することも考えていますので、問題があればご指摘ください。

AppVeyorBot · 2020-12-09T14:51:01Z

✅ Build sakura 1.0.3287 completed (commit 6073a73ce1 by @k-kagari)

berryzplus

指摘2件ありますが、クラッシュバグの回復が優先だと思うのでapproveにしておきます。

sakura_core/dlg/CDialog.cpp

kengoide · 2020-12-11T09:39:24Z

Approvalをいただきましたが、PRの出し直しを避けたいため先に指摘に対応させてください。

AppVeyorBot · 2020-12-11T09:53:32Z

✅ Build sakura 1.0.3288 completed (commit 25aff663e3 by @k-kagari)

kengoide · 2020-12-12T02:09:45Z

承認ありがとうございます。

CBS_DROPDOWNLISTであるコンボボックスをサブクラス化しないようにする

df44223

kengoide added 🐛bug🦋 ■バグ修正(Something isn't working) 💩degradation🧻🚽 デグレ (前に動いていた機能が動かなくなった) labels Dec 9, 2020

berryzplus previously approved these changes Dec 9, 2020

View reviewed changes

sakura_core/dlg/CDialog.cpp Outdated Show resolved Hide resolved

sakura_core/dlg/CDialog.cpp Outdated Show resolved Hide resolved

sanomari previously approved these changes Dec 10, 2020

View reviewed changes

レビュー指摘に対応: assertを除去

e726726

kengoide dismissed stale reviews from sanomari and berryzplus via e726726 December 11, 2020 09:30

berryzplus approved these changes Dec 12, 2020

View reviewed changes

kengoide merged commit ad90218 into sakura-editor:master Dec 12, 2020

kengoide deleted the bugfix/crash-on-dropdownlist branch December 12, 2020 02:10

This was referenced Jun 28, 2021

履歴管理クラスに関するインクルード文を整理する #1697

Merged

使用されていないCRecentの呼び出し箇所を除去する #1698

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

コンボボックスのサブクラス化に関連するクラッシュバグの修正 #1481

コンボボックスのサブクラス化に関連するクラッシュバグの修正 #1481

kengoide commented Dec 9, 2020 •

edited

Loading

AppVeyorBot commented Dec 9, 2020

berryzplus left a comment

kengoide commented Dec 11, 2020

AppVeyorBot commented Dec 11, 2020

kengoide commented Dec 12, 2020

	COMBOBOXINFO info = { sizeof(COMBOBOXINFO) };
	if (!::GetComboBoxInfo(hwndCtl, &info))
	return;
	::SetWindowSubclass(info.hwndItem, SubEditProc, 0, (DWORD_PTR)pRecent);

	LRESULT CALLBACK SubEditProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam,
	UINT_PTR uIdSubclass, DWORD_PTR dwRefData)
	{
	HWND hwndCombo = GetParent(hwnd);

	case WM_CHAR:
	// ASCII 削除文字。Ctrl + Backspace が入力された。
	if (wParam == 0x7f) {
	DWORD selStart, selEnd;
	Combo_GetEditSel(hwndCombo, selStart, selEnd);

	// 単語削除する
	const int length = ::GetWindowTextLength(hwndCombo);
	auto text = std::make_unique<wchar_t[]>(length + 1);
	::GetWindowText(hwndCombo, text.get(), length + 1);

	const int pos = DeletePreviousWord(text.get(), length, selStart);

	// コンボボックスのキャレット位置を取得
	DWORD dwSelStart = 0;
	DWORD dwSelEnd = 0;
	Combo_GetEditSel( hwndCombo, dwSelStart, dwSelEnd );

	// アイテムテキストとエディットテキストが異なる、またはエディットが全選択でなかった場合
	if ( cItemText != cEditText
	\|\| 0 < dwSelStart
	\|\| dwSelEnd < (DWORD)cEditText.GetStringLength()
	)
	{
	// 履歴削除をスキップする
	return;
	}

コンボボックスのサブクラス化に関連するクラッシュバグの修正 #1481

コンボボックスのサブクラス化に関連するクラッシュバグの修正 #1481

Conversation

kengoide commented Dec 9, 2020 • edited Loading

PR の目的

カテゴリ

PR の背景

仕様・動作説明

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

履歴削除の別バグ

AppVeyorBot commented Dec 9, 2020

berryzplus left a comment

Choose a reason for hiding this comment

kengoide commented Dec 11, 2020

AppVeyorBot commented Dec 11, 2020

kengoide commented Dec 12, 2020

kengoide commented Dec 9, 2020 •

edited

Loading