SonarScanで検出されたStaticVectorの範囲外アクセスを修正する

[berryzplus]

PR の目的

SonarCloudにBugだと言われている警告を減らします。

カテゴリ

• 不具合修正

PR の背景

停滞中の #1504 で対応を進められそうなものを見つけたので作ってみました。

SonarCloudで以下のBugが検出されています。

sakura/sakura_core/util/StaticType.h

Line 121 in 0f97078

Me& operator = (const CHAR_TYPE* src){ Assign(src); return *this; }

Returned pointer value points outside the original object (potential buffer overflow)
https://sonarcloud.io/project/issues?id=sakura-editor_sakura&issues=AXcz8fQ7UB5eMyZTW2IH&open=AXcz8fQ7UB5eMyZTW2IH

PR のメリット

SonarCloudのBugsレベル警告が1つ解消します。

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

とくにないと思います。

仕様・動作説明

問題のあるコードはココです。

sakura/sakura_core/util/StaticType.h

Lines 62 to 67 in 0f97078

	void push_back(SET_TYPE e)
	{
	assert(m_nCount<MAX_SIZE);
	m_nCount++;
	m_aElements[m_nCount-1]=e;
	}

m_nCount == MAX_SIZEな場合、
デバッグビルドではassertに失敗して、正常にクラッシュします。
リリースビルドでは66行目を実行してメモリ範囲外に書き込みます。

範囲外アドレスへの書き込みを行った結果がどうなるかは予測不能です。

このPRでは、単体テストを用意することにより、リリースビルドで実際にデータ破壊が発生することを示した後、データ破壊が起こらないように修正する提案を行います。

PR の影響範囲

• 共有メモリ内に保持する配列的データ構造に上限を超える数の要素を追加した場合の挙動に影響する変更です。通常、上限を超える数の要素を追加しないようにチェックしているので、チェック漏れを犯しているコードにのみ影響します。

テスト内容

妥当と思えるテストコードを用意して検証します。
・変更前　テストが失敗します。
・変更後　テストが成功します。

関連 issue, PR

#1504

参考資料

[berryzplus]

azpでリリースビルドのテストが失敗しました（想定通りです。
https://github.com/sakura-editor/sakura/pull/1679/checks?check_run_id=2649192925

[AppVeyorBot]

❌ Build sakura 1.0.3792 failed (commit 8a69775b9f by @berryzplus)

[AppVeyorBot]

✅ Build sakura 1.0.3793 completed (commit 99ad40b03b by @berryzplus)

[beru]

StaticVector はリリースビルドでは範囲外チェックを行わないようにしているのは意図的じゃないかなと思います。operator[] や resize メソッドもそうなので。

ヒープを用いないvector型を用意しているのはその方が軽量だからでしょうね。

[berryzplus]

StaticVector はリリースビルドでは範囲外チェックを行わないようにしているのは意図的じゃないかなと思います。operator[] や resize メソッドもそうなので。

ヒープを用いないvector型を用意しているのはその方が軽量だからでしょうね。

いや、設計仕様ではなく「漏れ」でしょう。

assertが入ってるので不正な呼出しに対してデバッグ処理が止まり、対応できたような気になってたのではないかと思います。リリースではそのまま続行してしまうので実際には危険で「何が起きるか分からない」という最も厄介な不具合を引き起こします。

「ヒープを使わないVector」は、共有メモリの一部に「特定の型の配列」が含まれていて、これに安全にアクセスするために共有メモリのレイアウトと互換性のある疑似Vectorクラスが必要になったから作られた、と理解しています。

疑似Vectorクラスの開発要件は、以下のようなものだったと推定されます。
・生配列と親和性のあるインターフェースを備えていること
・ヒープなどの外部メモリを使用しないこと
・メソッドが例外を投げないこと

よって、軽量化を意図した機能省略が行われたとは考えにくく、純粋に処理が漏れていたものと考えられます。

operator []やresizeのチェック漏れも対処を行うのが「筋」だと思いますが、このPRの目的はSonarScanで検出された潜在不具合の対処なので、関係ない変更は含めていません。

[beru]

いや、設計仕様ではなく「漏れ」でしょう。

うーん、そうなんでしょうか？まぁ明言されていないので真相は闇の中じゃないかと思います。

assertが入ってるので不正な呼出しに対してデバッグ処理が止まり、対応できたような気になってたのではないかと思います。リリースではそのまま続行してしまうので実際には危険で「何が起きるか分からない」という最も厄介な不具合を引き起こします。

おっしゃる通り不正メモリアクセスが行われていると問題になりかねないです。行われていれば、なのでもし行われていなかった場合には余計なチェックになります。ただし追加したチェックの為にどれだけ性能が低下するかというとおそらく微々たるものだと思います（きちんと計測すれば性能低下の度合いが見えるかもしれませんが手間だし…）。

