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

カスタムメニューの追加時に配列の範囲外を書き換えないようにする #1445

Merged
merged 1 commit into from Oct 28, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 28, 2020

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 カスタムメニュー設定画面の潜在バグ

参考資料

@ghost
Copy link
Author

ghost commented Oct 28, 2020

berryzplus様

#1440 を読んだ後に存在を思い出したので代理コミットさせていただきました。
自分はPR本文の通りに理解しましたが、間違っていればご指摘ください。

@berryzplus
Copy link
Contributor

#1440 を読んだ後に存在を思い出したので代理コミットさせていただきました。
自分はPR本文の通りに理解しましたが、間違っていればご指摘ください。

拾っていただいて、ありがとうございます。

関連 issue, PR
SAKURA Editor / Bugs / #13 カスタムメニュー設定画面の潜在バグ

自分が書いたやつだと思うんですが、何言ってるか分かんないですね(マテ

int nNum2; //配列サイズの格納先変数
int nIdx2; //配列アクセス用インデックスの格納変数

代入先を取り違ってるよ、というだけの指摘なはずです。

@ghost
Copy link
Author

ghost commented Oct 28, 2020

ご確認ありがとうございます。

代入先を取り違ってるよ、というだけの指摘なはず

代入先が正しくないと本文中で言及するよう、更新しておきました。

@AppVeyorBot
Copy link

Build sakura 1.0.3199 failed (commit 22869ca8d2 by @berryzplus)

@sanomari
Copy link
Contributor

ビルドが失敗してますね。

@AppVeyorBot
Copy link

@ghost
Copy link
Author

ghost commented Oct 28, 2020

またHTMLヘルプでビルドに失敗したみたいです。変更はしていないはずなのですが。
お手数おかけしました。

@k-takata
Copy link
Member

chmのビルドはリトライ回数を増やした方がいいのかな。
いや、AppVeyorはシステムロケールの切り替えに失敗しているのか。GitHub Actionsと同じようにLocale Emulatorを使った方法に移行するのもありか?

Copy link
Contributor

@sanomari sanomari left a 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;
Copy link
Contributor

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を更新してたってことですよね。

m_Common.m_sCustomMenu.m_nCustMenuItemFuncArr[nIdx1][nNum2] = eFuncCode;
m_Common.m_sCustomMenu.m_nCustMenuItemKeyArr[nIdx1][nNum2] = '\0';

これは明らかにダメな香りです。

@ghost
Copy link
Author

ghost commented Oct 28, 2020

レビューありがとうございます。
どなたかマージの代行をお願いします。

念のため自分のリポジトリにてCIビルドを再実行し、成功することを確認しました。

@berryzplus berryzplus merged commit 067b6f1 into sakura-editor:master Oct 28, 2020
@berryzplus
Copy link
Contributor

githubの登録メールアドレスにinvitationが届いているはずっす。

@ghost
Copy link
Author

ghost commented Oct 29, 2020

マージありがとうございました。

メール

届いてました。お手数おかけしました。

@ghost ghost deleted the fix/add_cust_menu_out_of_range branch October 29, 2020 03:11
@beru beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug🦋 ■バグ修正(Something isn't working)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants