Open berryzplus opened 3 years ago
kengoide
commented
3 years ago 特に文字列操作についてよく怒られているわけですが、適切に範囲チェックしているのに分析器がそれを検出できてないのが実態であるように見えます。
問題とされた箇所をざっと眺めて得た印象からの発言でしたが、その後 #1483 と #1489 でバッファオーバーランが2件見つかっています。 一見すると正しいようでコーナーケースを網羅できていない事例が他にもありそうだな、というのが今の印象です。
kengoide
commented
3 years ago 最終的に C++ ソース部分の Bugs と Security Hotspot は駆逐したいです。
https://github.com/sakura-editor/sakura/issues/1506#issuecomment-761284972
量が量なので複数人の参加が望ましいと思いますが、誰がどこに着手中かが分かるようにしなければ同じところで作業する事態が起きそうです SonarCloud に Assigned 欄があるので活用したいですね。
berryzplus
commented
3 years ago 量が量なので複数人の参加が望ましいと思いますが、誰がどこに着手中かが分かるようにしなければ同じところで作業する事態が起きそうです SonarCloud に Assigned 欄があるので活用したいですね。
いったん Assigned でいいと思います。
当面はカバレッジはスルーしていくしかないのかなぁ・・・
別件で、現在のカバレッジを少しでも高める内容のPRを検討しています。
kengoide
commented
3 years ago インライン関数やテンプレート関数のカバレッジが取れていないかもしれません。int2dec や charcode.h の判定関数 などです。
berryzplus
commented
3 years ago インライン関数やテンプレート関数のカバレッジが取れていないかもしれません。int2dec や charcode.h の判定関数 などです
現状のOpenCppCoverageの限界かも知れません。
対策として考えられるのは
妥当なのは「見なかったことにする」かな？ 解析対象を x64 - Debug にするのも可能な対策。
インラインを使わないようにするのは少し難しいですが、おそらく「やったほうがよい」だと思います。
berryzplus
commented
3 years ago 地味な対応を続けた結果、Bugs(Blocker)レベルの警告があと10件になりました。Blockerレベルの警告は分かりづらくてレビューしづらいようなので、一律対応できそうなBugs(Critical)レベルの対応を進めて行こうと思っています。
ちなみにラベルの日本語約は以下である認識です。
berryzplus
commented
3 years ago 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自体は閉じてしまおうと思っています。
berryzplus
commented
3 years ago berryzplus
commented
2 years ago PRが出ていない残件（Critical以上）。
[Blocker] sakura_core/charset/CUtf7.cpp
Out of bound memory access (access exceeds upper limit of memory block)
対応無理っぽいです。
[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.
対応無理っぽいです。
深刻度の高そうな警告の大半を潰せたので、あとはしょーもない警告が残ってるだけだと思っていましたが、何気にそうでもなさそうです。
このissueは、もう少しの間 keep-alive にしといたほうが良さそうです。
berryzplus
commented
1 year ago SonarCloudのルールが変更されたことにより、 せっかく減らした警告がまた増えているようです。
variadic paramater形式の関数に CLogicInt とか CLayoutInt とかを渡している部分が Major の Bugsとして検出されるようになった模様。一応、キャストしてあげれば回避はできそう。
Bug だと言われているもののうち最高レベルの Blocker は43件で、内訳は以下のような形です。
特に文字列操作についてよく怒られているわけですが、適切に範囲チェックしているのに分析器がそれを検出できてないのが実態であるように見えます。 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