SonarQubeの静的解析で検出されたバグを修正する #1439

sanomari · 2020-10-25T15:54:08Z

PR の目的

SonarQubeの静的解析で検出されたバグを修正します。

カテゴリ

リファクタリング

PR の背景

https://github.com/sakura-editor/sakura の README.md に貼ってある Sonar Qube の静的解析で、致命的なエラーが出ているのが気になっています。

　👆2020/10/26現在、failed って出てます。

現在検出されているNG箇所は 330個あります。

今回は、ランク分けされたNG箇所のうち、最もダメな警告に対処してみたいと思います。

対処するNG指摘

Potential leak of memory pointed to by field '_Myval2'

対処方法

std::wstring を使ってメモリ確保失敗時に例外が発生するように修正します。

PR の影響範囲

ウインドウテキストの取得に生配列のスマートポインタを利用していた箇所をstd::wstringで置換します。

PR のメリット

静的解析で検出される警告が減ります。

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

何か動作かえてしまっていたらごめんなさい。

仕様・動作説明

設計時にバッファサイズを確定できない wchar_t 配列を動的に確保するコードをリファクタリングします。

PR の影響範囲

テスト内容

動作を変えていないので軽く動かしてみただけです。

参考資料

std::make_unique<WCHAR[]>を使わないで済むようにラッパー関数を追加する

std::make_unique<WCHAR[]>を使わないように修正

AppVeyorBot · 2020-10-25T16:16:16Z

✅ Build sakura 1.0.3187 completed (commit cae422f7b5 by @sanomari)

berryzplus

ざっと見た感じ問題なさそうです。

sakura_core/CHokanMgr.cpp

sakura_core/apiwrap/StdControl.h

sakura_core/debug/Debug1.cpp

oooonduke · 2020-10-26T02:28:05Z

サクラエディタ v2.4.2.3187 64bit dev Alpha Version
(GitHash cae422f)
(GitURL https://github.com/sakura-editor/sakura.git)
バージョン情報のSourceURLが…giとなっているのが気になります。

berryzplus · 2020-10-26T03:15:47Z

バージョン情報のSourceURLが…giとなっているのが気になります。

ほんとですね。

sanomari · 2020-10-26T03:49:00Z

すみません、やらかしていました。

AppVeyorBot · 2020-10-26T04:05:40Z

❌ Build sakura 1.0.3188 failed (commit e9d0e88a1d by @sanomari)

AppVeyorBot · 2020-10-26T04:28:02Z

✅ Build sakura 1.0.3189 completed (commit c010103003 by @sanomari)

AppVeyorBot · 2020-10-26T04:50:00Z

✅ Build sakura 1.0.3190 completed (commit f02504c121 by @sanomari)

usagisita

エラーメッセージでぐぐると以下のものがヒットします。
https://bugs.llvm.org/show_bug.cgi?id=38176
これって誤検出なのではないか、と思うのですが、実際にバグで、メモリーリークするケースがあるのでしょうか？
どういう状況で問題があるのか分かっているなら、教えてほしいなと思います。
もっとも、誤検出かどうかにかかわらず、これらのべた書きの処理をまとめるという部分に関しては、大賛成です。

usagisita · 2020-10-26T04:22:54Z

sakura_core/CGrepAgent.cpp

+	while( NULL != (token = my_strtok<WCHAR>( strPath.data(), nPathLen, &nPathPos, L";")) ){
+		std::wstring strTemp( token );
+		if( strTemp.length() >= 2 && strTemp.front() == L'"' && strTemp.back() == strTemp.front() ){
+			strTemp = strTemp.substr( 1, strTemp.length() - 2 );


ここは一見同じ処理かもしれませんが、違いますね。
元のコードはパス中に見つかった"をすべて削除するコードです。途中に"がある場合も含まれています。
途中の"も勘案してくれるのは、my_strtokがそういう仕様です。
file"\hoge piyo"subfolder みたいなちょっと気持ち悪いパスにも対応しています。
新しいコードは前後で囲われた""のみに対応しています。
これが問題になるのはかなりのレアケースではあるんでしょうけど、違うといえば違います。

レビューありがとうございます。
パスに含まれる " を削除する処理に直したいと思います。

usagisita · 2020-10-26T04:31:35Z

sakura_core/apiwrap/StdControl.cpp

+		}
+
+		// ウィンドウテキストを取得するのに必要なバッファを確保する
+		strText.reserve( cchRequired );


reserveはバッファの確保はしてくれるものの、その拡張領域へ勝手にアクセスしてはいけないことになっています。
https://qiita.com/amayaw9/items/6e55b91c28cdc8d32cf2
この記事や、信頼性ありそうなサイトだと
https://cpprefjp.github.io/reference/string/basic_string/data.html

この関数を使用するユーザーは、p + size() (NULL終端) に格納されている値を変更してはならない

ということで、resize()のほうが好ましいです。
例えばreserveしていても、clearしているので中身の空文字列のdata()やc_str()が「スモールバッファ」側のポインタを返す場合が考えられるかもしれません。
そうすると、reserveしていても、size()より後ろを勝手に書き換えると、どうなるかは未保証です。

resize()に直しておきます。

usagisita · 2020-10-26T05:16:36Z

sakura_core/apiwrap/StdControl.cpp

+		}
+
+		// データサイズを反映する
+		strText.assign( strText.data(), actualCopied );


basic_stringのoperator=ではない自己代入(領域が重なる配列)の動作保証は、怪しいと思います。
検索はしてみたものの、問題ないという資料を発見できず、完全には安全ではないっぽいということまでしか、わかりませんでした。
https://stackoverflow.com/questions/28142011/can-you-assign-a-substring-of-a-stdstring-to-itself
それから上のreserveを修正したら無効になる話ですが、reserveだけの状態でsize()より後ろにアクセスしてはいけないので、actualCopiedがsize以上の場合もおそらく未定義の動作です。

str.resize()→str.data()による書き換え→万が一APIの結果文字列長が短い場合は再度str.resize()で縮める。
とするのがいいんじゃないかと思います。
重箱の隅のような指摘ではありますが、C言語にはmemmoveがわざわざ存在しているくらいなので、気になりました。

他の追加された処理ももちろん同様です。

ここは実際にはコピーは行われずlength)だけが更新されます。

ポインタからポインタに値をコピーする処理で、ポインタが同じかどうかをチェックしないでコピーするコードを書いて静的解析ツールに怒られたことがあります。絶対に大丈夫？って聞かれるとゴニョニョですが、大丈夫なんじゃないかと思います。（VC++の実装は大丈夫でした。

assign メソッドではなく resize メソッドの呼び出しに置き換えるのに心理的に抵抗が無ければそうした方が良いのではないかと自分も思います。というかそれに変える事によるデメリットってあるんでしょうか？メリットとしては安心感でしょうか？

ここは実際にはコピーは行われずlength)だけが更新されます。

自分は実装は確認していませんが assign で自己代入で長さを縮める操作をする場合はバッファの指定長の位置に null terminator の書き込みとかも行うんじゃないかと想像してます。

さて、動いているから問題ないだろと言われたら明確な反論は出来ないです…。

berryzplus · 2020-10-26T10:22:10Z

これって誤検出なのではないか、と思うのですが、実際にバグで、メモリーリークするケースがあるのでしょうか？

std::make_uniqueの仕様とMSVCのnew()の仕様を見た感じでは誤検出っぽいです。
https://cpprefjp.github.io/reference/memory/make_unique.html

std::make_uniqueは仕様的に operator new() を呼ぶ。
MSVC の operator new() は失敗時に std::bad_alloc を投げる。
ここだけ見ると誤検知に見えます。
operator new() は独自にオーバーロードできる。
いいか悪いかは別にして、例外を投げないオーバーロードも可能。
これを考慮すると、一概に誤検出だとは言えない気もします。

