Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Oct 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 9 additions & 31 deletions sakura_core/CGrepAgent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -114,43 +114,21 @@ void CGrepAgent::OnAfterSave(const SSaveInfo& sSaveInfo)
*/
void CGrepAgent::CreateFolders( const WCHAR* pszPath, std::vector<std::wstring>& vPaths )
{
const int nPathLen = wcslen( pszPath );
auto szPath = std::make_unique<WCHAR[]>(nPathLen + 1);
auto szTmp = std::make_unique<WCHAR[]>(nPathLen + 1);
wcscpy( &szPath[0], pszPath );
std::wstring strPath( pszPath );
const int nPathLen = static_cast<int>( strPath.length() );

WCHAR* token;
int nPathPos = 0;
while( NULL != (token = my_strtok<WCHAR>( &szPath[0], nPathLen, &nPathPos, L";")) ){
wcscpy( &szTmp[0], token );
WCHAR* p;
WCHAR* q;
p = q = &szTmp[0];
while( *p ){
if( *p != L'"' ){
if( p != q ){
*q = *p;
}
q++;
}
p++;
}
*q = L'\0';
#if 0
// 2011.12.25 仕様変更。最後の\\は取り除く
int nFolderLen = q - &szTmp[0];
if( 0 < nFolderLen ){
int nCharChars = &szTmp[nFolderLen] - CNativeW::GetCharPrev( &szTmp[0], nFolderLen, &szTmp[nFolderLen] );
if( 1 == nCharChars && (L'\\' == szTmp[nFolderLen - 1] || L'/' == szTmp[nFolderLen - 1]) ){
szTmp[nFolderLen - 1] = L'\0';
}
}
#endif
while( NULL != (token = my_strtok<WCHAR>( strPath.data(), nPathLen, &nPathPos, L";")) ){
std::wstring strTemp( token );
// パスに含まれる '"' を削除する
strTemp.erase( std::remove( strTemp.begin(), strTemp.end(), L'"' ), strTemp.end() );
/* ロングファイル名を取得する */
WCHAR szTmp2[_MAX_PATH];
if( ::GetLongFileName( &szTmp[0], szTmp2 ) ){
if( ::GetLongFileName( strTemp.c_str(), szTmp2 ) ){
vPaths.push_back( szTmp2 );
}else{
vPaths.push_back( &szTmp[0] );
vPaths.emplace_back( strTemp );
}
}
}
Expand Down
28 changes: 12 additions & 16 deletions sakura_core/CHokanMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -539,26 +539,23 @@ BOOL CHokanMgr::DoHokan( int nVKey )
if( VK_TAB == nVKey && !m_pShareData->m_Common.m_sHelper.m_bHokanKey_TAB ) return FALSE;/* VK_TAB 補完決定キーが有効/無効 */
if( VK_RIGHT == nVKey && !m_pShareData->m_Common.m_sHelper.m_bHokanKey_RIGHT ) return FALSE;/* VK_RIGHT 補完決定キーが有効/無効 */

HWND hwndList;
int nItem;
CEditView* pcEditView;
hwndList = GetItemHwnd( IDC_LIST_WORDS );
nItem = List_GetCurSel( hwndList );
HWND hList = GetItemHwnd( IDC_LIST_WORDS );
const int nItem = List_GetCurSel( hList );
if( LB_ERR == nItem ){
return FALSE;
}
int nLabelLen = List_GetTextLen( hwndList, nItem );
auto pszLabel = std::make_unique<WCHAR[]>(nLabelLen + 1);
List_GetText( hwndList, nItem, &pszLabel[0], nLabelLen + 1 );

std::wstring strLabel;
if( !ApiWrap::List_GetText( hList, nItem, strLabel ) ){
return FALSE;
}

/* テキストを貼り付け */
pcEditView = reinterpret_cast<CEditView*>(m_lParam);
CEditView* pcEditView = reinterpret_cast<CEditView*>(m_lParam);
// Apr. 28, 2000 genta
pcEditView->GetCommander().HandleCommand( F_WordDeleteToStart, false, 0, 0, 0, 0 );
pcEditView->GetCommander().HandleCommand( F_INSTEXT_W, true, (LPARAM)&pszLabel[0], wcslen(&pszLabel[0]), TRUE, 0 );
pcEditView->GetCommander().HandleCommand( F_INSTEXT_W, true, (LPARAM)strLabel.data(), strLabel.length(), TRUE, 0 );

// Until here
// pcEditView->GetCommander().HandleCommand( F_INSTEXT_W, true, (LPARAM)(pszLabel + m_cmemCurWord.GetLength()), TRUE, 0, 0 );
Hide();

return TRUE;
Expand Down Expand Up @@ -658,9 +655,8 @@ void CHokanMgr::ShowTip()
nItem = List_GetCurSel( hwndCtrl );
if( LB_ERR == nItem ) return ;

int nLabelLen = List_GetTextLen( hwndCtrl, nItem );
auto szLabel = std::make_unique<WCHAR[]>(nLabelLen + 1);
List_GetText( hwndCtrl, nItem, &szLabel[0], nLabelLen + 1 ); // 選択中の単語を取得
std::wstring strLabel;
if( !ApiWrap::List_GetText( hwndCtrl, nItem, strLabel ) ) return;

pcEditView = reinterpret_cast<CEditView*>(m_lParam);

Expand All @@ -681,7 +677,7 @@ void CHokanMgr::ShowTip()
if( point.y > m_poWin.y && point.y < m_poWin.y + m_nHeight )
{
::SetRect( &rcHokanWin , m_poWin.x, m_poWin.y, m_poWin.x + m_nWidth, m_poWin.y + m_nHeight );
if( !pcEditView -> ShowKeywordHelp( point, &szLabel[0], &rcHokanWin ) )
if( !pcEditView -> ShowKeywordHelp( point, strLabel.data(), &rcHokanWin ) )
pcEditView -> m_dwTipTimer = ::GetTickCount(); // 表示するべきキーワードヘルプが無い
}
}
Expand Down
121 changes: 121 additions & 0 deletions sakura_core/apiwrap/StdControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.resize( cchRequired );

// GetWindowText() はコピーした文字数を返す。
// https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getwindowtextw
const int actualCopied = ::GetWindowText( hWnd, strText.data(), (int)strText.capacity() );
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 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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がわざわざ存在しているくらいなので、気になりました。

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

自分は実装は確認していませんが 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.resize( 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);
Expand All @@ -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 )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
// バッファをクリアしておく
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);
Expand Down
11 changes: 11 additions & 0 deletions sakura_core/apiwrap/StdControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,15 @@ namespace ApiWrap{
return SetWindowText(hwnd, str.GetStringPtr());
}

