正規表現キーワードのインポートで許容サイズを超える文字列を無駄にコピーしているのを修正する #1244

berryzplus · 2020-04-25T04:49:21Z

PR の目的

PR #1034 で入れた不具合を修正します。

もともと入れてしまった不具合は
正規表現キーワードの1個あたりの最大長を超える文字列は切り詰める。
という処理を外してしまったことです。

カテゴリ

不具合修正

PR の背景

#1234 のバグ報告が入った。
　　↓
PR #1034 に問題があると分かった。
　　↓
#1034 (comment)
　　↓
PR向けの修正を作成。
動作確認にて、#1238 のマージが必須であると判明。
　　↓
いったんDraft版でPRを作り、 #1238 のマージ後にrebaseしてDraft解除します。
　　↓
マージされたのでrebaseしてDraft外します。

v2.4.1リリース作成にあたって必須の変更と考えられます。

PR のメリット

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

PR の影響範囲

参考資料

AppVeyorBot · 2020-04-25T05:09:29Z

✅ Build sakura 1.0.2713 completed (commit 7adade8293 by @berryzplus)

sakura-editor#1034 (review) 参照

AppVeyorBot · 2020-04-25T05:55:43Z

✅ Build sakura 1.0.2715 completed (commit 894193e528 by @berryzplus)

beru · 2020-04-25T06:48:37Z

sakura_core/typeprop/CImpExpManager.cpp

