sakura
sakura copied to clipboard
sakura-editor
SonarCloudにBugだと言われているものを対処したいなぁという話。
Bug だと言われているもののうち最高レベルの Blocker は43件で、内訳は以下のような形です。
- 27件 = 文字列操作（範囲外参照の可能性）
- 6件 = 「std::make_unique の戻り値がリークする」という内容の警告（？？）
- 4件 = std::forwardの使い方 (#1477)
- 3件 = 文字以外の配列操作
- 2件 = オブジェクト外への参照を戻り値にしている
- 1件 = 継承元クラスへのスライシング
特に文字列操作についてよく怒られているわけですが、適切に範囲チェックしているのに分析器がそれを検出できてないのが実態であるように見えます。 C文字列を使ってはいけない。すべて std::string にするべきだ。ということを言いたいようですが。 1万9千件もある Code Smell についての警告も大部分がそういう内容ですし、コードベース全体で審査に合格するのはきっと途方もなく大変です…。
PR などで追加・修正した部分だけ Quality Gate 適用できたらいいなあ、と思います。
Originally posted by @k-kagari in https://github.com/sakura-editor/sakura/issues/1476#issuecomment-738895981
特に文字列操作についてよく怒られているわけですが、適切に範囲チェックしているのに分析器がそれを検出できてないのが実態であるように見えます。
問題とされた箇所をざっと眺めて得た印象からの発言でしたが、その後 #1483 と #1489 でバッファオーバーランが2件見つかっています。 一見すると正しいようでコーナーケースを網羅できていない事例が他にもありそうだな、というのが今の印象です。
最終的に C++ ソース部分の Bugs と Security Hotspot は駆逐したいです。
https://github.com/sakura-editor/sakura/issues/1506#issuecomment-761284972
量が量なので複数人の参加が望ましいと思いますが、誰がどこに着手中かが分かるようにしなければ同じところで作業する事態が起きそうです SonarCloud に Assigned 欄があるので活用したいですね。
量が量なので複数人の参加が望ましいと思いますが、誰がどこに着手中かが分かるようにしなければ同じところで作業する事態が起きそうです SonarCloud に Assigned 欄があるので活用したいですね。
いったん Assigned でいいと思います。
- 自分で Assigned にする
- Bugs指摘に対策する
- 可能な範囲でテストを書く
- PRする。（テスト不可能な範囲が残った状態になる）
- マージする
- マージ以降30日間はQuality GateがFailedになる
当面はカバレッジはスルーしていくしかないのかなぁ・・・
別件で、現在のカバレッジを少しでも高める内容のPRを検討しています。
インライン関数やテンプレート関数のカバレッジが取れていないかもしれません。int2dec や charcode.h の判定関数 などです
現状のOpenCppCoverageの限界かも知れません。
対策として考えられるのは
- インライン関数を使わないようにする
- 解析対象を x64 - Debug に変える
- 見なかったことにする
妥当なのは「見なかったことにする」かな？ 解析対象を x64 - Debug にするのも可能な対策。
インラインを使わないようにするのは少し難しいですが、おそらく「やったほうがよい」だと思います。
地味な対応を続けた結果、Bugs(Blocker)レベルの警告があと10件になりました。Blockerレベルの警告は分かりづらくてレビューしづらいようなので、一律対応できそうなBugs(Critical)レベルの対応を進めて行こうと思っています。
ちなみにラベルの日本語約は以下である認識です。
- Blockerレベル ＝ リリースしてはマズいレベルの致命的な誤り。
- Criticalレベル ＝ 致命的な誤り。
- Majorレベル ＝ よくある誤り（いわゆる「あるある」。普通は対応する。）
- Minorレベル ＝ 細かい誤り（対応が必要か、怪しい。）
BlockerレベルのBugsが残り2件になりました。
-
Out of bound memory access (access exceeds upper limit of memory block) https://github.com/sakura-editor/sakura/blob/5135813a52b3339eb94462c66dab6407ac666a9c/sakura_core/charset/CUtf7.cpp#L112
*(pr_next - 1)がメモリ範囲外を参照する可能性があります。これは、たぶん誤検知なので実害はないと思います。 UTF-7のデコード処理（バイナリシーケンスから文字列への変換）は概念的に有限オートマトンなので、インスタンスメンバに状態を保持するように書き直したほうが分かりやすくなるはずです。
-
Returned pointer value points outside the original object (potential buffer overflow) https://github.com/sakura-editor/sakura/blob/5135813a52b3339eb94462c66dab6407ac666a9c/sakura_core/types/CTypeSupport.h#L95-L98
thisがnullptrの場合など、*m_pColorInfoArrがオブジェクト外を参照する可能性があります。これもたぶん誤検知なので実害はないと考えられます。 原因が分からないので対策が打てないです。 想定している原因は、
CFigureSpace::DrawImp_StyleSelectが複雑すぎることとメソッド内の変数定義にconstが付いてないことです（片っ端からconst付けたら消えるかもです。
ちょっと対処が難しそうなので、しばらくしたら個別のissueを作成してこのissue自体は閉じてしまおうと思っています。
PRが出ていない残件（Critical以上）。
-
[Blocker]sakura_core/charset/CUtf7.cpp Out of bound memory access (access exceeds upper limit of memory block) 対応無理っぽいです。- 文字列操作に配列を使うことがそもそもNGとなるようです。
- 元のコードが妥当であるかどうかに自信が持てないです。
-
[Critical]sakura_core/uiparts/CGraphics.h Explicitly define or delete the missing copy assignment operator so that it will not be implicitly provided. Additionally, you may consider defining the move assignment operator and the move constructor. 対応無理っぽいです。- RAII的に扱う要素を2つ以上持つクラスなので、そもそも分割すべきと思われます。
- 対応以前に定義メソッドが多過ぎるので、役割ごとに分割すべきと思われます。
- 元のコードが妥当ではないように思っています（致命的）
深刻度の高そうな警告の大半を潰せたので、あとはしょーもない警告が残ってるだけだと思っていましたが、何気にそうでもなさそうです。
https://sonarcloud.io/project/issues?id=sakura-editor_sakura&resolved=false&severities=MAJOR&types=BUG
- HTMLでframeset使うな（しょーもなっ・・・
- if文のtrue-partとfalse-partが同じです（へぇ～。。。
- 関数呼び出しの引数が初期化されてません（えっ！
- ポインタ値がnullptrのobjectを参照しています（マジか！
- 否定の等価演算子の右辺値が破棄されます（いみふ・・・
このissueは、もう少しの間 keep-alive にしといたほうが良さそうです。
SonarCloudのルールが変更されたことにより、 せっかく減らした警告がまた増えているようです。
variadic paramater形式の関数に CLogicInt とか CLayoutInt とかを渡している部分が Major の Bugsとして検出されるようになった模様。一応、キャストしてあげれば回避はできそう。