ファイルダイアログとフォルダ選択ダイアログでのバッファオーバーランを修正 #1599

usagisita · 2021-03-21T01:41:25Z

PR の目的

ファイルダイアログ、フォルダダイアログへ長い文字列を指定するとバッファオーバーランする不具合の修正をします。

カテゴリ

不具合修正
リファクタリング

PR の背景

極限値までいかなくても、ちょっと拡張子リストが長かったりすると、おかしいようなので、対策をします。

PR のメリット

バグが減ります。

PR のデメリット (トレードオフとかあれば)

ほとんどないと思います。

自動テストがないので、コードレビューが面倒です。

仕様・動作説明

「名前を付けて保存」ダイアログで、ファイルの種類の拡張子リストが正常に表示されないまたはクラッシュするのが修正されます。
保存ダイアログでは「ファイルの種類」で「ユーザ指定(*.txt:*.type;*.*)」と「テキストファイル(*.*)」と「すべてのファイル(*.*)」が選べます。
そのユーザ指定には今、開いているファイルの拡張子と現在のタイプ別設定が「ユーザ指定(*.open_file_ext:*.txt;*.log;*.*)」のように一度に表示されますが、長さ制限が足りていません。
開いているファイルの拡張子と、タイプ別設定に登録された拡張子リストが合わせてかなり長い場合、異常終了していました。

また同様に以下のマクロを実行する時も異常終了していました。
以前は落ちていたので実質的な影響はないと思います。
Editor.FileOpenDialog / FileSaveDialog("file_name")の指定にMAX_PATH制限を導入します。
Editor.FileOpenDialog("file_name.txt", "*.ext_filter")のext_filterが190文字前後以上長いとバッファオーバーランしてどこかおかしな動作をするのが修正されます。
Editor.FolderDialog("説明","default_folder")のdefault_folderにMAX_PATH制限を導入します。

PR の影響範囲

元の制限は
CFileExt::FileExtInfoTag::m_szExt[MAX_TYPES_EXTS*3+1]
MAX_TYPES_EXTSは64なので193文字(NULL含む)まで入力可能でした。
それ以上はチェックされずにオーバーランします。
こちらは保持する配列ごとstd::vector'とstd::wstringによりリファクタリングしました。

sakura/sakura_core/CFileExt.cpp

Line 85 in 95d9b03

wcscpy( m_puFileExtInfo[m_nCount].m_szExt, pszExt );

CDlgOpenFile_CommonFileDialogとCDlgOpenFile_CommonItemDialogのm_szDefaultWildCardはSFilePathなのでMAX_PATH制限があります。
こちらも異常終了する原因です。
こちらもstd::wstringに変更しました。

sakura/sakura_core/dlg/CDlgOpenFile_CommonItemDialog.cpp

Line 438 in 95d9b03

wcscpy( m_szDefaultWildCard, pszUserWildCard );

sakura/sakura_core/dlg/CDlgOpenFile_CommonFileDialog.cpp

Line 690 in 95d9b03

wcscpy( m_szDefaultWildCard, pszUserWildCard );

呼び出し側の関連処理では
CDocFileOperation.cppのCDocFileOperation::SaveFileDialog()では_MAX_PATH+10文字の制限がありました。

sakura/sakura_core/doc/CDocFileOperation.cpp

