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

[usagisita]

問題内容

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]

総評

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

~~問題「1つめ」の直し方~~

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

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

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

問題「2つめ」の直し方

// 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]

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

https://github.com/sakura-editor/sakura/blob/2deb79197c608f00897453a762d9178d07fc59e5/sakura_core/macro/CPPA.cpp#L367-L371

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

[berryzplus]

逆ですね・・・

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

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

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

• エラー発生元がマクロ関数だった場合に、機能名が取れない。（実害あり）
• エラー発生元がマクロコマンドだった場合に、無駄な検索が走る。（実害なし）

[berryzplus]

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

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

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