CPPA::stdErrorの処理が不正っぽい

usagisita commented 3 years ago

問題内容

CPPA.cppのコードを眺めていたところ、バグっぽいコードを見つけたので報告だけしておきます。

■1つめ https://github.com/sakura-editor/sakura/blob/2deb79197c608f00897453a762d9178d07fc59e5/sakura_core/macro/CPPA.cpp#L353 このifは前のループでヒットしなかった場合なので、m_MacroFuncInfoCommandArr[i] == -1ではないでしょうか。現状では処理対象が間違っている上、iが範囲外アクセスになる可能性がありそうです。

■2つめ https://github.com/sakura-editor/sakura/blob/2deb79197c608f00897453a762d9178d07fc59e5/sakura_core/macro/CPPA.cpp#L381 ppa.dllが細工された、または特殊なもの(自作言語のマクロDLLとか)だったりした場合に、Err_Mesが約2000文字以上を返してくると、szMesがバッファオーバーフローする危険があります。

berryzplus commented 2 years ago

指摘内容は正しいと思いますが、周辺課題が多いので放置してました。

if( szFuncDec[0] == '\0' ){

~~ここでやってるのは「機能名の取得」です。~~

~~関数系機能の定義を走査して、機能番号が見つからなかったらコマンド系機能を走査する~~ ~~をやりたいんだと思うので、取得できてるかを見た方が良さそうに思いました。~~ (追記：マクロコマンドでもマクロ関数でも正しく名前が取れることを確認しました。)

// L"未定義のエラー\nError_CD=%d\n%s"
auto_snprintf_s( szMes, _countof(szMes), LS(STR_ERR_DLGPPA5), Err_CD, to_wchar(Err_Mes) );

修正により szMes に入りきらない部分は切り捨てられるようになり、心配しているオーバーフローは起きなくなります。

ここに関しては、どちらかというと「メッセージ」を「文字列リソース」として格納していることが真因だと思っています。

「あるべき姿」は、適切なIDで「メッセージリソース」を定義してあげることなんですが、それをやると影響が大きいのでまだ当面放置かな？と思います。

berryzplus commented 2 years ago

IsBadStringPtr は使わないほうがよいです。

IsBadStringPtr には副作用があり、関数名の印象に反して「指定された文字列ポインタが不正であることを安全に確認する」ことはできないためです。 https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-isbadstringptra

berryzplus commented 2 years ago

逆ですね・・・

関数系機能の定義を走査して、機能番号が見つからなかったらコマンド系機能を走査するをやりたいんだと思うので、取得できてるかを見た方が良さそうに思いました。

先にマクロコマンド 320件から機能IDを探し、なかったらマクロ関数 61件からも探す、がやりたいことです。

if 文の記述が誤っていることによる効果は、以下です。

berryzplus commented 2 years ago

テストコードを書いてみたところ、問題「１つめ」は不具合でなかったことが分かりました。

コメント修正しときます・・・。

テストコードの追加とIsBadStringPtrAを使ってしまってる件の修正は、引き続き対応していこうと思います。