Lines 228 to 254 in 95d9b03

    
           WCHAR	szDefaultWildCard[_MAX_PATH + 10];	// ユーザー指定拡張子 
        
           { 
        
           	LPCWSTR	szExt; 
        
           	const STypeConfig& type = m_pcDocRef->m_cDocType.GetDocumentAttribute(); 
        
           	//ファイルパスが無い場合は *.txt とする 
        
           	if(!this->m_pcDocRef->m_cDocFile.GetFilePathClass().IsValidPath()){ 
        
           		szExt = L""; 
        
           	} 
        
           	else { 
        
           		szExt = this->m_pcDocRef->m_cDocFile.GetFilePathClass().GetExt(); 
        
           	} 
        
           	if (type.m_nIdx == 0) { 
        
           		// 基本 
        
           		if (szExt[0] == L'\0') {  
        
           			// ファイルパスが無いまたは拡張子なし 
        
           			wcscpy(szDefaultWildCard, L"*.txt"); 
        
           		} 
        
           		else { 
        
           			// 拡張子あり 
        
           			wcscpy(szDefaultWildCard, L"*"); 
        
           			wcscat(szDefaultWildCard, szExt); 
        
           		} 
        
           	} 
        
           	else { 
        
           		szDefaultWildCard[0] = L'\0';  
        
           		CDocTypeManager::ConvertTypesExtToDlgExt(type.m_szTypeExts, szExt, szDefaultWildCard);

ConvertTypesExtToDlgExt(type.m_szTypeExts, szExt, szDefaultWildCard)
szExtにファイルの拡張子を指定して、タイプ別設定と一緒にデフォルトフィルターに指定しています。
ローカル変数szDefaultWildCardに必要とされる最大値はMAX_PATH+MAX_TYPES_EXTS*2前後で足りていません。
m_szTypeExtsは「a;b;c」「.a;.b;.c」→「*.a;*.b;*.c」のように展開されるので最大でも約2倍必要です。
PRでは細かいことは気にせずstd::wstringに変更しています。

CDocTypeManager::ConvertTypesExtToDlgExtでは書き込みバッファの最大値がわからないため、std::wstringで返すようにリファクタリングしました。

テスト内容

共通設定ー編集「Vistaスタイルのファイルダイアログ」をON/OFFすると表示に使う実装クラスが異なるため、ON/OFF2回ずつテストが必要です。
自動テストがなくて申し訳ないです。

テスト1

手順
下記マクロを実行して、異常終了しないことを確認します。

マクロでFileOpenDialog/FileSaveDialogのファイル名のデフォルト指定が長いと異常終了していたのが、マクロ中断のエラーダイアログが表示されるようになります。

var s = FileOpenDialog("file_name_long_long_0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456.js", "*.js");

テスト2

手順
同マクロで、拡張子フィルターが長いと異常終了していたのが、直ります。
特にいまのところ拡張子フィルターの文字列長に制限はありません、

var s = FileOpenDialog("file_name.js", "*.js;*.long_long_ext_filter_012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567");

テスト3

手順
フォルダを開くマクロのデフォルト指定がMAX_PATHを超えていたときに落ちていたのが、マクロ中断のエラーダイアログが表示されるようになります。

FolderDialog("バッファチェックのテスト", "C:\Program files\デフォルト表示フォルダ名123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678900123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890")

テスト4

タイプ別設定の拡張子に以下を設定：
a;b;c;d;e;f;g;h;i;j;k;l;m;n;o;p;q;r;s;t;u;v;w;x;y;z;0;1;2;3;4;5

拡張子が異常に長いファイル(存在していなくてもいい)を開きます。
ファイル名例：
D:\testfile.long_long_ext_filter_01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234

上記のタイプ別を一時適用します。

「名前を付けて保存」でダイアログを表示します。
落ちないか確認します。
また拡張子リストが正しく表示されるか確認します。

ダイアログを閉じた後に異常終了することがありました、のでそこまで確認します。

参考資料

usagisita · 2021-03-21T01:43:21Z

ちょっとPRが大きくなってしまったので、どうしようか悩みましたが、とりあえずPRを投げて、分割したらいいのか、所感など様子見をしたいです。

このままで問題ないのであれば、このPRの状態でレビューをしていただして、先に進めたいと思います。
よろしくお願いします。

AppVeyorBot · 2021-03-21T02:13:00Z

✅ Build sakura 1.0.3585 completed (commit 717e9765b1 by @usagisita)

berryzplus

PRの趣旨には賛成で、見た限りではなんとなく良さそうに見えます。

とりあえず、👇についてコメントをお願いします。

8 Code Smells

既存CodeSmellsが2万件もあるので誤差レベルといえばそうなんですけど、修正した範囲で新たに入れるものはなるたけ避けたいです（取り除くのが無理な場合もあるのでその時は「無理です」と言ってください。）

berryzplus · 2021-03-21T05:55:43Z

sakura_core/macro/CMacro.cpp

+			// sDefaultの先はSFilePath型
+			if (MAX_PATH <= sDefault.length()) {
+				return false;
+			}


このファイルの変更は、他の変更と無関係なのでバラすことができます。（「バラせ」とは言いません。）
他の変更と異なり、かなり頑張らないとテストコードを書けない点も他と異なります。

berryzplus · 2021-03-21T06:04:33Z

sakura_core/env/CDocTypeManager.h

@@ -51,7 +51,7 @@ class CDocTypeManager{

 	static bool IsFileNameMatch(const WCHAR* pszTypeExts, const WCHAR* pszFileName);	// タイプ別拡張子にファイル名がマッチするか
 	static void GetFirstExt(const WCHAR* pszTypeExts, WCHAR szFirstExt[], int nBuffSize);	// タイプ別拡張子の先頭拡張子を取得する
-	static bool ConvertTypesExtToDlgExt( const WCHAR *pszSrcExt, const WCHAR* szExt, WCHAR *pszDstExt );	// タイプ別設定の拡張子リストをダイアログ用リストに変換する
+	static bool ConvertTypesExtToDlgExt(const WCHAR *pszSrcExt, const WCHAR* szExt, std::wstring& destExt);	// タイプ別設定の拡張子リストをダイアログ用リストに変換する


static関数で内部的なインスタンス依存もないので、挙動を確認するテストを書けそうです。
引数 std::wstring& destExt を戻り値 std::wstring に変えてしまったほうが、呼出元コードがすっきりすると思いました（変換失敗は戻り値.empty()で判定できますし。）

戻り値にしてみました。テストコードも追加しました。

sakura_core/doc/CDocFileOperation.cpp

sakura_core/CFileExt.h

berryzplus · 2021-03-21T06:16:35Z

sakura_core/CFileExt.h

@@ -41,7 +41,7 @@ class CFileExt
 {
 public:
 	CFileExt();
-	~CFileExt();
+	~CFileExt() = default;


独自デストラクタを削除できるのであれば、独自コンストラクタも削除して Rule-of-Zero に従うのがベターだと思います。SonarCloudのBugsレベル警告のうちCriticalで指摘されるものがこの件で、5種類セットで書くか何も書かないかどちらかにしないとCritical警告が出るみたいです。

デストラクタは消して、コンストラクタはdefaultにしました。
defaultを書いておかないと、コンパイラにエラーで怒られてしまいます。

自分が言ってたのは「何も書かないにできないか？」でした。
コンストラクタをdefaultにする必要があるなら、5種類セットで書く感じになります。

👇5種類

CFileExt() = default; CFileExt(const CFileExt&) = delete; CFileExt& operator = (const CFileExt&) = delete; CFileExt(CFileExt&&) noexcept = delete; CFileExt& operator = (CFileExt&&) noexcept = delete; virtual ~CFileExt() noexcept = default;

あれ？6種類ある・・・。

細かい仕様ははっきり知らなかったので、ちょっと調べてみたのですが、
http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29
https://en.cppreference.com/w/cpp/language/rule_of_three
を見たところ「普通のコンストラクタ」が5個には含まれない、つまりコンストラクタは存在していていい、ということみたいですね。
Rule of zeroのところにコンストラクタだけ宣言されています。

書くなら全部かけRule of fiveは、デストラクタを書くなら、コピーとムーブ演算子を定義するかdeleteする、という意味ですね。コピーやムーブ以外の、普通のコンストラクタは無関係かと。

・マクロでフォルダを開く初期フォルダ制限・pszUserWildCardがSFilePathでMAX_PATH制限・CFileExtのExtが短い固定長だったのをwstringへ変更

・バッファオーバーランしていたのを修正

usagisita · 2021-03-21T10:12:16Z

なんか単体テストがエラー吐いていて、ダメみたいですね。

AppVeyorBot · 2021-03-21T10:16:36Z

❌ Build sakura 1.0.3588 failed (commit 71b3f7465e by @usagisita)

berryzplus · 2021-03-21T12:10:20Z

なんか単体テストがエラー吐いていて、ダメみたいですね。

vs2019 を使って x64 - Debug ビルドしたモジュールで動かしたらテスト失敗が再現しました。

これ。
CDlgOpenFile::CommonItemDialogCreate
失敗原因はクラッシュみたいです。
なんとなく、テストが正しく実装されてないように見えます・・・。

berryzplus · 2021-03-21T12:36:27Z

テストがクラッシュする原因は、CDlgOpenFile_CommonItemDialogのコンストラクタが GetDllShareData() を呼びだしていることのようです。現状では、グローバル変数が初期化されていない状態で共有メモリ取得のためのグローバル GetDllShareData() を呼びだした場合には正常動作として「クラッシュ」するようになっています。サクラエディタのダイアログは、基本的に共有メモリに依存する設計になっており単体テストを記述することは不可能です。

たぶんテストを書けるのは CFileExt とか CDocTypeManager::ConvertTypesExtToDlgExt とかのグローバル依存がないコードだけだと思います。

sakura_core/dlg/CDlgOpenFile_CommonItemDialog.cpp

berryzplus · 2021-03-21T12:58:16Z

テスト失敗の対策案を出しときます。

TEST(CDlgOpenFile, Construct)

　👇

TEST(CDlgOpenFile, DISABLED_Construct)

他の TEST(CDlgOpenFile,で始まる行にも同様の修正をすれば、テスト可能な部分だけが実行される状態になります。
せっかく書いたテストを消すのももったいないので、GetDllShareData() の問題を解決できるまで無効にしとけばよいと思います。

usagisita · 2021-03-21T13:16:38Z

コメントありがとうございます。m_pShareDataの初期化を遅延させればと浅知恵しましたが、Createの後はいくつかの関数に分かれて呼び出されるため、バラバラに書くのも微妙だなという結論に達しました。
おとなしく教えてもらったように、DISABLEにしておきます。

AppVeyorBot · 2021-03-21T14:28:07Z

✅ Build sakura 1.0.3595 completed (commit 330d80cf65 by @usagisita)

AppVeyorBot · 2021-03-21T15:30:13Z

✅ Build sakura 1.0.3597 completed (commit 20e66183de by @usagisita)

usagisita · 2021-03-21T18:03:32Z

Code Smellsのうち1件は直前の修正のミスだったので、対応しました。
残り1件のほうは、処理構造によるもの(for/if/whileの階層が深い)だったので、中身をリファクタリングしないと無理なので、今回は無理ということで、お願いします。

suconbu · 2021-03-21T17:57:11Z

sakura_core/env/CDocTypeManager.cpp

-	@param szExt [in] リストの先頭にする拡張子 例「.h」
+	@param pszSrcExt [in]  拡張子リスト 例「.c cpp;.h」(ドットはオプション)
+	@param szExt [in] リストの先頭にする拡張子 例「.h」(ドット必須)
+	@param destExt [out] 拡張子リスト 例「*.h;*.c;*.cpp」


最後の @param destExt ... の所は @return になると思います。

suconbu · 2021-03-21T18:09:03Z

tests/unittests/test-cdoctypemanager.cpp

+TEST(CDocTypeManager, ConvertTypesExtToDlgExtThree)
+{
+	const std::wstring expected = { L"*.txt;*.cpp;*.h;*.hpp" };
+	std::wstring actual = CDocTypeManager::ConvertTypesExtToDlgExt(L"cpp;h;hpp", L".txt");
+	EXPECT_EQ(expected, actual);
+}


pszSrcExt の中に szExt の拡張子を含むパターン (例：ConvertTypesExtToDlgExt(L"txt;cpp;h;hpp", L".txt");) も用意されていた方が良いのではと思いました。

AppVeyorBot · 2021-03-21T18:36:29Z

✅ Build sakura 1.0.3601 completed (commit 45efb69175 by @usagisita)

sonarcloud · 2021-03-22T10:59:40Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

53.5% Coverage
2.1% Duplication

AppVeyorBot · 2021-03-22T11:15:02Z

✅ Build sakura 1.0.3602 completed (commit aa426b6117 by @usagisita)

usagisita · 2021-03-22T13:45:19Z

長くお付き合いくださり、ありがとうございます。
コミットしようと思います。

berryzplus · 2021-03-23T05:09:55Z

あえて揚げ足取っときますがマージですね。

git commit ＝変更をローカルブランチに保存する。
git push ＝ローカルブランチをリモートに送信する。
マージ＝ブランチの変更を master などに取り込む。

beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Mar 21, 2021

berryzplus reviewed Mar 21, 2021

View reviewed changes

usagisita added 10 commits March 21, 2021 18:21

CFileExtの助長なm_nCountを削除

f941df8

CFileExt::GetExtFilterからフィルター文字列作成を分離

77b5ee6

CDlgOpenFileのワイルドカード長制限修正

cdb7079

CDlgOpenFile_CommonItemDialogのm_strDefaultWildCardをstd::wstringに変更

22aca79

・バッファオーバーランしていたのを修正

SonarCloud一部対応

4470d7d

一部テスト追加

51f8ab4

指摘対応　ConvertTypesExtToDlgExtの処理結果を戻り値にする

4cbf16e

CFileExtのコンストラクタをdefault指定にしデストラクタを暗黙生成にする

5a77929

ConvertTypesExtToDlgExtのテストケースを追加

60008c8

usagisita force-pushed the feature/fix_file_dialog_limit branch from dfa3f75 to 60008c8 Compare March 21, 2021 09:50

beru reviewed Mar 21, 2021

View reviewed changes

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

usagisita added 3 commits March 21, 2021 22:40

m_strDefaultWildCardの比較をwcscmpから!=にする

40fb2a3

CDlgOpenFileのテストでGetDllShareData()が必要なためDISABLEDにする

b822a64

SonarCloud対策

fb761e8

PRミスのCDlgOpenFile_CommonItemDialogコンストラクタの初期化重複を削除

d55fa15

suconbu reviewed Mar 21, 2021

View reviewed changes

usagisita added 2 commits March 22, 2021 19:35

指摘対応コメント修正

5dab9c2

ConvertTypesExtToDlgExtの拡張子が同じだったときのテストケースを追加

5752f12

berryzplus approved these changes Mar 22, 2021

View reviewed changes

usagisita merged commit 4a64e53 into sakura-editor:master Mar 22, 2021

usagisita deleted the feature/fix_file_dialog_limit branch March 22, 2021 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ファイルダイアログとフォルダ選択ダイアログでのバッファオーバーランを修正 #1599

ファイルダイアログとフォルダ選択ダイアログでのバッファオーバーランを修正 #1599

usagisita commented Mar 21, 2021 •

edited

usagisita commented Mar 21, 2021

AppVeyorBot commented Mar 21, 2021

berryzplus left a comment

berryzplus Mar 21, 2021

berryzplus Mar 21, 2021

usagisita Mar 21, 2021

berryzplus Mar 21, 2021

usagisita Mar 21, 2021

berryzplus Mar 21, 2021

usagisita Mar 21, 2021

usagisita commented Mar 21, 2021

AppVeyorBot commented Mar 21, 2021

berryzplus commented Mar 21, 2021

berryzplus commented Mar 21, 2021

berryzplus commented Mar 21, 2021

usagisita commented Mar 21, 2021

AppVeyorBot commented Mar 21, 2021

AppVeyorBot commented Mar 21, 2021

usagisita commented Mar 21, 2021

suconbu Mar 21, 2021 •

edited

suconbu Mar 21, 2021

AppVeyorBot commented Mar 21, 2021

sonarcloud bot commented Mar 22, 2021

AppVeyorBot commented Mar 22, 2021

usagisita commented Mar 22, 2021

berryzplus commented Mar 23, 2021

	WCHAR szDefaultWildCard[_MAX_PATH + 10]; // ユーザー指定拡張子
	{
	LPCWSTR szExt;

	const STypeConfig& type = m_pcDocRef->m_cDocType.GetDocumentAttribute();
	//ファイルパスが無い場合は *.txt とする
	if(!this->m_pcDocRef->m_cDocFile.GetFilePathClass().IsValidPath()){
	szExt = L"";
	}
	else {
	szExt = this->m_pcDocRef->m_cDocFile.GetFilePathClass().GetExt();
	}
	if (type.m_nIdx == 0) {
	// 基本
	if (szExt[0] == L'\0') {
	// ファイルパスが無いまたは拡張子なし
	wcscpy(szDefaultWildCard, L"*.txt");
	}
	else {
	// 拡張子あり
	wcscpy(szDefaultWildCard, L"*");
	wcscat(szDefaultWildCard, szExt);
	}
	}
	else {
	szDefaultWildCard[0] = L'\0';
	CDocTypeManager::ConvertTypesExtToDlgExt(type.m_szTypeExts, szExt, szDefaultWildCard);

ファイルダイアログとフォルダ選択ダイアログでのバッファオーバーランを修正 #1599

ファイルダイアログとフォルダ選択ダイアログでのバッファオーバーランを修正 #1599

Conversation

usagisita commented Mar 21, 2021 • edited

PR の目的

カテゴリ

PR の背景

PR のメリット

PR のデメリット (トレードオフとかあれば)

仕様・動作説明

PR の影響範囲

テスト内容

テスト1

テスト2

テスト3

テスト4

関連 issue, PR

参考資料

usagisita commented Mar 21, 2021

AppVeyorBot commented Mar 21, 2021

berryzplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usagisita commented Mar 21, 2021

AppVeyorBot commented Mar 21, 2021

berryzplus commented Mar 21, 2021

berryzplus commented Mar 21, 2021

berryzplus commented Mar 21, 2021

usagisita commented Mar 21, 2021

AppVeyorBot commented Mar 21, 2021

AppVeyorBot commented Mar 21, 2021

usagisita commented Mar 21, 2021

suconbu Mar 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Mar 21, 2021

sonarcloud bot commented Mar 22, 2021

AppVeyorBot commented Mar 22, 2021

usagisita commented Mar 22, 2021

berryzplus commented Mar 23, 2021

usagisita commented Mar 21, 2021 •

edited

suconbu Mar 21, 2021 •

edited