/*!
@brief Window テキストを取得する
@param[in] hWnd ウィンドウハンドル
@param[out] strText ウィンドウテキストを受け取る変数
@return 成功した場合 true
@return 失敗した場合 false
*/
berryzplus marked this conversation as resolved.
Show resolved Hide resolved
bool Wnd_GetText( HWND hWnd, std::wstring& strText );

/*!
@brief Window テキストを取得する
@param[in] hwnd ウィンドウハンドル
Expand Down Expand Up @@ -219,6 +228,7 @@ namespace ApiWrap{
// -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- //
// リストボックス //
// -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- //
bool List_GetText( HWND hList, int nIndex, std::wstring& strText );
LRESULT List_GetText(HWND hwndList, int nIndex, WCHAR* pszText, size_t cchText);
template <size_t cchText>
LRESULT List_GetText(HWND hwndList, int nIndex, WCHAR(&pszText)[cchText]) {
Expand Down Expand Up @@ -294,6 +304,7 @@ namespace ApiWrap{
return SetDlgItemText(hwndDlg, nIDDlgItem, str);
}

bool DlgItem_GetText( HWND hDlg, int nIDDlgItem, std::wstring& strText );
UINT DlgItem_GetText(HWND hwndDlg, int nIDDlgItem, WCHAR* pszText, int nMaxCount);

bool TreeView_GetItemTextVector(HWND hwndTree, TVITEM& item, std::vector<WCHAR>& vecStr);
Expand Down
10 changes: 6 additions & 4 deletions sakura_core/debug/Debug1.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
#include <stdio.h>
#include <stdarg.h>

#include "util/string_ex.h"

#if defined(_DEBUG) || defined(USE_RELPRINT)

// -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- //
Expand Down Expand Up @@ -54,10 +56,10 @@ void DebugOutW( LPCWSTR lpFmt, ...)

::DebugBreak();

int count = _vscwprintf( lpFmt, argList );
auto pLargeBuf = std::make_unique<WCHAR[]>( count + 1 );
if( vswprintf( &pLargeBuf[0], count + 1, lpFmt, argList ) > 0 )
::OutputDebugStringW( &pLargeBuf[0] );
std::wstring strTooLongMessage;
vstrprintf( strTooLongMessage, lpFmt, argList );

::OutputDebugStringW( strTooLongMessage.c_str() );
}

va_end(argList);
Expand Down
16 changes: 8 additions & 8 deletions sakura_core/dlg/CDlgAbout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -390,14 +390,14 @@ BOOL CUrlWnd::SetSubclassWindow( HWND hWnd )
SendMessageAny( hWnd, WM_SETFONT, (WPARAM)m_hFont, (LPARAM)FALSE );

// 設定されているテキストを取得する
const ULONG cchText = ::GetWindowTextLength( hWnd );
auto textBuf = std::make_unique<WCHAR[]>( cchText + 1 );
WCHAR* pchText = textBuf.get();
::GetWindowText( hWnd, pchText, cchText + 1 );

// サイズを調整する
auto retSetText = OnSetText( pchText, cchText );
return retSetText ? TRUE : FALSE;
std::wstring strText;
if( ApiWrap::Wnd_GetText( hWnd, strText ) ){
// サイズを調整する
auto retSetText = OnSetText( strText.data(), strText.length() );
return retSetText ? TRUE : FALSE;
}

return FALSE;
}

LRESULT CALLBACK CUrlWnd::UrlWndProc( HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam )
Expand Down
11 changes: 3 additions & 8 deletions sakura_core/dlg/CDlgFind.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,8 @@ void CDlgFind::SetCombosList( void )
while (Combo_GetCount(hwndCombo) > 0) {
Combo_DeleteString( hwndCombo, 0);
}
int nBufferSize = ::GetWindowTextLength( GetItemHwnd(IDC_COMBO_TEXT) ) + 1;
auto vText = std::make_unique<WCHAR[]>(nBufferSize);
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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

::DlgItem_SetText( GetHwnd(), IDC_COMBO_TEXT, m_strText.c_str() );
}
}
Expand All @@ -212,10 +210,7 @@ int CDlgFind::GetData( void )
m_pShareData->m_Common.m_sSearch.m_bNOTIFYNOTFOUND = m_bNOTIFYNOTFOUND; // 検索/置換 見つからないときメッセージを表示

/* 検索文字列 */
int nBufferSize = ::GetWindowTextLength( GetItemHwnd(IDC_COMBO_TEXT) ) + 1;
auto vText = std::make_unique<WCHAR[]>(nBufferSize);
::DlgItem_GetText( GetHwnd(), IDC_COMBO_TEXT, &vText[0], nBufferSize);
m_strText = &vText[0];
ApiWrap::DlgItem_GetText( GetHwnd(), IDC_COMBO_TEXT, m_strText );

/* 検索ダイアログを自動的に閉じる */
m_pShareData->m_Common.m_sSearch.m_bAutoCloseDlgFind = ::IsDlgButtonChecked( GetHwnd(), IDC_CHECK_bAutoCloseDlgFind );
Expand Down
7 changes: 2 additions & 5 deletions sakura_core/dlg/CDlgGrep.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -791,11 +791,8 @@ int CDlgGrep::GetData( void )
m_bGrepSeparateFolder = IsDlgButtonCheckedBool( GetHwnd(), IDC_CHECK_SEP_FOLDER );

/* 検索文字列 */
int nBufferSize = ::GetWindowTextLength( GetItemHwnd(IDC_COMBO_TEXT) ) + 1;
auto vText = std::make_unique<WCHAR[]>(nBufferSize);
::DlgItem_GetText( GetHwnd(), IDC_COMBO_TEXT, &vText[0], nBufferSize);
m_strText = &vText[0];
m_bSetText = true;
m_bSetText = ApiWrap::DlgItem_GetText( GetHwnd(), IDC_COMBO_TEXT, m_strText );;

/* 検索ファイル */
::DlgItem_GetText( GetHwnd(), IDC_COMBO_FILE, m_szFile, _countof2(m_szFile) );
/* 検索フォルダ */
Expand Down
5 changes: 1 addition & 4 deletions sakura_core/dlg/CDlgGrepReplace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,7 @@ int CDlgGrepReplace::GetData( void )
m_bPaste = IsDlgButtonCheckedBool( GetHwnd(), IDC_CHK_PASTE );

/* 置換後 */
int nBufferSize = ::GetWindowTextLength( GetItemHwnd(IDC_COMBO_TEXT2) ) + 1;
auto vText = std::make_unique<WCHAR[]>(nBufferSize);
::DlgItem_GetText( GetHwnd(), IDC_COMBO_TEXT2, &vText[0], nBufferSize);
m_strText2 = &vText[0];
ApiWrap::DlgItem_GetText(GetHwnd(), IDC_COMBO_TEXT2, m_strText2 );

if( 0 == ::GetWindowTextLength( GetItemHwnd(IDC_COMBO_TEXT) ) ){
WarningMessage( GetHwnd(), LS(STR_DLGREPLC_REPSTR) );
Expand Down
Loading