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

バッファが空の状態で CNativeW::Clear を呼び出したときに落ちる不具合修正 #780

Merged

Conversation

m-tmatma
Copy link
Member

@m-tmatma m-tmatma commented Feb 2, 2019

バッファが空の状態で CNativeW::Clear を呼び出したときに落ちる不具合修正
(#777 によるデグレ)

@m-tmatma m-tmatma added this to the next release milestone Feb 2, 2019
@berryzplus
Copy link
Contributor

assertの書きっぷりを見る限り、バグでなく「仕様」だと思います。

つまり、サイズチェックなしに Clear() を呼んではならない、ということですね。

たぶんそれだと困るので、assert落ちしないように対策を入れる必要はあると思います。

assertの条件とif文の条件が違うのが気になりました。

m-tmatma added a commit to m-tmatma/sakura that referenced this pull request Feb 2, 2019
@berryzplus
Copy link
Contributor

別解です。

CNative::CNative( void )
    : CMemory( NULL, 0 ) {}

原因はCMemoryの引数なしコンストラクタが最低限のメモリ確保をしないことなので、中間クラスのコンストラクタを引数ありにしてしまえば(=CNativeTが必ずメモリ付きで初期化されるようにすれば)解決すると思います。

@m-tmatma
Copy link
Member Author

m-tmatma commented Feb 2, 2019

別解です。

こちらにしました。

@m-tmatma
Copy link
Member Author

m-tmatma commented Feb 2, 2019

assertの書きっぷりを見る限り、バグでなく「仕様」だと思います。

引数無しで CMemory のコンストラクタが呼ばれると、m_nDataBufSize が 0 で
初期化されるので、使い方によっては落ちるのでバグと言えると思います。

引数無しで CMemory のインスタンスが作成されて _SetRawLength が引数 に 0 が渡されて
呼ばれることを想定してないだけなので、単に assert が間違っているだけかと思います。

@m-tmatma
Copy link
Member Author

m-tmatma commented Feb 2, 2019

別解です。

こちらにしました。

予定変更

引数無しで CMemory のコンストラクタが呼ばれた場合に、最低限のメモリ確保されるようにしました。

@m-tmatma
Copy link
Member Author

m-tmatma commented Feb 2, 2019

CMemory に対するテストを追加

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

問題ないと思います。

vs2017の4環境でビルドして問題なく、test1の28個のテストがOKになってるのを見ました。
何かあったら見つかった時点でまた改善していけばいいと考えています。

@m-tmatma m-tmatma merged commit e225bc1 into sakura-editor:master Feb 3, 2019
@berryzplus
Copy link
Contributor

別解です。

こちらにしました。

予定変更

引数無しで CMemory のコンストラクタが呼ばれた場合に、最低限のメモリ確保されるようにしました。

そうですよね。CNative::Clear() が問題なら CNative に対処すればいいですが、
CMemory::_SetRawLength() が問題っぽいので CMemory に対処しないといかん気がしました 😄

追加された CMemory のテストを見て、どっかで他のケースも書かないといかんな、と思ったんですがケース書くのは意外とめんどくさいっていう、アレですね・・・。

@m-tmatma m-tmatma deleted the feature/fix-bug-CNative-Clear branch February 3, 2019 05:59
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
…ative-Clear

バッファが空の状態で CNativeW::Clear を呼び出したときに落ちる不具合修正
m-tmatma added a commit to m-tmatma/sakura that referenced this pull request Jun 14, 2019
…x-bug-CNative-Clear"

This reverts commit e225bc1, reversing
changes made to 4245d6e.
m-tmatma added a commit that referenced this pull request Jun 29, 2019
#780 でのCNativeW の初期状態での仕様を取り消して、 #948 の不具合を修正する
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
…x-bug-CNative-Clear"

This reverts commit e225bc1, reversing
changes made to 4245d6e.
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
sakura-editor#780 でのCNativeW の初期状態での仕様を取り消して、 sakura-editor#948 の不具合を修正する
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants