-
Notifications
You must be signed in to change notification settings - Fork 167
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
カスタムメニューの追加時に配列の範囲外を書き換えないようにする #1445
Conversation
berryzplus様 #1440 を読んだ後に存在を思い出したので代理コミットさせていただきました。 |
拾っていただいて、ありがとうございます。
自分が書いたやつだと思うんですが、何言ってるか分かんないですね(マテ int nNum2; //配列サイズの格納先変数
int nIdx2; //配列アクセス用インデックスの格納変数 代入先を取り違ってるよ、というだけの指摘なはずです。 |
ご確認ありがとうございます。
代入先が正しくないと本文中で言及するよう、更新しておきました。 |
❌ Build sakura 1.0.3199 failed (commit 22869ca8d2 by @berryzplus) |
ビルドが失敗してますね。 |
✅ Build sakura 1.0.3203 completed (commit 22869ca8d2 by @berryzplus) |
またHTMLヘルプでビルドに失敗したみたいです。変更はしていないはずなのですが。 |
chmのビルドはリトライ回数を増やした方がいいのかな。 |
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.
危険な香りのするコードですが変更に問題はなさそうに見えました。
@@ -492,7 +492,7 @@ INT_PTR CPropCustmenu::DispatchEvent( | |||
} | |||
nNum2 = List_GetCount( hwndLIST_RES ); | |||
if( LB_ERR == nNum2 ){ | |||
nIdx2 = 0; | |||
nNum2 = 0; |
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.
LB_ERR = -1 だったと思います。
nNum2 == -1 のときに、nNum2を放置してnIdx2を更新してたってことですよね。
sakura/sakura_core/prop/CPropComCustmenu.cpp
Lines 519 to 520 in 4b14223
m_Common.m_sCustomMenu.m_nCustMenuItemFuncArr[nIdx1][nNum2] = eFuncCode; | |
m_Common.m_sCustomMenu.m_nCustMenuItemKeyArr[nIdx1][nNum2] = '\0'; |
これは明らかにダメな香りです。
レビューありがとうございます。 念のため自分のリポジトリにてCIビルドを再実行し、成功することを確認しました。 |
githubの登録メールアドレスにinvitationが届いているはずっす。 |
マージありがとうございました。
届いてました。お手数おかけしました。 |
PR の目的
sf.netで指摘されていた潜在バグに対する修正パッチを取り込みます。
カテゴリ
PR の背景
カスタムメニューの設定画面には、メニュー項目を追加する処理において、特定の条件下で実行される処理に誤りがあるため、後続の処理にて配列の範囲外にアクセスする潜在バグが存在します。
メニュー項目の追加処理では、コンボボックスで選択されたメニュー配列のリストアイテム数をnNum2に、そのうち選択されているもののインデックスをnIdx2に格納しています。
ここでそれぞれの数を取得できなかった(=返り値がLB_ERRだった)場合、その配列の先頭に項目を追加するものとして処理を続行しますが、このときnNum2がLB_ERRだった場合に先頭の要素を参照するための値を格納する代入先が正しくないため、その後の配列へのアクセス時に範囲外の領域を参照していました。
bug#13のパッチを取り込んでこの問題を修正します。
PR のメリット
PR のデメリット (トレードオフとかあれば)
仕様・動作説明
テスト内容
通常の使用ではこの問題の影響はありません。
v2.4.1と修正後のビルドとの間で、カスタムメニューの項目追加・削除の動作とメニューの表示に変わりがないことを確認しました。
PR の影響範囲
共通設定ダイアログ → カスタムメニュー → メニュー項目の追加
関連 issue, PR
SAKURA Editor / Bugs / #13 カスタムメニュー設定画面の潜在バグ
参考資料