ところで Visual Studio 2019 version 16.9 から AddressSanitizer がサポートされたようなのでデバッグに役立てられそうです。
link
clang とか gcc のように色々な Sanitizer が VC++ でも使えるようになると良いですね。

「ヒープを使わないVector」は、共有メモリの一部に「特定の型の配列」が含まれていて、これに安全にアクセスするために共有メモリのレイアウトと互換性のある疑似Vectorクラスが必要になったから作られた、と理解しています。

疑似Vectorクラスの開発要件は、以下のようなものだったと推定されます。
・生配列と親和性のあるインターフェースを備えていること
・ヒープなどの外部メモリを使用しないこと
・メソッドが例外を投げないこと

よって、軽量化を意図した機能省略が行われたとは考えにくく、純粋に処理が漏れていたものと考えられます。

列挙して頂いた推定内容から「軽量化を意図した機能省略が行われたとは考えにくく」に、どのように繋がるのかよくわかりません。論理の飛躍とまで言ったら失礼かもしれませんが、想像力が働かない自分みたいな杓子定規な人間には行間を読むのが難しいです。開発要件というほど明確に内容を定めてから実装したのかどうかは自分には不明です。冒頭でも書きましたが、コメントで解説されているわけではないのでなんともです…。

operator []やresizeのチェック漏れも対処を行うのが「筋」だと思いますが、このPRの目的はSonarScanで検出された潜在不具合の対処なので、関係ない変更は含めていません。

berryzplusさんがどうしてそれが「筋」だと思うのかは省略して書かれていないので想像してみますが、おそらく足並みを揃えるというか push_back で境界チェック処理を追加するなら他のメソッドにも追加するべきって考えですよね？自分は他のメソッドにも境界チェックは追加されていなかったから StaticVector は意図して（assert以外の）境界チェックを行わないように実装したのだと思いました。

ところで std::vector の operator [] は境界チェックは行わない仕様です。link
std::vector の at は境界チェックを行う仕様で範囲外アクセスの場合には std::out_of_range 例外を送出します。link
これに合わせるとなると StaticVector の operator [] では境界チェックは行わないでも良いとは思います。

サクラエディタの StaticVector の operator [] や resize で境界チェックを行うべきなのかどうかについて考えてみます。
もし境界チェックを行わないなら呼び出し側の方では境界越えが発生しない正常な範囲で呼び出す必要が有ります。

仮に境界チェックを行う場合の話ですが、境界越えの範囲が指定された場合にメモリアクセスは行わないのは前提としてその後にどうするかについてですが、

1. std::out_of_range 例外を送出する。
2. 範囲外が指定されたかどうかを変数(戻り値、引数、グローバル変数、etc...)に記録する
3. 何もしない

が思いつきます。このPRでは3番の何もしない方式ですが、それで問題が起きないのかどうか、呼び出し元では何もしないでよいのかどうかはちゃんと確認してないのでわかりません。

まぁとにかくSonarScanで検出されたBugを潰さないと怒られるとかいう状況だったら自分は何も考えずにApproveしてMergeします。

[berryzplus]

いや、設計仕様ではなく「漏れ」でしょう。

うーん、そうなんでしょうか？まぁ明言されていないので真相は闇の中じゃないかと思います。

断言しといてアレですが、結論はどっちでもいいです。

assertが入ってるので不正な呼出しに対してデバッグ処理が止まり、対応できたような気になってたのではないかと思います。リリースではそのまま続行してしまうので実際には危険で「何が起きるか分からない」という最も厄介な不具合を引き起こします。

おっしゃる通り不正メモリアクセスが行われていると問題になりかねないです。行われていれば、なのでもし行われていなかった場合には余計なチェックになります。ただし追加したチェックの為にどれだけ性能が低下するかというとおそらく微々たるものだと思います（きちんと計測すれば性能低下の度合いが見えるかもしれませんが手間だし…）。

高速化のために、必要なチェックをまとめて1回にすることはあると思います。

どうしても速度が欲しい処理のためにC言語やアセンブラを使うことに異論はないです。
必要かどうか分からないのに、普通の配列クラスが持ってる機能を削るのは違う気がします。

ところで Visual Studio 2019 version 16.9 から AddressSanitizer がサポートされたようなのでデバッグに役立てられそうです。
link
clang とか gcc のように色々な Sanitizer が VC++ でも使えるようになると良いですね。

これはよく知らんです。

msvcには、スコープ脱出時にヒープ破壊が行われていないかチェックする機能がありますが、それ系かな？

「ヒープを使わないVector」は、共有メモリの一部に「特定の型の配列」が含まれていて、これに安全にアクセスするために共有メモリのレイアウトと互換性のある疑似Vectorクラスが必要になったから作られた、と理解しています。
疑似Vectorクラスの開発要件は、以下のようなものだったと推定されます。
・生配列と親和性のあるインターフェースを備えていること
・ヒープなどの外部メモリを使用しないこと
・メソッドが例外を投げないこと
よって、軽量化を意図した機能省略が行われたとは考えにくく、純粋に処理が漏れていたものと考えられます。

