#780 でのCNativeW の初期状態での仕様を取り消して、 #948 の不具合を修正する #949

m-tmatma · 2019-06-14T13:24:08Z

PR の目的

#780 でのCNativeW の初期状態での仕様を取り消して、 #948 の不具合を修正する

カテゴリ

バッファが空の状態で CNativeW::Clear を呼び出したときに落ちる不具合修正 #780 での意図しない仕様変更を元に戻す
不具合修正

PR の背景

#780 で #780 (comment) に対応した結果CNativeW の初期状態での仕様が変更になってしまった。
CNativeW の初期状態での仕様に依存したコードによって #948 の不具合が発生した。

PR のメリット

#948 の不具合を修正できる。

PR のデメリット (トレードオフとかあれば)

特になし

PR の影響範囲

CNativeW の初期状態でバッファが確保されていないことに依存したコード

参考資料

なし

変更手順

バッファが空の状態で CNativeW::Clear を呼び出したときに落ちる不具合修正 #780 が master にマージされた e225bc1 を revert する
e225bc1 のうち、単体テストが revert されたのを戻す (再度登録する)
fcb340b を cherry-pick する

…x-bug-CNative-Clear" This reverts commit e225bc1, reversing changes made to 4245d6e.

tests/unittests/test-cmemory.cpp

tests/unittests/test-cnative.cpp

sakura_core/mem/CMemory.cpp

berryzplus · 2019-06-15T05:14:05Z

sakura_core/mem/CMemory.cpp

