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.

Sign up for GitHub

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

Jump to bottom

CNativeW::AllocStringBufferの失敗時にコピーを中断させる #1732

Merged
merged 3 commits into from
Oct 4, 2021
Merged

CNativeW::AllocStringBufferの失敗時にコピーを中断させる #1732

merged 3 commits into from
Oct 4, 2021

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented Oct 2, 2021 •
edited
Loading

PR の目的

不具合を修正します。

カテゴリ

  • 不具合修正

PR の背景

#1575 で報告された不具合の対策 #1710 の焼き直しです。

発生事象

大きなデータをコピーしようとすると固まる。
（データが2GBを超えているかどうかは関係ありません。）

原因

選択範囲のデータを取得するコードで
メモリ確保の失敗が考慮されていません。

CNativeWはメモリ確保に失敗するとバッファが解放されるので、メモリの事前確保が成立せずに超低速になります。

PR のメリット

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

  • 修正量が増えるので横展開を行いません。
    横展開要否を検討すべき箇所が、他に30箇所あります。

仕様・動作説明

仕様変更/機能追加はありません。

対策

メモリ確保が失敗した場合に、データ取得を異常終了させる修正を行います。

PR の影響範囲

  • コピー、カット、ドラッグアンドドロップなど、クリップボードに関連する機能。
  • サクラエディタ上の変換機能

テスト内容

テスト1

#1575 の再現手順で、アプリが固まる不具合が解消することを確認します。

手順

  1. このPRをfetchし、x64ビルドします。
  2. ビルドした sakura.exe を実行し、64bit版で2GBより大きいサイズのテキストのコピペをする事が出来ない #1575 に添付された大きなテキストファイルを開きます。
    ファイルを開くまでにかなり時間がかかります。
  3. Ctrl+A、Ctrl+Cの順にキー押下します。（すべて選択、コピー）

アプリが無応答になる事象が解消されているのを確認できます。

関連 issue, PR

#1710
#1575

参考資料

usagisita
usagisita reviewed Oct 2, 2021
View reviewed changes
sakura_core/view/CEditView.cpp Outdated
@@ -1883,6 +1883,11 @@ bool CEditView::GetSelectedData(
cmemBuf->AllocStringBuffer(nBufSize);
//>> 2002/04/18 Azumaiya

// メモリ確保に失敗したら抜ける
if( 0 == cmemBuf->GetStringLength() ){
Copy link
Contributor

Choose a reason for hiding this comment

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

cmemBufはこの時点で空文字列なので、GetStringLength()は必ず0ではないのですか。
下も同上。
ついでに書いておきますが、正規表現で長さ0マッチとコピーを組み合わせると、長さ0の(\0のみの)文字列をコピーできるので「文字列の長さが0だった場合」を判断材料にするのは、よろしくないと思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

capacityのほうをみないといかんですね。

@AppVeyorBot
Copy link

✅ Build sakura 1.0.3927 completed (commit e258ada82b by @berryzplus)

usagisita
usagisita reviewed Oct 2, 2021
View reviewed changes
Copy link
Contributor

@usagisita usagisita left a comment •
edited
Loading

Choose a reason for hiding this comment

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

int nBufSize = wcslen(WCODE::CRLF) * (Int)i;

そもそもこの変数、intなので特にx64で正しく計算できないときありますね。
AllocStringBufferにマイナスを指定した時どういう動作をするかは分かりませんけど。

ああ、でも(2GBを超えているかどうかは関係ありません)と書かれているので、今回のPRはその範囲ではないということなのかな。

@beru beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Oct 2, 2021
@berryzplus
Copy link
Contributor Author

ああ、でも(2GBを超えているかどうかは関係ありません)と書かれているので、今回のPRはその範囲ではないということなのかな。

そうなんですよ。32bitでも搭載メモリが少なければ発生したはずの不具合を対処する内容です。
x64対応の観点が入ると修正行数が増えて正しい判断ができなくなるおそれがあるので、あえて修正範囲を極小化しています。
（その結果、SonarCloudから変な指摘が入りましたがｗ）

@berryzplus berryzplus marked this pull request as draft October 2, 2021 08:31
@berryzplus
Copy link
Contributor Author

int nBufSize = wcslen(WCODE::CRLF) * (Int)i;

そもそもこの変数、intなので特にx64で正しく計算できないときありますね。 AllocStringBufferにマイナスを指定した時どういう動作をするかは分かりませんけど。

この件は既知の問題で、どうすべきか検討しないといかんです。
CNativeW::AllocStringBufferの引数は既に size_t になっているので、nBufSizeがマイナス値の場合は正負反転してものすごく大きなバッファを確保しようとすることになり、通常はメモリ確保に失敗すると思います。

@AppVeyorBot
Copy link

✅ Build sakura 1.0.3929 completed (commit 5af6d409f6 by @berryzplus)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 2, 2021

Kudos, SonarCloud Quality Gate passed!    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.3931 completed (commit dddfc3bf43 by @berryzplus)

@berryzplus berryzplus marked this pull request as ready for review October 2, 2021 10:47
@berryzplus
Copy link
Contributor Author

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

@berryzplus berryzplus merged commit 1ff0b48 into sakura-editor:master Oct 4, 2021
@berryzplus berryzplus deleted the feature/fix_hung_on_huge_copy branch October 4, 2021 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@usagisita usagisita usagisita left review comments

@beru beru beru left review comments

@sanomari sanomari sanomari approved these changes

Assignees
No one assigned
Labels
🐛bug🦋 ■バグ修正(Something isn't working)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@berryzplus @AppVeyorBot @beru @usagisita @sanomari