列挙して頂いた推定内容から「軽量化を意図した機能省略が行われたとは考えにくく」に、どのように繋がるのかよくわかりません。論理の飛躍とまで言ったら失礼かもしれませんが、想像力が働かない自分みたいな杓子定規な人間には行間を読むのが難しいです。開発要件というほど明確に内容を定めてから実装したのかどうかは自分には不明です。冒頭でも書きましたが、コメントで解説されているわけではないのでなんともです…。

ああ、確かに。
「こういうクラスが欲しい！」で考えたと思われることを列挙しましたが、
列挙に含まれていなかったからといって「考えなかった」ではないですね。

operator []やresizeのチェック漏れも対処を行うのが「筋」だと思いますが、このPRの目的はSonarScanで検出された潜在不具合の対処なので、関係ない変更は含めていません。

berryzplusさんがどうしてそれが「筋」だと思うのかは省略して書かれていないので想像してみますが、おそらく足並みを揃えるというか push_back で境界チェック処理を追加するなら他のメソッドにも追加するべきって考えですよね？自分は他のメソッドにも境界チェックは追加されていなかったから StaticVector は意図して（assert以外の）境界チェックを行わないように実装したのだと思いました。

どうして対応するのが「筋」なのか？　👉　対応方針に統一感がないのはキモいからっす。

もっとも、push_backとresize/opereator[]ではアクセスの種類が違うので、分けて考えていいのかも知れません。

• push_back ＝ 書き込み先が範囲内かチェックせずに書き込むのは危険だからチェックを入れる。
• resize/opereator[] ＝ 書き込まないから👆の変更と同じ理屈にはならない（かも。

ところで std::vector の operator [] は境界チェックは行わない仕様です。link
std::vector の at は境界チェックを行う仕様で範囲外アクセスの場合には std::out_of_range 例外を送出します。link
これに合わせるとなると StaticVector の operator [] では境界チェックは行わないでも良いとは思います。

まぁそういうこと（≒operator[]で境界チェックしてるの、おかしくね？）です。

サクラエディタの StaticVector の operator [] や resize で境界チェックを行うべきなのかどうかについて考えてみます。
もし境界チェックを行わないなら呼び出し側の方では境界越えが発生しない正常な範囲で呼び出す必要が有ります。

仮に境界チェックを行う場合の話ですが、境界越えの範囲が指定された場合にメモリアクセスは行わないのは前提としてその後にどうするかについてですが、

1. std::out_of_range 例外を送出する。
2. 範囲外が指定されたかどうかを変数(戻り値、引数、グローバル変数、etc...)に記録する
3. 何もしない

が思いつきます。このPRでは3番の何もしない方式ですが、それで問題が起きないのかどうか、呼び出し元では何もしないでよいのかどうかはちゃんと確認してないのでわかりません。

例外は、導入コストが高そうなので除外しました。
引数や戻り値を追加するのは、手間なので除外しました。
何もしない、の対処には本質的な問題※がありますが、気付かなかったことにしてこれにしました。

※処理の失敗を考慮しないループ内で「何もしない」を行うと、ループ脱出条件が満たされなくなりハングアップする危険があります。おそらく #1635 がこのケースの実例っす:smiley:

まぁとにかくSonarScanで検出されたBugを潰さないと怒られるとかいう状況だったら自分は何も考えずにApproveしてMergeします。

誰に？ｗ

[k-takata]

ところで Visual Studio 2019 version 16.9 から AddressSanitizer がサポートされたようなのでデバッグに役立てられそうです。
link
clang とか gcc のように色々な Sanitizer が VC++ でも使えるようになると良いですね。

AddressSanitizer ぐらいしか知りませんでしたが、Clang 13 (開発中)のドキュメントによると、
AddressSanitizer, ThreadSanitizer, MemorySanitizer, UndefinedBehaviorSanitizer, DataFlowSanitizer, LeakSanitizer
があるようですね。
この内 Windows 版 Clang で使えると書いてあるのが、AddressSanitizer と UndefinedBehaviorSanitizer。
ちなみに Windows 用 ASan は、MS が Clang の ASan を Windows に移植し、それを Clang にバックポートしたことで VC++/Clang 両方で使えるようになったようです。

[sanomari]

範囲外のときにクラッシュさせても既存コードに影響はない建前ですよね？
気付いてないならないのと同じ？って見方もできます。
「何もしない」の危険性を考慮するなら、セオリー通り例外を使えばよいような。

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

66.7% Coverage
0.0% Duplication

[AppVeyorBot]

✅ Build sakura 1.0.3800 completed (commit c66035543f by @berryzplus)

[AppVeyorBot]

✅ Build sakura 1.0.3801 completed (commit da6a4bfbc0 by @berryzplus)

[beru]

問題無いと思います。

[berryzplus]

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