+	{
+		assert(m_nRawLen <= m_nDataBufSize-2);
+		m_nRawLen = nLength;
+		assert(m_nRawLen <= m_nDataBufSize-2);


389行目と387行目の違いはなんでしょう？
この話も別件でよいです。

388 行目を実行する前と後で両方とも成立するか確認しています。

コメント足しました

ここ、本当はこういうことだと思います。

if (m_nDataBufSize - 2 <= nLength) throw std::out_of_range("invalid arg"); if (m_nDataBufSize - 2 <= m_nRawLen) throw std::runtime_error("invalid state"); m_nRawLen = nLength;

コード記述上、代入してからチェックしてるので同じ書きっぷりのassertですが、
実態は引数チェックとメンバ変数の状態チェックだと思うんです。

変更後のコードに新しいコメント付けましたが、コードにコメント付けちゃうと「めんどいから後回し！」の大義名分を失うような気がしています。

berryzplus · 2019-06-15T05:24:50Z

sakura_core/mem/CMemory.cpp

-	m_pRawData[m_nRawLen+1]=0; //終端'\0'を2つ付加する('\0''\0'==L'\0')。
+	if (m_nDataBufSize > 0)
+	{
+		assert(m_nRawLen <= m_nDataBufSize-2);


メモ：これはClear関数呼出の前提条件を外すための内部仕様変更。

変更前：利用中サイズが確保済みサイズ未満であればクラッシュ。
変更後：確保済みサイズが0より大きい場合、利用中サイズが確保済みサイズ未満であればクラッシュ。

berryzplus · 2019-06-15T05:28:29Z

sakura_core/mem/CMemory.cpp

-	assert(m_nRawLen <= m_nDataBufSize-2);
-	m_pRawData[m_nRawLen  ]=0;
-	m_pRawData[m_nRawLen+1]=0; //終端'\0'を2つ付加する('\0''\0'==L'\0')。
+	if (m_nDataBufSize > 0)


この条件だと「(メモリ確保状態に関係なく)確保済みサイズが0より大きい場合」と読めます。
個人的な感覚では、if (m_pRawData && m_nDataBufSize) が適切な条件だと思います。

チェックを足しました。

berryzplus · 2019-06-15T09:06:09Z

sakura_core/mem/CMemory.cpp

+	{
+		assert(m_nRawLen <= m_nDataBufSize-2);		// m_nRawLen を変更する前に必要な条件が成立しているか確認する
+		m_nRawLen = nLength;
+		assert(m_nRawLen <= m_nDataBufSize-2);		// m_nRawLen を変更した後も必要な条件が成立しているか確認する


「既存コードそのままインデントしただけなんで、細かいツッコミは勘弁してください」ならassert条件の精査は要らないと思っています。コメント足してしまうと、中身を理解した上でこれでおっけー、という意味になっちゃうような気がします。別件でいいと思っていたassert条件の精査を、いまやんないといけなくなる気がします。

やらなくていいかどうかは置いておいて、ロジック自体は簡単です。

m_nRawLen が実際のデータサイズで、それに終端'\0'を2つ付加しているので
その分の 2 を考慮してサイズが OK かチェックしています。

まあ -2 は左辺に持ってきたほうがいいかもしれませんが。

sakura/sakura_core/mem/CMemory.cpp

Lines 104 to 107 in 3d9d418

memcpy( &m_pRawData[m_nRawLen], pData, nDataLen );

m_nRawLen += nDataLen;

m_pRawData[m_nRawLen] = '\0';

m_pRawData[m_nRawLen+1] = '\0'; //終端'\0'を2つ付加する('\0''\0'==L'\0')。 2007.08.13 kobake 追加

berryzplus · 2019-06-17T16:41:00Z

sakura_core/mem/CMemory.cpp

-	assert(m_nRawLen <= m_nDataBufSize-2);
-	m_nRawLen = nLength;
-	assert(m_nRawLen <= m_nDataBufSize-2);
-	m_pRawData[m_nRawLen  ]=0;


えいやっ！でapproveするタイミングを逸したので、
あらためて元のコードをよく見てみます。

PRの内容は、既に修正目的を果たしているように思うんですが、
メソッドの元々の目的と修正目的を合わせてみたときに「適切な変更かどうか」に疑問があります。

このメソッドでやりたいことは次の2つです。

387行目 利用中サイズに引数「新しいサイズ」を代入する。

389行目 確保済みバッファの引数「新しいサイズ」の位置に文字列終端を書き込む。

他の3行はオマケです。エロい人にはそれが分からんのです・・・が、
386行目のassertが原因でメモリ確保前にClear()してはならないという仕様になっていました。
メモリ確保前はClear()できないとすると、Clear()の前に毎回チェックが必要になります。
これはめんどくさいので、メモリ確保状態に関係なく呼出できるように改善しようとしてます。

今回の修正の第1目的は、メモリ確保前でもClear()できるようにするだと思っています。
PRでは、既存の処理を if (メモリ確保後) で括ることによってこれを実現しています。
これはこれで正しいように思っています。

しかし、ちょっと待ってください。
目的が メモリ確保前でもClear()できるようにする なのに、なんで確保後かどうかを判定してるんでしたっけ？
386行目の前に確保前なら抜けるを足してやればいいような気がします。

if (!m_pRawData || !m_nDataBufSize) { assert(!nLength); return; }

※コードは動確しとりません。

↑ 条件判定文は判定条件が逆になっていますが、内容的にはこの PR の最新のコードとほぼ同じです。
(m_nDataBufSize が int なので負の場合を考慮していないという点が違いますが)

既存のコードを変更せずに追加になるので、提案の方法のほうが差分確認しやすいという面はありますが。

berryzplus · 2019-06-17T17:11:28Z

sakura_core/mem/CMemory.cpp

-	m_nRawLen = nLength;
-	assert(m_nRawLen <= m_nDataBufSize-2);
-	m_pRawData[m_nRawLen  ]=0;
-	m_pRawData[m_nRawLen+1]=0; //終端'\0'を2つ付加する('\0''\0'==L'\0')。


（別件です。）

このコードの有効性に疑問があります。

引数nLengthに偶数を指定した場合には一定の意味があります。
ANSIバッファとして書き込んだバイナリ列をUNICODEバッファとして解釈したい場合、UNICODEのNULは2バイトなので2個目のNULを書く意味があります。

引数nLengthに基数を指定した場合にはやや微妙だと思います。
ANSIバッファは1バイトずつアクセスできますが、UNICODEバッファは2バイト単位でアクセスします。WORD境界のアラインメントを無視して2個目のNULを書く意味はありません。どうせやるなら3個目のNULの要否判断も組み込んで、有効データの後ろに1つ以上の(WORD)NULが含まれることを保証したほうがいいように思います。

　あふぉ　　← 6バイトのsjis文字列　　↓　半角カナに変換して _SetLength(3) する　ｱﾌｫ␀␀ｱ　← 6バイトのsjis文字列

berryzplus · 2019-06-28T15:13:46Z

このPRのステータス。
以下いずれかのアクションを待っています...

「メモリ確保前でもClearできるように」を実現する対応で「メモリ確保後か？」を判定する「もっともらしい理由」の提示
「メモリ確保前か？」を判定する、最小限の改修に変更
他レビュアーのコメント待ち

個人的には 3. で構わないと思っています。
「俺はこれでいい」と approve する人が出てもよいです。

berryzplus

対応ありがとうございます。

assertに付けたコメントは、このまま「追加」でよいと思っています。
「NULを2個付けるコードの有効性」に関する疑問は別途検証でよいような気がしています。

sakura-editor#780 でのCNativeW の初期状態での仕様を取り消して、 sakura-editor#948 の不具合を修正する

m-tmatma added 3 commits June 14, 2019 22:00

Revert "Merge pull request sakura-editor#780 from m-tmatma/feature/fi…

f3737c7

…x-bug-CNative-Clear" This reverts commit e225bc1, reversing changes made to 4245d6e.

単体テストの revert をもとに戻す

551fa2f

バッファが確保されていない状態のときにクリア要求が来たときに落ちる不具合を修正

2dc4aa4

m-tmatma added the 🐛bug🦋 ■バグ修正(Something isn't working) label Jun 14, 2019

m-tmatma added this to the v2.4.0 milestone Jun 14, 2019

m-tmatma mentioned this pull request Jun 14, 2019

.txt のカーソル位置がアウトラインのツリー位置に反映されない #948

Closed

m-tmatma added 2 commits June 15, 2019 06:48

CNativeW を単にインスタンス化した状態ではバッファが確保されていないのを確認するテストを追加

f2e4767

CMemory を単にインスタンス化した状態ではバッファが確保されていないのを確認するテスト

3d9d418

berryzplus reviewed Jun 15, 2019

View reviewed changes

m-tmatma added 5 commits June 15, 2019 16:01

コメント追加

148b445

m_pRawData の NULL チェックも行う

e26009f

コメント追加

e879d09

コメント追加

3b8c8ed

インスタンス化しただけではバッファが確保されないのを確認する

3320767

berryzplus reviewed Jun 15, 2019

View reviewed changes

berryzplus reviewed Jun 17, 2019

View reviewed changes

判定条件変更

4b9cbb9

berryzplus approved these changes Jun 29, 2019

View reviewed changes

m-tmatma merged commit abf90da into sakura-editor:master Jun 29, 2019

m-tmatma deleted the feature/fix-up-PR780 branch June 29, 2019 05:16

m-tmatma mentioned this pull request Aug 4, 2019

行データ管理クラスでメモリプールを使う事でメモリ確保と解放処理を高速化 #980

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#780 でのCNativeW の初期状態での仕様を取り消して、 #948 の不具合を修正する #949

#780 でのCNativeW の初期状態での仕様を取り消して、 #948 の不具合を修正する #949

m-tmatma commented Jun 14, 2019

berryzplus Jun 15, 2019

m-tmatma Jun 15, 2019

m-tmatma Jun 15, 2019

berryzplus Jun 15, 2019

berryzplus Jun 15, 2019

berryzplus Jun 15, 2019

m-tmatma Jun 15, 2019

berryzplus Jun 15, 2019

m-tmatma Jun 15, 2019

berryzplus Jun 17, 2019

m-tmatma Jun 21, 2019 •

edited

Loading

m-tmatma Jun 21, 2019

berryzplus Jun 17, 2019

berryzplus commented Jun 28, 2019

berryzplus left a comment

	memcpy( &m_pRawData[m_nRawLen], pData, nDataLen );
	m_nRawLen += nDataLen;
	m_pRawData[m_nRawLen] = '\0';
	m_pRawData[m_nRawLen+1] = '\0'; //終端'\0'を2つ付加する('\0''\0'==L'\0')。 2007.08.13 kobake 追加

#780 でのCNativeW の初期状態での仕様を取り消して、 #948 の不具合を修正する #949

#780 でのCNativeW の初期状態での仕様を取り消して、 #948 の不具合を修正する #949

Conversation

m-tmatma commented Jun 14, 2019

PR の目的

カテゴリ

PR の背景

PR のメリット

PR のデメリット (トレードオフとかあれば)

PR の影響範囲

関連チケット

参考資料

変更手順

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-tmatma Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus commented Jun 28, 2019

berryzplus left a comment

Choose a reason for hiding this comment

m-tmatma Jun 21, 2019 •

edited

Loading