Skip to content

Conversation

@usagisita
Copy link
Contributor

PR の目的

細工されたり\\?\形式の長いパスのGrep結果のタグジャンプで「◆■」がある形式でバッファオーバーフローが発生する不具合の修正です。
同一箇所として、下記の「◎◆■」のタグで、存在しないパスのタグがあると、それに無条件でジャンプしようとする不具合を修正します。

カテゴリ

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

PR の背景

strcat/strcpy、固定長バッファの一連の点検作業の一部です。

他にも、バグっぽい挙動をする部分が見られましたが、今回はスルーしました。
さらに不具合など修正リファクタリングしたコードも手元にはありますが、単体テストがないため、修正確認が非常に面倒です。
とりあえず、第1弾としたいです。
本格的にリファクタリングしたいなら、どうぞ、お願いします。

PR のメリット

バグが修正されます。
細工されたGrep結果のタブジャンプでクラッシュしにくくなります。

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

例のごとく、自動テストがないため、新しい不具合が出る可能性があります。

関数の中身が煩雑で、処理を追うのも難しいため、目で検証するのも大変だと思われます。

仕様・動作説明

テストの通りです。

ファイル名の固定配列をstd::wstringで置き換えました。
関連するサブ関数をwstring仕様に差し替えました。

GetQuoteFilePathではバッファ長指定が_countof()なのでszPathszFileは最大MAX_PATHまでコピーを切り出してきます。
AddLastYenFromDirectoryPath()szPathMAX_PATH限界だと1文字はみ出します。
wcscatszPathMAX_PATHszFileも制限はMAX_PATHなので長さを確認しないなら倍は必要です。

AddLastYenFromDirectoryPath( szPath );
}
wcscat( szPath, szFile );

下記も1024文字と長いですが、条件に一致すればバッファオーバーフローします。
またパス文字列の中身をチェックする前にszJumpToFileにコピーしてしまっているため、IsFileExists2()falseになるルートでは本来、タグジャンプ不可のはずが、不正パスでもタグジャンプ扱いになっていました。

if( GetQuoteFilePath( &pLine[2], szJumpToFile, _countof(szJumpToFile) ) ){
AddLastYenFromDirectoryPath( szJumpToFile );
wcscat( szJumpToFile, szFile );
if( IsFileExists2( szJumpToFile ) ){

元のコードから、カーソル行の解析ではif (!GetQuoteFilePath())と否定ですが、後半の上の行にさかのぼる処理のほうはif (GetQuoteFilePath())と肯定になっていますので、注意してください。

PR の影響範囲

Grep結果からのタグジャンプに関する範囲のみです。

テスト内容

テスト1

手順
「ファイル毎」「フォルダ毎」でGrepした結果のテキストを細工します。

■"C:\Users\USERNAME\Documents\git\sakura_rep\sakura_core\dlg"
◆"CDlgOpenFile_long_012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890.cpp" [UTF-8]
・(   967,6    ): grep_result

サクラエディタに上記を貼り付け「◆」か「・」の行にカーソルを置き「F12」を押します。
 →フォルダ+ファイルがstrcatで連結されてバッファオーバーフローしていました。
デバッグビルドで実行してジャンプさせるとスタックのエラーかフリーズすることがあります。
修正後は、タグジャンプできなくなります。
(パスのファイルは実在している必要はありません)

テスト2

手順
「ファイル毎」「フォルダ毎」「ベースフォルダ」でGrepした結果のテキストを細工します。

◎"C:\Users\USERNAME\Documents\git\sakura_rep\sakura_core"
■"dlg"
◆"CDlgOpenFile_xxxxx.cpp"  [UTF-8]
・(   967,6    ): grep_result
◎"C:\Users\USERNAME\Documents\git\sakura_rep\sakura_core"
■"d*g"
◆"CDlgOpenFile_xxxxx.cpp"  [UTF-8]
・(   967,6    ): grep_result

サクラエディタに上記を貼り付け「◆」か「・」の行にカーソルを置き「F12」を押します。
 →存在しないファイルとして新規で開かれてしまうバグがありました。
修正するか非常に迷いましたが、同一の箇所であり、ファイル名、フォルダ名に*や?など任意の文字があってもタブジャンプしてしまうため、これはPRに含めました。

Grep結果には、オプションが「ノーマル/ファイル毎」「フォルダ毎」「ベースフォルダ」と全部で8パターンあります。

関連 issue, PR

参考資料

@AppVeyorBot
Copy link

Build sakura 1.0.3607 completed (commit c604f923e0 by @usagisita)

@berryzplus
Copy link
Contributor

berryzplus commented Mar 27, 2021

Bug D 1 Bug

修正範囲外なのでレビュー対象外とするのが妥当と思います。
この不具合(?)は別途対応したほうがよいのかもです。

Code Smell C 6 Code Smells

CodeSmellsのほうも、対策が難しい指摘ばかりです。

  • 複雑度が 168 です。 25 を目標にリファクタリングしてください。
  • if/for/do/while/switch の入れ子が3レベルを越えています。×3
  • 結合して1つにできるif文があります。

対策できるとしても if 文のやつだけですね・・・。

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR趣旨は了解です。

「if等のネストレベルが3を超える」の警告が、今回作り込んだもののようなので、そこの対策がいるんじゃないかと思います。

サクラエディタには AddLastYen みたいな関数が多数定義されていて、このPRの修正で std::wstring バージョンを追加している感じなんですけど、個人的に「あんまりよくない」と思っています。

あんまりよくないと思う理由は2点あります。

  • LastYen という名前が変。
    LastYenは「パス文字列の末尾にあるパス区切り記号」を意味します。
    「Windowsにおけるパス区切り記号」は back slash(0x5C) で、字形は ⧵ です。
    歴史的な理由により、日本語Windowsではこの文字が ¥ に見えます。
    パス文字列の見た目から「最後の円マーク」で LastYen なんだと思いますが、実際の処理対象は「末尾のパス区切り文字」なので、ちょっと違います。
    AddLastBackslash(英語向け) とか AddLastWon(韓国語向け)とか作るんかい!になります。
  • 処理対象が「ただの文字列」じゃないような気がします。
    C言語の wchar_t* は生のデータなので属性を持たせられませんが、C++で処理を組むなら「文字列」と「パス文字列」を区別できるような気がします。この系統の自作関数を用意することは否定しませんけれども、C++のシステムライブラリを活用したほうが良いように思います。

@usagisita
Copy link
Contributor Author

LastYenは元の関数からの命名ですね。
自分は命名および英語にセンスがない自覚があるので、AddLastBackslash以上のものは思いつきません。
AddLastDirectorySeparatorぐらいですが、これも不満はあります。
この文字列はテスト2の「■"dlg"」のような「ディレクトリパスの真ん中」であることがあるので、できれば「パス操作」ではなく「明確で単純な文字列操作レベル」であることを主張したいです。
そのためDirectoryという実在パスか確認しそうな名前を避けました。
何かよりよい名前、(もしあれば処理方法なども)があれば、提案いただきたく思います。

@berryzplus
Copy link
Contributor

すみません、ややこしくなっていますが、修正依頼は以下の1点のみです。

「if等のネストレベルが3を超える」の警告が、今回作り込んだもののようなので、そこの対策がいるんじゃないかと思います。

AddLastYenの件は感想なので、このままで良いです。

@usagisita
Copy link
Contributor Author

オーキードーキー
if/for/whileのネストレベル3段も、if文の統合も元からのコードです。
ネスト段数などは変わっていません。
ifが統合できるについては、簡単だったので対応しました。

@usagisita
Copy link
Contributor Author

すみません、再び実行テストをしてみたところ、タブジャンプができない状態になっているオプションのパターンがあるようです。
どこかで作業ミスがあると思います。
しばしお待ちください。すみません。確認します。

@AppVeyorBot
Copy link

Build sakura 1.0.3611 completed (commit 26f62acf2d by @usagisita)

@AppVeyorBot
Copy link

Build sakura 1.0.3613 completed (commit d608e02bab by @usagisita)

@berryzplus berryzplus dismissed their stale review March 28, 2021 11:21

対応確認したので。

berryzplus
berryzplus previously approved these changes Mar 31, 2021
Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

判断できずに放置してしまっていました。
対応したかった問題の対処はできているように思うので結論出しておきます。

@beru
Copy link
Contributor

beru commented Mar 31, 2021

何かよりよい名前、(もしあれば処理方法なども)があれば、提案いただきたく思います。

AppendPathSeparator という名前も長いですが考えられます。

なお基本的に英語圏出身の人達同士で作ってきたソフトじゃないし、ピジン英語での命名でも No hay problema ではないかと。

GetLineColumn( &pLine[2 + nPathLen], &nJumpToLine, &nJumpToColumn );
break;
}else if( !GetQuoteFilePath( &pLine[2], szFile, _countof(szFile) ) ){
}else if (strFile = GetQuoteFilePath(&pLine[2], MAX_TAG_PATH); strFile.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if 文の中に変数への代入とその変数を使った条件式をまとめて書くと少し読みづらいと感じるのは自分が古いからかもしれません。きっと strFile 変数に代入するのは後でその値を使用するからだと思います。この関数は元々長くて読み取りづらいのでブラウザで見る差分表示で全体像を把握するのは難しそうです。。

思いついた方法として GetQuiteFilePath 関数に std::wstring& 型の引数を追加して戻り値の型も std::wstring& 型に変更する事で

}else if (GetQuoteFilePath(&pLine[2], MAX_TAG_PATH, strFile).empty()) {

のようにセミコロンで区切らずに書けると思います。更に GetQuiteFilePath を関数ではなくラムダ式にすれば引数で strFile を渡さずに済みます。ただそういう風な記述もそれはそれでトリッキーな感じがするので、その書き方に変えて本当に可読性が向上するのかというと、あくまで好みの問題かもしれません。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ごもっともだと思うので、どうしようか考え中です。
記述としては、元のコードに近いものに戻るということですね。
戻り値もstd::wstring&からさらに戻して、empty()かどうかをboolで返したほうが、記述そのものはシンプルになりそうですが、どうするかは考えようです。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetQuoteFilePath 関数の最後の引数の size にはすべての呼び出し箇所で MAX_TAG_PATH しか渡していないのもなんか気になりますね。まぁでも構造化を怠ってずるずる実装してきた果てに引数の数が数十になっている酷い関数というわけでは全然無いし、細かい事は気にしない方が良いですね。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else if で変数宣言してるのが違和感の正体かな?と思いました。
直前のifのtrueパートはbreakで抜けてそうなので、else ifにしてるのが違和感の原因なのではないかと思います。

@beru beru added refactoring リファクタリング 【ChangeLog除外】 🐛bug🦋 ■バグ修正(Something isn't working) labels Mar 31, 2021
@usagisita usagisita force-pushed the feature/fix_buffer_tagjump_long_name branch from 3ea9e8b to feaa552 Compare April 4, 2021 16:36
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 4, 2021

@AppVeyorBot
Copy link

Build sakura 1.0.3641 completed (commit 98ac1a3fe5 by @usagisita)

@usagisita
Copy link
Contributor Author

if 文の中に変数への代入とその変数を使った条件式

指摘点を対応してみました。
if文の前への変数宣言に戻しましたが、Code Smellが出ています。

if (std::wstring strPath; GetQuoteFilePath(&pLine[2], strPath, MAX_TAG_PATH)) {

こういう風に書くこともできますが、どうしましょうか……

@beru
Copy link
Contributor

beru commented Apr 6, 2021

if 文の中に変数への代入とその変数を使った条件式

指摘点を対応してみました。
if文の前への変数宣言に戻しましたが、Code Smellが出ています。

if (std::wstring strPath; GetQuoteFilePath(&pLine[2], strPath, MAX_TAG_PATH)) {

こういう風に書くこともできますが、どうしましょうか……

これに関しては究極的には好きな方で良いと思います。不具合ではないのでコメントはしますが強制はしません。

Code Smell を確認しました。Why is this an issue? をクリックすると説明が出てきました。

https://sonarcloud.io/organizations/sakura-editor/rules?open=cpp%3AS6004&rule_key=cpp%3AS6004

C++17以降ではスコープを狭められるから使った方が良いよ、っていう文章でした。最初のコード例では std::unique_lock<std::mutex> 型を使った例がありますが、この場合は確かに使った方が良いですね。

なお、可読性が落ちるけど新しい言語仕様で入った記法を使わないと駄目だよと仮に強制されたら自分はそれは理不尽だと思います。こういうコーディングスタイルに関しては自分は自由に任せたら良いと思いますね。実際に不具合があるかないかとかに比べたら些細な事だと思いますし、個人個人で書き方の癖や好みって違うと思うので。

@berryzplus
Copy link
Contributor

berryzplus commented Apr 9, 2021

#1607 (comment) に関して。

Use the init-statement to declare "strPath" inside the if statement.
https://sonarcloud.io/project/issues?id=sakura-editor_sakura&issues=AXid1kifcR4UKLHBEfcZ&open=AXid1kifcR4UKLHBEfcZ&pullRequest=1607

if (std::wstring strPath; GetQuoteFilePath(&pLine[2], strPath, MAX_TAG_PATH)) {

こういう風に書くこともできますが、どうしましょうか……

「正しい」対策はおそらく、以下のようなコードです。

if ( auto strPath = GetQuoteFilePath(&pLine[2], MAX_TAG_PATH); strPath.length() > 0 ) {

・・・要するに、GetQuoteFilePathのシグニチャを変えにゃならんです。

GetQuoteFilePathのシグニチャを再定義するのは、明らかにめんどくさい作業なので「めんどくさいので対応しません」と言っても普通に「そうだね。」ってなると思います。

というか、話の流れがわからないです。

Get Quote FilePath って関数名も意味不明だと思います。

  • Get = 動詞、取得する、意味。
  • Quote = 動詞、囲う、の意味。
  • FilePath = 名詞、ファイルパスの意味。

処理内容が「囲う」ならばGetなしでQuoteFilePathだし、
処理内容が「取得する」ならQuoted(=囲われた)とするのが自然です。

いや、やり取りちゃんと読めよ!wって話かも知れませんけれども。

@berryzplus
Copy link
Contributor

もしかして、TryQuoteFilePathなんですかね?
C# なら bool TryQuoteFilePath( string, out string ) みたいな関数を書けますが、そういうノリならネーミングも引数型も理解できるっす。

@beru
Copy link
Contributor

beru commented Apr 9, 2021

というか、話の流れがわからないです。

#1607 (comment) で if 文の中に変数への代入とその変数を使った条件式をまとめて書くとわかりづらいといったコメントを自分がして、そうしないように usagisita さんが変更したらSonarCloud の Code Smell が出たという流れだと思います。#1607 (comment)

こんな事を書くと白い目で見られそうですがPRのレビューでSonarCloudの指摘事項に対応するかどうかを考えるコストが無駄な気もします。自分の感覚ではSonarCloudよりusagisitaさんの方が不具合検知能力が高いので、捕獲後に科学者達が脳を解析して人類の未来に役立

Get Quote FilePath って関数名も意味不明だと思います。

同じ事は自分も思いましたが、このPRで追加された関数じゃないし変更する必要は無いと思いますね。もちろん変更するのがいけないというわけではないですが。

66ee57f#diff-040dfa00369971e38c307d19fe43f4a18bd8cf7dd258d687eb805fcf731b7baaR42 で追加されてますね。

@berryzplus
Copy link
Contributor

というか、話の流れがわからないです。

#1607 (comment) で if 文の中に変数への代入とその変数を使った条件式をまとめて書くとわかりづらいといったコメントを自分がして、そうしないように usagisita さんが変更したらSonarCloud の Code Smell が出たという流れだと思います。#1607 (comment)

SonarCloudの指摘は「スコープを狭められる変数宣言は狭いほうに寄せたほうがいいぜ?」な意図だと思います。読みづらい場合は、改行を入れたり変数名や関数名を短くするなどして対応すればよいと思います。

この手の検出を防ぐには関数先頭に宣言をまとめたらよいのではないかと思います。その場合、現代人の感覚に反するおかしなコードになるので、人間による恣意的なレビューで指摘を浴びることになると考えられますが 😃

こんな事を書くと白い目で見られそうですがPRのレビューでSonarCloudの指摘事項に対応するかどうかを考えるコストが無駄な気もします。自分の感覚ではSonarCloudよりusagisitaさんの方が不具合検知能力が高いので、捕獲後に科学者達が脳を解析して人類の未来に役立

白い目w

usagisitaさんやberuさんの嗅覚が鋭いことに疑いはありませんけど、それとコレとは別です。
人間の異常検出能力をアテにしなくてもよいように導入した静的解析っす。

機械による検出は、指摘基準がブレません。
指摘基準がブレないことは、とても重大なメリットだと思います。
一定の基準に従った不審点を、漏れなく検出してくれます。
人間がレビューしたら、そうはいかないです。
(「何度も同じこと言わすな、○○が!」とか汚い言葉が飛び交う世界はイヤっす。)

楽するためのものなので、一律に対応してゆけばよいだけです。
明らかな誤検出もあるので、そういうのは人間が判断したらよいです。
対応があまりにめんどくさい場合は、あとまわしにするのも選択肢です。

Get Quote FilePath って関数名も意味不明だと思います。

同じ事は自分も思いましたが、このPRで追加された関数じゃないし変更する必要は無いと思いますね。もちろん変更するのがいけないというわけではないですが。

結局、このPRの残件はなんでしょう?

  • 目的とした改修を達成できているか?
  • 「新たな不具合」を作り込んでいないか?

この2点がクリアできていたら残件なしとしてよい認識です。

Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

問題無いと思います。

PRの説明のテスト内容に書かれているような方法でGrep結果を加工したテキストを用意して動作確認しました。

修正前は存在しないファイルを新規エディタで開く変な挙動ですが、このPRではタグジャンプ出来なくなる事を確認しました。

確認に使用したテキスト

□検索条件  "_IsFileUpdatedByOther"
検索対象       *.h
フォルダ       D:\projects\sakura\sakura_core
除外ファイル   *.msi;*.exe;*.obj;*.pdb;*.ilk;*.res;*.pch;*.iobj;*.ipdb
除外フォルダ   .git;.svn;.vs
    (サブフォルダも検索)
    (英大文字小文字を区別しない)
    (文字コードセットの自動判別)
    (一致した行を出力)


■"D:\projects\sakura\sakura_core"
◆"CAutoReloadAgent.h"  [UTF-8]
・(    58,7    ): 	bool _IsFileUpdatedByOther(FILETIME* pNewFileTime) const;
◆"CAutoReloadAgent_0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456012345678901234567890123456789.h"
・(    58,7    ): 	bool _IsFileUpdatedByOther(FILETIME* pNewFileTime) const;

色々な組み合わせのパターンでは動作確認はしていません。面倒くさくて…。

@beru
Copy link
Contributor

beru commented Apr 9, 2021

SonarCloudの指摘は「スコープを狭められる変数宣言は狭いほうに寄せたほうがいいぜ?」な意図だと思います。

SonarCloudの指摘内容は自分もそういう内容だと読みました。ただし if 文の中に変数宣言や代入を書いてスコープを狭められる事に利点はあるにしても可読性が落ちる事は無いですか?可読性が落ちる事をSonarCloudは判断材料に入れてないのかもしれませんが…。

feaa552 はこのPRのusagisita さんの commit ですが、if 文の中で GetQuoteFilePath 関数を呼んでいる箇所で左と右のどちらが読みやすいですか?

読みづらい場合は、改行を入れたり変数名や関数名を短くするなどして対応すればよいと思います。

うーん、具体的にどのように書けば先ほどの記述が読みやすくなるんでしょうか?どのように途中で改行を入れたり変数名や関数名を短くするなどすれば読みやすくなるんでしょうか??

とはいえ、if 文の中で変数宣言や変数への代入とその変数を使って条件判定をする記述があっても可読性は落ちない!と主張する人も世の中にはいるかもしれません。またコードゴルフや4k introをやっている人達からしたら高級言語の記述で揉めるのが意味不明かもしれません。

この手の検出を防ぐには関数先頭に宣言をまとめたらよいのではないかと思います。その場合、現代人の感覚に反するおかしなコードになるので、人間による恣意的なレビューで指摘を浴びることになると考えられますが 😃

PascalやC90的な書き方は嫌ですが、究極的には問題ではないかもしれません。

usagisitaさんやberuさんの嗅覚が鋭いことに疑いはありませんけど、それとコレとは別です。
人間の異常検出能力をアテにしなくてもよいように導入した静的解析っす。

今はまだ人間みたいな判断は出来ないですよね。
人間の補助になるようにうまく使っていくのは良いと思いますが、あまりあてにできる代物ではないなと思います。
半世紀ほど経てば大分良くなっていると思いますがその前に寿命が来てしまいそう:ghost:

@sanomari
Copy link
Contributor

何か残件ありますか?

そろそろ放置で2週間経過します...

@usagisita
Copy link
Contributor Author

放置していてすみません。

色々不満点は残っていますが、致命的な残件そのものはないという認識です。
その辺無視してマージしていいか判断に迷っていたので、放置していました。

これってPR作成者が自分で判断して、マージしなければならないんですかね?
この状態でいいのであれば、そのままマージしてしまいたい気持ちが強いです。
このPRの対象部分では、どのみち本格的な複雑度に関する対応など別で対処が必要そうというのもあります。

@beru
Copy link
Contributor

beru commented Apr 22, 2021

これってPR作成者が自分で判断して、マージしなければならないんですかね?

PR作成者が自分でマージしてもOKだし、他のマージ出来る人がマージしてもOKです。

この状態でいいのであれば、そのままマージしてしまいたい気持ちが強いです。
このPRの対象部分では、どのみち本格的な複雑度に関する対応など別で対処が必要そうというのもあります。

複雑なコードは至る所にあって対処が難しいですね。力技なのか秘伝のタレなのかスパゲッティなのか、まぁ明日の献立を考えるのは止めておきます。

@beru beru merged commit b8c3830 into sakura-editor:master Apr 22, 2021
dep5 added a commit to dep5/sakura that referenced this pull request Jun 18, 2021
dep5 added a commit to dep5/sakura that referenced this pull request Jun 18, 2021
@usagisita usagisita deleted the feature/fix_buffer_tagjump_long_name branch October 2, 2021 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛bug🦋 ■バグ修正(Something isn't working) refactoring リファクタリング 【ChangeLog除外】

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants