コマンドラインでMAX_PATHを超えるファイル名を指定すると落ちる #1423

usagisita · 2020-10-03T17:01:43Z

根本的にやり方が違うと思ったときにやるべきことはカウンターPRの作成っす。

最新版のPRでは直っているので、やり方が違うとは思わないのですが、これも勉強になると思って、一応作っては直し、作っては直し結構時間が掛かったPRを置いておきます。

コマンドラインオプションが指定されたときに以下のように変更します。

・パスが260(MAX_PATH)より長いときはエラーメッセージを表示してそのパスは無視するように
・エラーメッセージは最初の一件のみを表示
・表示されるパスが適当な長さ(MAX_PATH*2)より長い場合は省略して表示
・一部コードをリファクタリングもどきとして変更
・エラーが出てもサクラエディタは起動する

以下、本来は本件とは無関係だけど一応適用
・エラー1回のみとダイアログ表示の共通化で「ファイル名に使えない文字」エラーにも適用
>sakura.exe * * * * * * とかやるとエラーを閉じるのも大変です。

なんかよく分からない感じですが、一応公開してみます。

PR の目的

#1406 　このIssues対策
#1420 　このPRのカウンターPR？というらしいものです
関連の細かい仕様とかは、向こう参照

カテゴリ

不具合修正

PR の背景

PR のメリット

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

仕様・動作説明

テスト内容

テスト1

手順

PR の影響範囲

参考資料

長いときはエラーメッセージを表示するようにエラーメッセージは最初の一回のみ表示

usagisita · 2020-10-03T17:11:05Z

sakura_core/_main/CCommandLine.h

@@ -90,7 +90,7 @@ class CCommandLine : public TSingleton<CCommandLine> {
 	LPCWSTR GetDocType() const noexcept { return m_fi.m_szDocType; }
 	ECodeType GetDocCode() const noexcept { return m_fi.m_nCharCode; }
 	void ParseKanjiCodeFromFileName( LPWSTR pszExeFileName, int cchExeFileName );
-	void ParseCommandLine( LPCWSTR pszCmdLineSrc, bool bResponse = true );
+	void ParseCommandLine( LPCWSTR pszCmdLineSrc, bool bResponse = true, bool bError = false );


自分で言うのも何ですが、bError の引数があんまりイケてないですね。

bError だと引数がエラーに関する事だとしか読み取れないので bErrorFound とかいう名前にすると良いと思います。

usagisita · 2020-10-03T17:12:38Z

sakura_core/_main/CCommandLine.cpp

@@ -536,7 +557,7 @@ void CCommandLine::ParseCommandLine( LPCWSTR pszCmdLineSrc, bool bResponse )
 		while(input){
 			responseData += input.ReadLineW();
 		}
-		ParseCommandLine( responseData.c_str(), false );
+		return ParseCommandLine( responseData.c_str(), false, bError );


returnの部分、void関数のreturnにvoid関数の結果を書くのは合法ですけど、無意味な変更です。
あと下にも本物のreturn文があるので、あんまイケてないですね。

ここであえて return を付けた事で工数見積もりが増えて良いと思います。

berryzplus · 2020-10-03T17:34:56Z

根本的にやり方が違うと思ったときにやるべきことはカウンターPRの作成っす。

最新版のPRでは直っているので、やり方が違うとは思わないのですが、これも勉強になると思って、一応作っては直し、作っては直し結構時間が掛かったPRを置いておきます。

大事だと思います 😄

・エラーメッセージは最初の一件のみを表示

どっちでもいい感じの変更なので、もめそうな気配を感じました。

考え方として2つあって、それと別にたくさんエラーが出る場合のうっとおしさの軽減策の話があると思います。

エラーを検出したら、その時点で実行を終了させるべきだ。
エラー検出後に続行する場合は、検出したすべてのエラーを表示するべきだ。
たくさんエラーが出るとうっとおしいので、一定回数以上のエラー表示は抑制できると嬉しい。

ぼくの利用状況ではエラーは発生しないので、抑制機構を導入する意味はなさそうです。

ただ、エラーが頻発する利用環境では欲しい機能かも知れません。
「なんかエラーがある」と「めっちゃエラーがある」を区別できたほうが嬉しいのでは？と思います。
そうすると設定ファイルを利用して挙動を変える検討が必要になるわけですが、コマンドライン解析関数のコメントに書いてある通り、コマンドライン解析を行うタイミングでは sakura.ini の情報を参照することはできません。

・表示されるパスが適当な長さ(MAX_PATH*2)より長い場合は省略して表示

表示できないほどの分量ではないので、省略表示しなくてもよいと思います。
VGAサイズとかだと無理かもしれませんけど普通のPCディスプレイなら余裕で表示できるはず・・・。

以下、本来は本件とは無関係だけど一応適用
・エラー1回のみとダイアログ表示の共通化で「ファイル名に使えない文字」エラーにも適用
>sakura.exe * * * * * * とかやるとエラーを閉じるのも大変です。

「ファイル名に使えない文字」の変更は仕方ないですよね。（何故か自明扱い

個人的には >sakura.exe * * * * * * は「やる奴が悪い」と思うんですが、そうも言ってられない利用環境の人もいると思うので必要あれば別途対応がよいと思います。

なんかよく分からない感じですが、一応公開してみます。

作ってみたPRを自分がレビューする立場でみてみると「これ何？おいしい？」って聞きたくなると思います。

AppVeyorBot · 2020-10-03T17:57:07Z

✅ Build sakura 1.0.3135 completed (commit bc3ee755c6 by @usagisita)

beru · 2020-10-05T12:14:32Z

sakura_core/_main/CCommandLine.cpp

 */
-void CCommandLine::ParseCommandLine( LPCWSTR pszCmdLineSrc, bool bResponse )
+void CCommandLine::ParseCommandLine( LPCWSTR pszCmdLineSrc, bool bResponse, bool bError )


この関数って再帰関数なんですね。しかし300行以上あるのは長いですね。。

#1420 の解説にちらっと書いているんですが、パラメータ解析のロジックを自作するんではなく、ルールだけ定義してコードは自動生成させてしまう、という手法もあるです。

beru · 2020-10-05T12:21:41Z

sakura_core/_main/CCommandLine.cpp

@@ -287,8 +302,8 @@ void CCommandLine::ParseCommandLine( LPCWSTR pszCmdLineSrc, bool bResponse )
 		if( ( bParseOptDisabled ||
 			! (pszToken[0] == '-' || pszToken[0] == '"' && pszToken[1] == '-' ) )){

+			WCHAR	szPath[_MAX_PATH] = {0};


変数のスコープを狭める事は良い事だとは思うんですが、配列のゼロ初期化の頻度が高まりそうだなと思いました。

beru · 2020-10-05T12:25:05Z

sakura_core/_main/CCommandLine.cpp

@@ -300,16 +315,26 @@ void CCommandLine::ParseCommandLine( LPCWSTR pszCmdLineSrc, bool bResponse )
 				//	ファイル名の後ろにあるOptionを解析するため，ループは継続
 				int len = lstrlen( pszToken + 1 );
 				if( len > 0 ){
+					CNativeW cmWork;


szPath に収まるサイズなら cmWork を経由せずに szPath にコピーした方がヒープを使う cmWork 変数を使用せずに済むので効率が良くなると思います。記述がちょっと大変そうですが…。

beru · 2020-10-05T12:37:05Z

sakura_core/_main/CCommandLine.cpp

-					);
-					MessageBox( NULL, msg_str, L"FileNameError", MB_OK);
+					if (false == bError) {
+						LongFilePathErrorMessage(szPath, STR_CMDLINE_PARSECMD1);


ここでも使っているので、関数名を LongFilePathErrorMessage としていますが Long という修飾は要らないのではないかと思いました。

beru · 2020-10-05T12:41:30Z

sakura_core/_main/CCommandLine.cpp

+					else if(false == bError){
+						LongFilePathErrorMessage(cmWork.GetStringPtr(), STR_ERR_FILEPATH_TOO_LONG);
+						bError = true;
+					}


思い付きですが下記のように書く事で行数を減らせて可読性も下げられるので一石二鳥だと思いました。

else if(!bError && (bError = true)) LongFilePathErrorMessage(cmWork.GetStringPtr(), STR_ERR_FILEPATH_TOO_LONG);

可読性も下げられるので

二度見！ 😃

beru · 2020-10-05T12:43:13Z

sakura_core/_main/CCommandLine.cpp

+				else if(false == bError){
+					LongFilePathErrorMessage(pszToken, STR_ERR_FILEPATH_TOO_LONG);
+					bError = true;
+				}


https://github.com/sakura-editor/sakura/pull/1423/files#r499568007 と同様のコメントですがこちらも

else if(!bError && (bError = true)) LongFilePathErrorMessage(pszToken, STR_ERR_FILEPATH_TOO_LONG);

のように書く事で行数を減らせると思いました。

beru · 2020-10-05T12:44:37Z

sakura_core/_main/CCommandLine.cpp

+					if (false == bError) {
+						LongFilePathErrorMessage(szPath, STR_CMDLINE_PARSECMD1);
+						bError = true;
+					}


https://github.com/sakura-editor/sakura/pull/1423/files#r499568007 と同様のコメントですがこちらも

if (!bError && (bError = true)) LongFilePathErrorMessage(szPath, STR_CMDLINE_PARSECMD1);

のように書く事ですっきりすると思います。

sakura-editor#1423

usagisita · 2021-03-12T14:23:43Z

#1453 で対処されたのでこちらは閉じておきます。

コマンドラインでMAX_PATHを超えるファイル名を指定すると落ちる

90f7247

長いときはエラーメッセージを表示するようにエラーメッセージは最初の一回のみ表示

usagisita commented Oct 3, 2020

View reviewed changes

beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Oct 4, 2020

beru reviewed Oct 5, 2020

View reviewed changes

berryzplus added a commit to berryzplus/sakura-editor that referenced this pull request Oct 12, 2020

エラーがたくさんあるとき対策で1つのみ表示するように実装

1122750

sakura-editor#1423

berryzplus mentioned this pull request Nov 1, 2020

サクラエディタのコマンドライン引数に超長い文字列を指定するとクラッシュする問題に対処する。(Take2) #1453

Merged

usagisita closed this Mar 12, 2021

usagisita deleted the feature/fix_cmdline_limit_max_path_length branch March 12, 2021 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

コマンドラインでMAX_PATHを超えるファイル名を指定すると落ちる #1423

コマンドラインでMAX_PATHを超えるファイル名を指定すると落ちる #1423

usagisita commented Oct 3, 2020

usagisita Oct 3, 2020

beru Oct 5, 2020

usagisita Oct 3, 2020

beru Oct 5, 2020 •

edited

Loading

berryzplus commented Oct 3, 2020

AppVeyorBot commented Oct 3, 2020

beru Oct 5, 2020

berryzplus Oct 8, 2020

beru Oct 5, 2020

beru Oct 5, 2020

beru Oct 5, 2020

beru Oct 5, 2020 •

edited

Loading

berryzplus Oct 8, 2020

beru Oct 5, 2020

beru Oct 5, 2020

usagisita commented Mar 12, 2021

コマンドラインでMAX_PATHを超えるファイル名を指定すると落ちる #1423

コマンドラインでMAX_PATHを超えるファイル名を指定すると落ちる #1423

Conversation

usagisita commented Oct 3, 2020

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

テスト内容

テスト1

PR の影響範囲

関連 issue, PR

参考資料

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

berryzplus commented Oct 3, 2020

AppVeyorBot commented Oct 3, 2020

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

beru Oct 5, 2020 • edited Loading

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 12, 2021

beru Oct 5, 2020 •

edited

Loading

beru Oct 5, 2020 •

edited

Loading