バックアップで詳細$0～$9指定を使いMAX_PATH近いパスを保存しようとすると落ちる不具合を修正

[usagisita]

タイトルの通りです。

PR の目的

バックアップをしようとすると落ちる不具合の修正です。

カテゴリ

• 不具合修正

PR の背景

異常終了は減らしたいです。

PR のメリット

バックアップで不用意に落ちなくなります。

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

特にないです。

仕様・動作説明

ソースコード上では、auto_strlcatによるコメントが書かれていましたが、この箇所でMAX_PATHを超えると、サクラエディタが強制終了することがあります。

PR の影響範囲

バックアップのファイル名作成の範囲でエラーになります。

テスト内容

テスト1

手順
「バックアップ」を有効にして「詳細設定」を選び「$0_back_long_long_name.*」等と入力します。
「指定フォルダに作成する」で「.\skrtmp」等とファイルに対して有効な260ぎりぎりのパスを入力しておきます。
C:\Users\abc\Documents\git\sakuratmp\test_long_path\long_245_path_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa012345\b.txt
などのファイルパスに対して、バックアップ作成を実行します。
「元ファイルのフォルダ+サブフォルダ」が有効なパスの場合で、
元ファイルのフォルダ+サブフォルダのパス+新ファイル名が260を超えると、バッファオーバーランが発生します。

パッチ適用後は、wcsncat_sで検出されて、エラー画面が正常に表示されるようになります。

関連 issue, PR

参考資料

[AppVeyorBot]

✅ Build sakura 1.0.3570 completed (commit 9519101dc5 by @usagisita)

[berryzplus]

バックアップをしようとすると落ちる不具合の修正です。

これは、実際に落ちるんですかね？

バッファオーバーフローの嫌なところは、落ちるとは限らないところです。
実際に確実に落ちるのであればバッファオーバーフローではなく、パス長が _MAX_PATH を超過したことによるファイルオープン失敗などが原因ではないかと思います。

関数の使い方が誤っているのを修正　→　必ずしも動作確認不要。
関数の使い方を誤った結果で落ちる不具合を修正　→　動作確認必須。

手動の動作確認を要求するPRはレビューされない傾向にあります。

そして、不具合として修正するなら、明らかな誤字脱字を除いて原因を明確にすべきと思います。

[beru]

少し古いですが e25539b で動作確認してみました。Debug ビルドをデバッグ実行してテストしてみたところ開発環境の画面に Run-Time Check Failure #2 - Stack around the variable 'szPath' was corrupted. というメッセージが表示されました。落ちる現象は条件を満たしてないのか確認できませんでした。

[beru]

問題無いと思います。

Debug実行で動作確認しました。スタック破壊の不具合が無くなりました。

wcsncat_s の戻り値は 0 が成功という事で入力によっては 0 と STRUNCATE 以外の値が返る事もあるようですが、おそらくこの実装ではそういう入力は渡さないので扱わないで問題無いと思います。

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strncat-s-strncat-s-l-wcsncat-s-wcsncat-s-l-mbsncat-s-mbsncat-s-l?view=msvc-160

[sanomari]

1 Code Smell

見落としました。

今回新たに作ったものだから対応するのが筋だと思います。

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[berryzplus]

修正内容がif文の結合とポインタ値の比較相手をnullptrに直してるだけなのを見ました。

細かいことは気にしない方向でレビューするなら approve が妥当と思います。

[AppVeyorBot]

✅ Build sakura 1.0.3577 completed (commit b9f244cee6 by @usagisita)

[AppVeyorBot]

✅ Build sakura 1.0.3578 completed (commit aeb903920f by @usagisita)

[usagisita]

確認ありがとうございます。
テストを兼ねてバックアップをオンにしていたら色々なところにバックアップフォルダと沢山のファイルができてました……
マージしようと思います。