@@ -661,11 +661,14 @@ bool CImpExpRegex::Import( const wstring& sFileName, wstring& sErrMsg )
 				}
 				if( k != -1 )	/* 3文字カラー名からインデックス番号に変換 */
 				{
-					if( 0 < MAX_REGEX_KEYWORDLISTLEN - keywordPos - 1 ){
+					// pKeywordに書き込める残りサイズ(NUL終端分を含む)
+					const size_t cchAvailableSize = MAX_REGEX_KEYWORDLISTLEN - keywordPos;


ここって念の為

const size_t cchAvailableSize = MAX_REGEX_KEYWORDLISTLEN - 1 - keywordPos;

にした方が良いのかも？と思いました。681行目でNULL終端を追記しているので。

入力用意して動作確認をまだしていないので不確かですが…。

それはともかく、maxdata.h にある下記の記述を見てコメントが無いので何が何やら（どう使い分ければ？）という感じですｗまぁ連番とかにされないだけ全然マシなのかもしれませんけど。

MAX_REGEX_KEYWORD = 100, //@@@ 2001.11.17 add MIK MAX_REGEX_KEYWORDLEN = 1000, MAX_REGEX_KEYWORDLISTLEN = MAX_REGEX_KEYWORD * 100 + 1,

にした方が良いのかも？と思いました。681行目でNULL終端を追記しているので。

ええ、そうなんですが大丈夫だと思います。

NUL終端する位置 keywordPos を増加させるコードは、
667行目の if( 0 < cchAvailableSize - 1 ) が成立しない場合実行されないからです。

バッファは pKeyword は std::make_unique<wchar_t[]>(MAX_REGEX_KEYWORDLISTLEN); で確保されているので MAX_REGEX_KEYWORDLISTLEN は終端NUL を含むバッファサイズです。

sakura/sakura_core/typeprop/CImpExpManager.cpp

Line 624 in 941ca08

auto szKeyWordList = std::make_unique<wchar_t[]>(MAX_REGEX_KEYWORDLISTLEN);

std::min で比較する相手 MAX_REGEX_KEYWORDLEN も終端NUL を含むバッファサイズで、NULを含むサイズを使う箇所2箇所 vs NULを含まないサイズを使う箇所1箇所だったので NULを含むサイズのほうで const 切った感じです。

berryzplus · 2020-04-25T09:01:37Z

sakura_core/typeprop/CImpExpManager.cpp

 						regexKeyArr[count].m_nColorIndex = k;
-						wcsncpy_s( &pKeyword[keywordPos], MAX_REGEX_KEYWORDLISTLEN - keywordPos, p, _TRUNCATE );
+						wcsncpy_s( &pKeyword[keywordPos], std::min<size_t>(MAX_REGEX_KEYWORDLEN, cchAvailableSize), p, _TRUNCATE );


@usagisita さんが指摘コメント #1034 (comment) で言ってた「元のコードが 完全に正しいかは別 にして」と言ってたのは、たぶんココのことです。

元コードは1キーワードあたりのバッファサイズ MAX_REGEX_KEYWORDLEN と書き込んでよい残り文字数 ‘MAX_REGEX_KEYWORDLISTLEN - keywordPos - 1` を比較して小さいほうをバッファサイズとしています。

変更後コードは1キーワードあたりのバッファサイズ MAX_REGEX_KEYWORDLEN と書き込んでよい残りバッファサイズ ‘MAX_REGEX_KEYWORDLISTLEN - keywordPos` を比較して小さいほうをバッファサイズとしています。

ちゃんと検証してないですが、元コードの実装はマズそうです。
（実害があるのかどうか微妙なレアケースになりますが。）

まずいのは元コードですか？

MAX_REGEX_KEYWORDLISTLEN の最後まできっちり書き込んだときに

sakura/sakura_core/typeprop/CImpExpManager.cpp

Line 681 in 1b4f138

pKeyword[keywordPos] = L'\0';

が１文字はみ出しているような気がしました(無責任に検証を投げ出す姿勢)。

あ、周回遅れでした。>#1244 (comment)

しかしこの答えは答えになっていないと思う。>#1244 (comment)

NUL終端する位置 keywordPos を増加させるコードは、 667行目の if( 0 < cchAvailableSize - 1 ) が成立しない場合実行されないからです

cchAvailableSize が 1２以上あるときに cchAvailableSize がヌル文字を含んだ上限として wcsncpy_s が実行されるのでは。

必要かどうかは知らないが MAX_REGEX_KEYWORDLISTLEN で確保された配列はヌル文字単独だけを書き込まれるか１つ以上のヌル終端正規表現キーワードプラスヌル文字になるように書かれていた、たぶん。

MAX_REGEX_KEYWORDLISTLEN の最後まできっちり書き込んだときに

これはその通り！

PRしてる修正内容だとマズいっすね。
修正します。

beru · 2020-04-25T11:02:29Z

UI側では入力できる文字数を 999 文字までに制限しています。

sakura/sakura_core/typeprop/CPropTypesRegex.cpp

Lines 540 to 547 in 941ca08

    
           /* ダイアログデータの設定 正規表現キーワード */ 
        
           void CPropTypesRegex::SetData( HWND hwndDlg ) 
        
           { 
        
           	HWND		hwndWork; 
        
           	int			i, j; 
        
           	/* ユーザーがエディット コントロールに入力できるテキストの長さを制限する */ 
        
           	EditCtl_LimitText( ::GetDlgItem( hwndDlg, IDC_EDIT_REGEX ), MAX_REGEX_KEYWORDLEN - 1 );

NULL終端含めないで1000文字以上のキーワードは長すぎるので自動で切り詰めて読み込むより、エラーメッセージをLSマクロで読んで設定して return false; する方が良い気がします。呼び出し元は CImpExpManager::ImportUI でそこでメッセージ表示の関数呼んでますね。

beru · 2020-04-25T11:20:52Z

PRのタイトルの 1000文字を超える正規表現キーワードを含む正規表現キーワードセットをインポートすると落ちる ですがこちらで現時点の master (941ca08) で動作確認したところ落ちる動作は確認出来ませんでした。

確認に使用したファイル : test.rkw.txt

あまり長すぎると CRegexKeyword::RegexKeyCheckSyntax で撥ねられてしまうので、1000より少し長いぐらいにしています。

なおリストビューのアイテムには設定したデータのうち先頭の 259 文字分までしか表示されないようです。

https://docs.microsoft.com/en-us/windows/win32/api/commctrl/ns-commctrl-lvitema

あと前のコメントに書きましたがエディット部分にはNULL終端を除いて999文字までで切り詰められます。

ds14050 · 2020-04-25T11:56:37Z

sakura_core/typeprop/CImpExpManager.cpp

@@ -661,21 +661,27 @@ bool CImpExpRegex::Import( const wstring& sFileName, wstring& sErrMsg )
 				}
 				if( k != -1 )	/* 3文字カラー名からインデックス番号に変換 */
 				{
-					if( 0 < MAX_REGEX_KEYWORDLISTLEN - keywordPos - 1 ){
+					// pKeywordに書き込める上限サイズ(NUL終端分を含む)
+					const size_t cchAvailableSize = std::min<size_t>( MAX_REGEX_KEYWORDLEN, MAX_REGEX_KEYWORDLISTLEN - 1 - keywordPos );


変数名とコメントが嘘です。MAX_REGEX_KEYWORDLEN は pKeyword からくる制約ではありません。

符号なし整数に負の値を設定しようとするケースがあります。

符号なし整数に負の値を設定しようとするケースがあります。

考えてなかったです。

AppVeyorBot · 2020-04-25T11:58:59Z

✅ Build sakura 1.0.2717 completed (commit 5a67b4bcb9 by @berryzplus)

berryzplus · 2020-04-25T11:59:04Z

NULL終端含めないで1000文字以上のキーワードは長すぎるので自動で切り詰めて読み込むより、エラーメッセージをLSマクロで読んで設定して return false; する方が良い気がします。呼び出し元は CImpExpManager::ImportUI でそこでメッセージ表示の関数呼んでますね。

これをやるべきか否か思案中・・・

いや、やるべきなんだろうけどメッセージ追加するのが意外とめんどいｗ

テスト用サンプル正規表現キーワードセット作りました。
test.rkw.txt
test.rkwにリネームして使ってください。

ds14050 · 2020-04-25T12:03:24Z

sakura_core/typeprop/CImpExpManager.cpp

 				sErrMsg = LS(STR_IMPEXP_REGEX3);
 			}
 		}
 	}
-	pKeyword[keywordPos] = L'\0';
+	pKeyword[keywordPos] = L'\0';	// 2個目のNUL終端を書き込む


必ずしも２個目ではありません。

berryzplus · 2020-04-25T12:24:46Z

「長過ぎる」メッセージを追加したバージョンをpushしました。

PRのタイトルの 1000文字を超える正規表現キーワードを含む正規表現キーワードセットをインポートすると落ちるですがこちらで現時点の master (941ca08) で動作確認したところ落ちる動作は確認出来ませんでした。

確認に使用したファイル : test.rkw.txt

タイトル微妙に変えました。

確かに落ちないっすね。
取り込んだ正規表現キーワードが使えないみたいなので、少なくとも正常動作はしてなさそう。

PRタイトルもう１回編集しなきゃですね・・・。
もともとの目的が PR #1034 で入れたと考えられる「何らかの不具合」を改修することなので。

AppVeyorBot · 2020-04-25T12:37:30Z

✅ Build sakura 1.0.2718 completed (commit b71f90412d by @berryzplus)

berryzplus · 2020-04-25T13:59:57Z

うん、現master(941ca08) でやっぱり落ちない。

999文字を超えたパターンが含まれていた場合、単に999文字までに切り詰められて表示されます。
- サクラエディタ本体は正常動作します。
- 実際には 999文字を超える部分も取り込まれているため、表示されない「無駄なデータ」を取り込んでしまっています。
  - 無駄なデータのために登録可能パターン数が減る恐れもあるため、この事象は問題です。
正規表現キーワードは /正規表現/k のように PERL 風の「囲み」を付けて定義するものなので、途中で切られると囲みの終端がなくなってしまい「そのパターン」が機能しません。
- 現master ではパターンが途中で着られてても「だんまり」で続行してしまうので、インポートが失敗したことに気付きにくいという問題があります。
- 「囲み」なしパターンの話はややこしいので知らなかったことにします。
- 「囲み」なしパターンであれば「正常に機能するか？」も途中で切られる以上ビミョーだと思います。

グダグダな感じに整理すると、こんなん。

不具合と思しき事象は発生しているがアプリは落ちない。
切り詰めが発生したときに警告が出てない。

で、どうするか。

落ちるのを修正する、は変えないといけない。
- だってアプリ落ちないもん。
じゃあ一体、何を修正したんだろうか？
- 正規表現キーワードの最大サイズを超えていても読み込んでいたのを、最大サイズまでで切り詰めるように修正した。
  - この変更によるメリットはバッファオーバーラン防止というより、無駄なコピーの抑制とメモリの節約にあると思う。
- 切り詰めが発生したことが分かるように新規のメッセージを追加した。
  - この変更によるメリットはインポートが微妙に失敗しているのに傾向が出ないことにより、インポートが完全に成功したと誤認することを防げることにあると思う。

ちょっと変えてみて、文句、いちゃもん他いろいろを募集する感じにします。

beru

コピーしてから長さ調べるんじゃなくて、コピー前に長さ調べて長過ぎたらコピーせずに キーワードが長過ぎる というメッセージをセットして return false; するべきだと思います。

UIで設定出来ない不正な長さのデータ、つまり通常にエクスポートした場合で作れないデータは不正扱いしてよいと思います。

berryzplus · 2020-04-25T16:52:16Z

コピーしてから長さ調べるんじゃなくて、コピー前に長さ調べて長過ぎたらコピーせずにキーワードが長過ぎるというメッセージをセットして return false; するべきだと思います。

同意です！と直しかけましたが、ちょっと提案があります。

今回対処するのは 1キーワードあたりの最大長 を超える場合なんですが、
return false すると「そのキーワード以外」に取り込めるキーワードがある場合でも
インポート操作全体がキャンセルされて全く取り込めない状態になります。

既存の取り込みエラーに対するエラー処理が、
取り込めなかったキーワードは取り込まない（＝取り込めるものは取り込む）
になっています。

無駄なコピーはしないは賛成なのでコピー前にチェックは入れたいです。
return false だと処理全体がエラーになっちゃうので
return せずに処理を続行するカタチにしたいです。
（ここは好みの問題なのかも知れませんが、大筋の挙動は変えないほうがいいような気がします。）

AppVeyorBot · 2020-04-25T17:14:12Z

✅ Build sakura 1.0.2720 completed (commit ac99d5177f by @berryzplus)

beru · 2020-04-25T19:45:06Z

確かに従来の記述でも return false; はあんまりしてないですね。なんか判定追加の分だけ記述が冗長になるのはうーむ…ですがしょうがないのかもですね。

個人的に気になるのは複数のキーワード行でイレギュラーが有った場合に後続の処理でエラーメッセージが上書きされることでしょうか。気にするだけ損かもですが…。

あとインポート処理って既存のリストに追加じゃなくて入れ換え動作っぽいですね。あまり自分がふだんいじらないところなので思わぬ挙動にびっくりする時が有ります。

beru · 2020-04-25T20:01:03Z

sakura_core/typeprop/CImpExpManager.cpp

-						count++;
-						keywordPos += wcsnlen( &pKeyword[keywordPos], MAX_REGEX_KEYWORDLISTLEN - keywordPos ) + 1;
+					// pに入っている文字列の長さ
+					const auto ncpyLength = ::wcsnlen( p, MAX_REGEX_KEYWORDLEN );


行のサイズってここのインスタンスに本来入っているので、メモリスキャンする wcsnlen とかの関数を呼び出すのは本来は良くないと思うんですよね。とはいえ元コードが無駄に行バッファにコピーしてそこから wcslen 呼んでたりするんで 俺のせーじゃねーよ！ と言われたらそうなんですが…。

なんかもうこのメソッドの実装ってぐだぐだなので全体を書き直したいです。まぁテストしないとバグだらけになる未来が見えますが…。:sob:

行のサイズって line.length() のことですかね？

ローカル変数 line の内容はココ（↓）のコメントの内容になってます。

sakura/sakura_core/typeprop/CImpExpManager.cpp

Line 640 in 941ca08

//RxKey[999]=ColorName,RegexKeyword

で、ココのコードの時点でローカル変数 p には RegexKeyword が入ってます。

sakura/sakura_core/typeprop/CImpExpManager.cpp

Line 644 in 941ca08

WCHAR *p = wcsstr(&buff[11], L",");

なので、 line.length() - p - buff としたら p の長さは取れると思います。

そうかくと「なんでやねん！」ってなると思ったので、
編集前コードにあった wcsnlen を使う構成にしてみました。

とはいえ元コードが無駄に行バッファにコピーしてそこから wcslen 呼んでたりするんで俺のせーじゃねーよ！と言われたらそうなんですが…。

実は、編集できる文字数までで切り捨てるなら、コピーは無駄じゃない気もしています。
設定ダイアログ側に「エラーパターンを強調表示する」ような拡張が必要ですが、
登録せずに捨てちゃうと「どのパターンが切り捨てられたか」が分かんないので:smile:

なんかもうこのメソッドの実装ってぐだぐだなので全体を書き直したいです。まぁテストしないとバグだらけになる未来が見えますが…。

メソッド単位でみると意外とまともだと思います:smile:

基底クラスで定義したインターフェースが利用実態とあってないので継承パターンなのに同じコードをコピペして回った感じになってるあたりに不満がありますが、言うほどｇｄｇｄでもない印象です。

C++のコードは単体テストが書けるコードと単体テストを書けないコードに分かれると思っていますが、このあたりのコードは後者だと思います。

テストが書けないから「動かしてみる」以外の方法で検証ができず、書かれたテストコードがないから「検証方法が妥当か？」の検証もできず、結果としてバグが入りこむことになる、という考え方に基づいて「テスト可能な処理を少しずつ切り出してみよう」と思っています。

まあ、要約すると「そのあたりは別件で。」になるわけですが:smile:

不正なデータが含まれてたらその旨を表示してインポート処理をしない、という処理が単純で良いと思いますね。異常データに手厚いケアを行うコスト掛けるのはもったいない気がします。

この機能やユースケースに自分が思い入れが少ないのであまり異常データを救おうという気になれないです。

beru

問題無いと思います。

#1244 (comment) のコメントにあったファイルを使って aaad252 で動作確認は行いました。

beru · 2020-04-25T20:24:14Z

ちょっと気になるのがキーワードが長すぎる場合に結局追加はしない実装になっていますが、キーワードが長過ぎるため切り捨てました。 というメッセージだと切り詰めたけどそのキーワードをまるまる捨ててはいないようにも思えます。とはいえ自然言語は時と共に変遷するので自分の解釈の問題鴨？

berryzplus · 2020-04-26T03:46:06Z

現状で「落ちない」ことが確認できたので IMPORTANT 外します。

berryzplus · 2020-04-26T04:39:24Z

ちょっと気になるのがキーワードが長すぎる場合に結局追加はしない実装になっていますが、キーワードが長過ぎるため切り捨てました。というメッセージだと切り詰めたけどそのキーワードをまるまる捨ててはいないようにも思えます。とはいえ自然言語は時と共に変遷するので自分の解釈の問題鴨？

こっち #1244 (comment) にも書いたんですが、もしかしたら「まるまる捨てたらあかん」のかも知れません。

「切り捨てました」って出たら「どれや？」って確認したくなるのが人情で、
いまこの実装は「切り捨てが発生したら取り込まない」なので、
設定ダイアログ上で「切り捨てられたキーワード」を確認できないっす。。。

berryzplus · 2020-04-26T04:53:45Z

対策を含めようとして キーワード領域がいっぱいなため切り捨てました。 が出るときに、最後のキーワードは登録されてないことを確認しました。

いったん、「長過ぎたら取り込まない」で進めておくことにします。

berryzplus · 2020-04-26T04:54:32Z

レビューありがとうございます。
マージしちゃいます。

berryzplus · 2020-04-26T05:01:10Z

既存の取り込みエラーに対するエラー処理が、
取り込めなかったキーワードは取り込まない（＝取り込めるものは取り込む）
になっています。

正確には、既存コードでは1キーワードあたりのサイズオーバーの判定は行ってなくて、サイズオーバー判定を行ってないから切り捨てを検出せずに「切り捨てられた状態のキーワード」が登録されるという挙動です。

sakura-editor#1244 (comment) なんか違う気がしてきたのでボツにします。

…r1034_buffer_size_limitation 正規表現キーワードのインポートで許容サイズを超える文字列を無駄にコピーしているのを修正する

berryzplus added 🐛bug🦋 ■バグ修正(Something isn't working) IMPORTANT 早急に解消すべきもの labels Apr 25, 2020

berryzplus added 2 commits April 25, 2020 14:24

削ってはいけないサイズ制約を復活させる

9bc1009

sakura-editor#1034 (review) 参照

式に名前を付けて間違いを防止する

1b4f138

berryzplus force-pushed the feature/amend_pr1034_buffer_size_limitation branch from 7e122ca to 1b4f138 Compare April 25, 2020 05:25

berryzplus marked this pull request as ready for review April 25, 2020 05:29

berryzplus mentioned this pull request Apr 25, 2020

SJISエンコードのキーワードヘルプ辞書を設定するとき表示化け・Crashする #1234

Closed

beru reviewed Apr 25, 2020

View reviewed changes

berryzplus commented Apr 25, 2020

View reviewed changes

指摘対応

eb8363a

ds14050 reviewed Apr 25, 2020

View reviewed changes

正規表現キーワードのインポート用に「長過ぎる」メッセージを追加

d7ee72b

beru reviewed Apr 25, 2020

View reviewed changes

コピー前にサイズチェックして、無駄なコピーを行わないようにする

aaad252

beru reviewed Apr 25, 2020

View reviewed changes

beru approved these changes Apr 25, 2020

View reviewed changes

berryzplus removed the IMPORTANT 早急に解消すべきもの label Apr 26, 2020

berryzplus merged commit c0ee803 into sakura-editor:master Apr 26, 2020

berryzplus deleted the feature/amend_pr1034_buffer_size_limitation branch April 26, 2020 04:54

beru mentioned this pull request May 6, 2020

CImpExpRegex::Import の実装を見直し #1273

Merged

This pull request was closed.

正規表現キーワードのインポートで許容サイズを超える文字列を無駄にコピーしているのを修正する #1244

正規表現キーワードのインポートで許容サイズを超える文字列を無駄にコピーしているのを修正する #1244

Conversation

berryzplus commented Apr 25, 2020 • edited Loading

PR の目的

カテゴリ

PR の背景

PR のメリット

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

PR の影響範囲

関連チケット

参考資料

AppVeyorBot commented Apr 25, 2020

AppVeyorBot commented Apr 25, 2020

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

ds14050 Apr 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru commented Apr 25, 2020

beru commented Apr 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Apr 25, 2020

berryzplus commented Apr 25, 2020

Choose a reason for hiding this comment

berryzplus commented Apr 25, 2020

AppVeyorBot commented Apr 25, 2020

berryzplus commented Apr 25, 2020

beru left a comment

Choose a reason for hiding this comment

berryzplus commented Apr 25, 2020

AppVeyorBot commented Apr 25, 2020

beru commented Apr 25, 2020 • edited Loading

beru Apr 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru left a comment

Choose a reason for hiding this comment

beru commented Apr 25, 2020

berryzplus commented Apr 26, 2020

berryzplus commented Apr 26, 2020

berryzplus commented Apr 26, 2020

berryzplus commented Apr 26, 2020

berryzplus commented Apr 26, 2020

berryzplus commented Apr 25, 2020 •

edited

Loading

ds14050 Apr 25, 2020 •

edited

Loading

beru commented Apr 25, 2020 •

edited

Loading

beru Apr 25, 2020 •

edited

Loading