Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grepダイアログのフォルダ名設定処理でのバッファオーバーラン修正 #1588

Conversation

usagisita
Copy link
Contributor

PR の目的

見た感じ SetGrepFolder関数のWCHAR[MAX_PATH] の変数で長さを確認せずに、前後をクオートで囲う処理をしていて、パスがぎりぎりの長さの場合にはバッファに入りきりません。
それを修正します。

カテゴリ

  • 不具合修正

PR の背景

PR のメリット

バグが減ります。

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

特にありません。

仕様・動作説明

「現フォルダ」ボタンやフォルダ参照を押すと、パスがエディットコントロールに設定されます。
そのとき、パスに;(セミコロン)が含まれているときはセパレーターと区別するためにパス全体をダブルクォートで囲う処理があります。
これがコード上はバッファオーバーランするように見えるのを、直します。

PR の影響範囲

C言語の文字列操作関数は止めて、可変長のCNativeWにしました。パスが長くても安心です。
変更は関数内のみです。

テスト内容

テスト1

手順

長いフォルダのパスはエクスプローラーやCMDでもうまく作れなかったので未確認です。
普通の;セミコロンを含むパスをダブルクォートで囲う処理が動作することは、目視の動作確認で一応しました。

関連 issue, PR

参考資料

@AppVeyorBot
Copy link

Build sakura 1.0.3553 completed (commit e4ea1a62cf by @usagisita)

berryzplus
berryzplus previously approved these changes Mar 14, 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.

問題ないと思います。

本来的にはCNativeW型の利用は「必要なときのみ」に控えるべきと思います。

■ CNativeWが必要なケース

  • CCodeBase派生クラスでANSI文字列から変換したデータを格納したい場合。
    変換失敗時の独自エンコードの仕様があるので std::wstring で代替できません。
  • 「指定なし」を示す nullptr を格納したい場合。
    std::optional<std::wstring> で代替可能ですが 実装修正がややめんどうです。

@sanomari
Copy link
Contributor

このままで問題ないと思いますが、std::wstringを使うようにしたほうが良さそうに思いました。

@usagisita
Copy link
Contributor Author

コメント、確認ありがとうございます。
approved いただいていますが、確かに自分もstd::wstringのほうが良いと思いますので、書き直したいと思います。

@sonarcloud
Copy link

sonarcloud bot commented Mar 15, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

Build sakura 1.0.3558 completed (commit e957961b07 by @usagisita)

wcscat( szQuoteFolder, L"\"" );
::SetWindowText( hwndCtrl, szQuoteFolder );
std::wstring strQuoteFolder;
strQuoteFolder = std::wstring(L"\"") + folder + std::wstring(L"\"");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
strQuoteFolder = std::wstring(L"\"") + folder + std::wstring(L"\"");
strQuoteFolder = strprintf( LR"("%s")", folder );

というのもアリかと思いましたが、既存コードではこっちのが多数派ですしどっちでも良さそうに思います。

@usagisita
Copy link
Contributor Author

レビューありがとうございます。
マージしてしまいます。

@usagisita usagisita merged commit 8f92e0b into sakura-editor:master Mar 16, 2021
@usagisita usagisita deleted the feature/fix_buffer_grep_dlg_set_folder branch March 16, 2021 15:22
@beru beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug🦋 ■バグ修正(Something isn't working)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants