SonarQubeの静的解析で検出されたバグを修正する #1439
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,107 @@ | |
| #include "StdAfx.h" | ||
| #include "StdControl.h" | ||
|
|
||
| #include <windowsx.h> | ||
|
|
||
| namespace ApiWrap{ | ||
|
|
||
| /*! | ||
| @brief Window テキストを取得する | ||
| @param[in] hWnd ウィンドウハンドル | ||
| @param[out] strText ウィンドウテキストを受け取る変数 | ||
| @return 成功した場合 true | ||
| @return 失敗した場合 false | ||
| */ | ||
| bool Wnd_GetText( HWND hWnd, std::wstring& strText ) | ||
| { | ||
| // バッファをクリアしておく | ||
| strText.clear(); | ||
|
|
||
| // GetWindowTextLength() はウィンドウテキスト取得に必要なバッファサイズを返す。 | ||
| // 条件によっては必要なサイズより大きな値を返すことがある模様 | ||
| // https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-getwindowtextlengthw | ||
| const int cchRequired = ::GetWindowTextLength( hWnd ); | ||
| if( cchRequired < 0 ){ | ||
| // ドキュメントには失敗した場合、あるいはテキストが空の場合には 0 を返すとある。 | ||
| // 0 の場合はエラーかどうか判断できないのでテキストの取得処理を続行する。 | ||
| // 仕様上は負の場合はありえないが、念の為エラーチェックしておく。 | ||
| return false; | ||
| }else if( cchRequired == 0 ){ | ||
| // GetWindowTextLength はエラーの場合、またはテキストが空の場合は 0 を返す | ||
| if( GetLastError() != 0 ){ | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // ウィンドウテキストを取得するのに必要なバッファを確保する | ||
| strText.reserve( cchRequired ); | ||
|
There was a problem hiding this comment. reserveはバッファの確保はしてくれるものの、その拡張領域へ勝手にアクセスしてはいけないことになっています。
ということで、resize()のほうが好ましいです。 |
||
|
|
||
| // GetWindowText() はコピーした文字数を返す。 | ||
| // https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getwindowtextw | ||
| const int actualCopied = ::GetWindowText( hWnd, strText.data(), cchRequired ); | ||
| if( actualCopied < 0 ){ | ||
| // 仕様上は負の場合はありえないが、念の為エラーチェックしておく。 | ||
| return false; | ||
| } | ||
| else if( actualCopied == 0 ){ | ||
| // GetWindowText はエラーの場合、またはテキストが空の場合は 0 を返す | ||
| if( GetLastError() != 0 ){ | ||
| return false; | ||
| } | ||
| } | ||
| else if( (int)strText.capacity() <= actualCopied ){ | ||
| // GetWindowText() の仕様上はありえないはず | ||
| return false; | ||
| } | ||
|
|
||
| // データサイズを反映する | ||
| strText.assign( strText.data(), actualCopied ); | ||
|
There was a problem hiding this comment. basic_stringのoperator=ではない自己代入(領域が重なる配列)の動作保証は、怪しいと思います。 str.resize()→str.data()による書き換え→万が一APIの結果文字列長が短い場合は再度str.resize()で縮める。 他の追加された処理ももちろん同様です。 There was a problem hiding this comment. ここは実際にはコピーは行われずlength)だけが更新されます。 ポインタからポインタに値をコピーする処理で、ポインタが同じかどうかをチェックしないでコピーするコードを書いて静的解析ツールに怒られたことがあります。絶対に大丈夫？って聞かれるとゴニョニョですが、大丈夫なんじゃないかと思います。（VC++の実装は大丈夫でした。 There was a problem hiding this comment. assign メソッドではなく resize メソッドの呼び出しに置き換えるのに心理的に抵抗が無ければそうした方が良いのではないかと自分も思います。というかそれに変える事によるデメリットってあるんでしょうか？メリットとしては安心感でしょうか？
自分は実装は確認していませんが assign で自己代入で長さを縮める操作をする場合はバッファの指定長の位置に null terminator の書き込みとかも行うんじゃないかと想像してます。 さて、動いているから問題ないだろと言われたら明確な反論は出来ないです…。 |
||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /*! | ||
| @brief リストアイテムのテキストを取得する | ||
| @param[in] hList リストコントロールのウインドウハンドル | ||
| @param[in] nIndex リストアイテムのインデックス | ||
| @param[out] strText アイテムテキストを受け取る変数 | ||
| @return 成功した場合 true | ||
| @return 失敗した場合 false | ||
| */ | ||
| bool List_GetText( HWND hList, int nIndex, std::wstring& strText ) | ||
| { | ||
| // バッファをクリアしておく | ||
| strText.clear(); | ||
|
|
||
| const int cchRequired = ListBox_GetTextLen( hList, nIndex ); | ||
| if( cchRequired < 0 ){ | ||
| // LB_ERR(-1)とその他のエラーは区別しない | ||
| return false; | ||
| }else if( cchRequired == 0 ){ | ||
| return true; | ||
| } | ||
|
|
||
| // ウィンドウタイトルを設定するのに必要なバッファを確保する | ||
| strText.reserve( cchRequired ); | ||
|
|
||
| // ListBox_GetText() はコピーした文字数を返す。 | ||
| const int actualCopied = ListBox_GetText( hList, nIndex, strText.data() ); | ||
| if( actualCopied < 0 ){ | ||
| // LB_ERR(-1)とその他のエラーは区別しない | ||
| return false; | ||
| } | ||
| else if( (int)strText.capacity() <= actualCopied ){ | ||
| // ListBox_GetText() の仕様上はありえないはず | ||
| return false; | ||
| } | ||
|
|
||
| // データサイズを反映する | ||
| strText.assign( strText.data(), actualCopied ); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| LRESULT List_GetText(HWND hwndList, int nIndex, WCHAR* pszText, size_t cchText) | ||
| { | ||
| LRESULT nCount = SendMessage( hwndList, LB_GETTEXTLEN, (WPARAM)nIndex, (LPARAM)0); | ||
|
|
@@ -14,6 +113,28 @@ namespace ApiWrap{ | |
| return SendMessage( hwndList, LB_GETTEXT, (WPARAM)nIndex, LPARAM(pszText) ); | ||
| } | ||
|
|
||
| /*! | ||
| @brief ダイアログアイテムのテキストを取得する | ||
| @param[in] hDlg ウィンドウハンドル | ||
| @param[in] nIDDlgItem ダイアログアイテムのID | ||
| @param[out] strText アイテムテキストを受け取る変数 | ||
| @return 成功した場合 true | ||
| @return 失敗した場合 false | ||
| */ | ||
| bool DlgItem_GetText( HWND hDlg, int nIDDlgItem, std::wstring& strText ) | ||
|
There was a problem hiding this comment. ヘッダ側の他の関数と同様にインライン関数にしてしまうのが良い気がします。 |
||
| { | ||
| // バッファをクリアしておく | ||
| strText.clear(); | ||
|
|
||
| // アイテムのハンドルを取得する | ||
| HWND hWnd = ::GetDlgItem( hDlg, nIDDlgItem ); | ||
| if( hWnd == NULL ){ | ||
| return false; | ||
| } | ||
|
|
||
| return Wnd_GetText( hWnd, strText ); | ||
| } | ||
|
|
||
| UINT DlgItem_GetText(HWND hwndDlg, int nIDDlgItem, WCHAR* pszText, int nMaxCount) | ||
| { | ||
| return GetDlgItemText(hwndDlg, nIDDlgItem, pszText, nMaxCount); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここは一見同じ処理かもしれませんが、違いますね。
元のコードはパス中に見つかった"をすべて削除するコードです。途中に"がある場合も含まれています。
途中の"も勘案してくれるのは、my_strtokがそういう仕様です。
file"\hoge piyo"subfolder みたいなちょっと気持ち悪いパスにも対応しています。
新しいコードは前後で囲われた""のみに対応しています。
これが問題になるのはかなりのレアケースではあるんでしょうけど、違うといえば違います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビューありがとうございます。
パスに含まれる
"を削除する処理に直したいと思います。