個人的には「failedなのはマズいっしょ！」という趣旨と理解してて、
バッチが真っ赤なのに気付いてなかったんで「やっべ！」と思っています。

sakura_core/util/string_ex.h

前後の '"" を取り除くように実装してたのをパスに含まれる '"' を削除するように修正。

AppVeyorBot · 2020-10-26T20:12:45Z

❌ Build sakura 1.0.3193 failed (commit 79424e436b by @sanomari)

beru

883eee4 で確認しましたがビルドに失敗します。

1>CMainToolBar.cpp
1>C:\projects\sakura\sakura_core\window\CMainToolBar.cpp(189,2): error C3861: 'CreateToolBarButtons': identifier not found

最初のエラーメッセージが上記のものですが、CMainToolBar.h ファイルに CreateToolBarButtons メソッドのプロトタイプが存在しないので git のステージングし忘れのような気がします。

手間ですがローカル側でいったん別の場所で pull してビルド出来るかの確認はした方が良いです。

あとこれは強制的ではないですが開発ブランチの名前は master ではない方が良いと思います。

beru · 2020-10-26T23:08:18Z

あくまで個人の意見に過ぎないですが、１つのPRで観点が色々な変更を一挙にやろうとしない方が良いと思います。変更のボリュームが大きい場合はなおさらです。レビューするのが大変というレビューアー側の能力の問題もありますが…。

SonarQubeの静的解析で検出されたバグ が多数ある場合はそれをまず新規に Issue を作成してでも良いので列挙して、複数のPRに分けた方がシンプルに片づけられそうならそうした方が良いかなと思います。

ただまぁ考え方は人によって違うので正解は無い事だと思います。急がば回れにするか、回りくどいやり方はかったるくてやってられるかで一気に済ませるか、人それぞれですね。

berryzplus · 2020-10-27T02:09:37Z

CodeFactorの警告対応は修正と関係ない箇所で、
かつ、対応入れても警告が消えなかったようなので、
今回は外したほうがいいように思います。

AppVeyorBot · 2020-10-27T03:18:30Z

✅ Build sakura 1.0.3194 completed (commit 276fc006c9 by @sanomari)

sanomari · 2020-10-27T03:41:20Z

最初のエラーメッセージが上記のものですが、CMainToolBar.h ファイルに CreateToolBarButtons メソッドのプロトタイプが存在しないので git のステージングし忘れのような気がします。

レビューありがとうございます。
原因はステージング漏れでした。

sanomari · 2020-10-27T03:41:48Z

CodeFactorの警告対応は修正と関係ない箇所で、
かつ、対応入れても警告が消えなかったようなので、
今回は外したほうがいいように思います。

今回は外してしまうことにしました。

std::wstringのsize()が返すインデックス以降をアクセスした場合の動作は保証されない。 reserveだけだとsizeが変更されないのでresizeに変更しておく。

AppVeyorBot · 2020-10-27T07:19:12Z

✅ Build sakura 1.0.3195 completed (commit 132c52897b by @sanomari)

berryzplus · 2020-10-28T11:51:23Z

結果を確認したいのでマージしてしまいます・・・・って出来ませんでした。

beru · 2020-10-28T14:32:20Z

sakura_core/apiwrap/StdControl.cpp

+		@return		成功した場合 true
+		@return		失敗した場合 false
+	*/
+	bool DlgItem_GetText( HWND hDlg, int nIDDlgItem, std::wstring& strText )


ヘッダ側の他の関数と同様にインライン関数にしてしまうのが良い気がします。
/LTCG (リンク時のコード生成) があるから不要と言われるかもしれませんが、ヘッダファイルの行数を増やす事で目grepに頼るプログラマの眼神経に負荷を与える事を思い浮かべて大きな自己満足を得る事が出来ます。

beru · 2020-10-28T14:35:51Z

sakura_core/dlg/CDlgFind.cpp

-	Combo_GetText( hwndCombo, &vText[0], nBufferSize );
-	if (m_strText.compare( &vText[0] ) != 0) {
+	std::wstring strText;
+	if( !ApiWrap::DlgItem_GetText( GetHwnd(), IDC_COMBO_TEXT, strText ) || strText != m_strText ) {


IDC_COMBO_TEXT の HWND を取る操作を Get と Set で2回繰り返していることが気になります。

beru

ビルドでエラーが起きないで起動して簡単な操作が出来る事は確認しました。
変更箇所が多いので変更箇所それぞれに対応する動作確認は取っていません。

sanomari · 2020-10-29T00:01:50Z

ビルドでエラーが起きないで起動して簡単な操作が出来る事は確認しました。
変更箇所が多いので変更箇所それぞれに対応する動作確認は取っていません。

お手数おかけしました。change requiredを無視するのはよくないと思ってお願いしていました。

sanomari · 2020-11-15T08:24:55Z

いま数えてみたら残数302でした。

静的解析の結果で、この修正と同じ内容の警告がいくつか残っていることに気づきました。

berryzplus · 2020-11-16T13:58:37Z

いま335件ですね。

SonarQubeのビルドは4本走っているので、バグの数は日によって異なります。
（Win32/x64、Debug/Releaseの組み合わせで4本です。）

今日の分は x64 - Release のビルドが最後に終わってるから、致命的にダメな警告がちょっと多いってことみたいです。

sanomari added 8 commits October 25, 2020 23:01

ApiWrap::Wnd_GetTextのオーバーロード関数を追加

dd9b851

std::make_unique<WCHAR[]>を使わないで済むようにラッパー関数を追加する

ApiWrap::DlgItem_GetTextのオーバーロード関数を追加

2e8452c

std::make_unique<WCHAR[]>を使わないで済むようにラッパー関数を追加する

ApiWrap::List_GetTextのオーバーロード関数を追加

ed530f8

std::make_unique<WCHAR[]>を使わないで済むようにラッパー関数を追加する

Wnd_GetTextを適用する

c5b4a0f

std::make_unique<WCHAR[]>を使わないように修正

List_GetTextを適用する

2318efb

std::make_unique<WCHAR[]>を使わないように修正

DebugOutWのリファクタリング

74e75f2

std::make_unique<WCHAR[]>を使わないように修正

CGrepAgent::CreateFoldersリファクタリング

085ac88

std::make_unique<WCHAR[]>を使わないように修正

CPropTypesRegexリファクタリング

020f8b7

std::make_unique<WCHAR[]>を使わないように修正

berryzplus previously approved these changes Oct 26, 2020

View reviewed changes

sakura_core/CHokanMgr.cpp Outdated Show resolved Hide resolved

sakura_core/apiwrap/StdControl.h Show resolved Hide resolved

sakura_core/debug/Debug1.cpp Outdated Show resolved Hide resolved

sakura_core/debug/Debug1.cpp Outdated Show resolved Hide resolved

デッドコード削除

336cc8e

sanomari marked this pull request as draft October 26, 2020 03:36

sanomari dismissed berryzplus’s stale review via 7f89316 October 26, 2020 03:47

sanomari marked this pull request as ready for review October 26, 2020 03:49

sanomari force-pushed the master branch from 7f89316 to a4c1e2e Compare October 26, 2020 03:58

strprintf/vstrprintfを追加

6d9cb04

取得可能サイズの指定を修正

6401489

sanomari force-pushed the master branch from a4c1e2e to 6401489 Compare October 26, 2020 04:06

usagisita reviewed Oct 26, 2020

View reviewed changes

berryzplus reviewed Oct 26, 2020

View reviewed changes

sakura_core/util/string_ex.h Outdated Show resolved Hide resolved

レビュー指摘(二重引用符を取り除く処理が変わってる)の対応

d722d8c

前後の '"" を取り除くように実装してたのをパスに含まれる '"' を削除するように修正。

beru requested changes Oct 26, 2020

View reviewed changes

sanomari force-pushed the master branch from 883eee4 to 3c0a2f4 Compare October 27, 2020 02:56

sanomari added 2 commits October 27, 2020 14:00

レビュー指摘(reserveよりresizeのが良さそう)の対応

6036009

std::wstringのsize()が返すインデックス以降をアクセスした場合の動作は保証されない。 reserveだけだとsizeが変更されないのでresizeに変更しておく。

レビュー指摘(strprintf/vstrprintfはテンプレートじゃないほうがいい)の対応

943e1d9

sanomari force-pushed the master branch from 3c0a2f4 to 943e1d9 Compare October 27, 2020 05:01

berryzplus approved these changes Oct 27, 2020

View reviewed changes

sanomari requested a review from beru October 28, 2020 12:08

beru reviewed Oct 28, 2020

View reviewed changes

beru approved these changes Oct 28, 2020

View reviewed changes

sanomari merged commit 388a736 into sakura-editor:master Oct 29, 2020

kengoide mentioned this pull request Feb 3, 2021

Wnd_GetText が一文字取りこぼす問題に対処 #1528

Merged

beru added the refactoring リファクタリング【ChangeLog除外】 label Mar 21, 2021

ghost mentioned this pull request Jan 3, 2022

補完候補一覧ダイアログの不具合修正 #1750

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SonarQubeの静的解析で検出されたバグを修正する #1439

SonarQubeの静的解析で検出されたバグを修正する #1439

sanomari commented Oct 25, 2020

AppVeyorBot commented Oct 25, 2020

berryzplus left a comment

oooonduke commented Oct 26, 2020

berryzplus commented Oct 26, 2020

sanomari commented Oct 26, 2020

AppVeyorBot commented Oct 26, 2020

AppVeyorBot commented Oct 26, 2020

AppVeyorBot commented Oct 26, 2020

usagisita left a comment

usagisita Oct 26, 2020

sanomari Oct 26, 2020

usagisita Oct 26, 2020

sanomari Oct 26, 2020

usagisita Oct 26, 2020

sanomari Oct 26, 2020

beru Oct 28, 2020

berryzplus commented Oct 26, 2020

AppVeyorBot commented Oct 26, 2020

beru left a comment

beru commented Oct 26, 2020

berryzplus commented Oct 27, 2020

AppVeyorBot commented Oct 27, 2020

sanomari commented Oct 27, 2020

sanomari commented Oct 27, 2020

AppVeyorBot commented Oct 27, 2020

berryzplus commented Oct 28, 2020 •

edited

Loading

beru Oct 28, 2020

beru Oct 28, 2020

beru left a comment

sanomari commented Oct 29, 2020

sanomari commented Nov 15, 2020

berryzplus commented Nov 16, 2020

SonarQubeの静的解析で検出されたバグを修正する #1439

SonarQubeの静的解析で検出されたバグを修正する #1439

Conversation

sanomari commented Oct 25, 2020

PR の目的

カテゴリ

PR の背景

対処するNG指摘

対処方法

PR の影響範囲

PR のメリット

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

仕様・動作説明

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

AppVeyorBot commented Oct 25, 2020

berryzplus left a comment

Choose a reason for hiding this comment

oooonduke commented Oct 26, 2020

berryzplus commented Oct 26, 2020

sanomari commented Oct 26, 2020

AppVeyorBot commented Oct 26, 2020

AppVeyorBot commented Oct 26, 2020

AppVeyorBot commented Oct 26, 2020

usagisita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus commented Oct 26, 2020

AppVeyorBot commented Oct 26, 2020

beru left a comment

Choose a reason for hiding this comment

beru commented Oct 26, 2020

berryzplus commented Oct 27, 2020

AppVeyorBot commented Oct 27, 2020

sanomari commented Oct 27, 2020

sanomari commented Oct 27, 2020

AppVeyorBot commented Oct 27, 2020

berryzplus commented Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru left a comment

Choose a reason for hiding this comment

sanomari commented Oct 29, 2020

sanomari commented Nov 15, 2020

berryzplus commented Nov 16, 2020

berryzplus commented Oct 28, 2020 •

edited

Loading