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

マクロの文字列型引数における文字列長が未チェックのものがあるのを修正したい #1825

Open
usagisita opened this issue Apr 8, 2022 · 2 comments

Comments

@usagisita
Copy link
Contributor

問題内容

マクロ引数のうち文字列型を扱うものの一部で、ファイルパスなど内部仕様による制限が存在しているにもかかわらず、一切チェックされずバッファがコピーされたり強制終了したりする現状の動作を改善したいです。

再現手順

Editor.FileSaveAs("<260文字を超えるような長い文字列>")

などのマクロを実行します。

またExtHtmlHelpを悪用することで設定によってはwcscpyのバッファオーバーランによりメモリー上の共有データ構造体を破壊することができます。

なおInsText、InsBoxText、AddTailなどで巨大な文字列を指定しメモリー上限に達してクラッシュするようなタイプはこのIssueの考慮対象外としたいです。

再現頻度

いつも

問題のカテゴリ

  • 仕様の問題
  • プログラムの動作上の問題
    • 正式リリース版

その他

参考情報として、FileSaveAsマクロ等でのファイル名に関して、CON、PRN、NUL、AUX、COM1などの特殊ファイル名を指定すると、保存処理の段階でクラッシュしたり応答なしになったりすることもありますが、これも今回のIssueの範囲外としたいです。
対応を考えるのであれば、またIssueなどを作成するなりコメントなどをお願いします。

マクロの文字列長には「\0(NUL)までの長さ(wcslen、lstrlen)」と「文字列全体の長さ==BSTRの長さ」が存在しています。
処理の仕方によっては、この2つの混同により現状では正しく処理されていないケースもあるかもしれません。
例えばInsText、InsBoxTextなどでは文字列の途中にNULが含まれるケースが想定されておりBSTRの長さが使わています。
一方、SearchNext、Replaceなど検索系ではNULまでの長さが使われているようです。NUL文字を検索するには正規表現でのエスケープ処理が必要です。
必要があるのであれば、これらの処置の確認もしたほうがいいかもしれません。

@usagisita
Copy link
Contributor Author

文字列を引数に取る関数
対策が必要そうな関数の一覧暫定版
FileOpen BSTR I4 I4 BSTR
FileSaveAsDialog BSTR I4 I4
FileSaveAs BSTR I4 I4
PutFile BSTR I4 I4
InsFile BSTR I4 I4
Diff BSTR I4
ExecExternalMacro BSTR BSTR
ExecCommand BSTR I4 BSTR
ExtHtmlHelp BSTR BSTR
ExpandParameter BSTR
FileOpenDialog BSTR BSTR
FileSaveDialog BSTR BSTR
FolderDialog BSTR BSTR

対策が必要なさそうな関数の一覧暫定版
InsBoxText BSTR
InsText BSTR
AddTail BSTR
SearchNext BSTR I4
SearchPrev BSTR I4
Replace BSTR BSTR I4
ReplaceAll BSTR BSTR I4
Grep BSTR BSTR BSTR I4
GrepReplace BSTR BSTR BSTR BSTR
KeywordTagJump BSTR
BookmarkPattern BSTR I4
SetMsgQuoteStr BSTR
TraceOut BSTR I4
StatusMsg BSTR I4
IsCurTypeExt BSTR
IsSameTypeExt BSTR BSTR
InputBox BSTR BSTR I4
MessageBox BSTR I4
ErrorMsg BSTR
WarnMsg BSTR
InfoMsg BSTR
OkCancelBox BSTR
YesNoBox BSTR
CompareVersion BSTR BSTR
GetCookie BSTR BSTR
GetCookieDefault BSTR BSTR BSTR
SetCookie BSTR BSTR BSTR
DeleteCookie BSTR BSTR
GetCookieNames BSTR
GetStrWidth BSTR I4
GetStrLayoutLength BSTR I4
IsIncludeClipboardFormat BSTR
GetClipboardByFormat BSTR I4 I4
SetClipboardByFormat BSTR BSTR I4 I4
CreateMenu I4 BSTR

@berryzplus
Copy link
Contributor

大変難しい話題なので放置していました。

「問題内容」に対して「ん?」と思った点が3つありました。

マクロ引数のうち文字列型を扱うものの一部で、ファイルパスなど内部仕様による制限が存在しているにもかかわらず、一切チェックされずバッファがコピーされたり強制終了したりする現状の動作を改善したいです。

  1. マクロ引数のうち文字列型を扱うもの
    BSTR型のマクロ引数は、「文字列」か「バッファ」のどちらかを想定している認識です。
    C/C++での「文字列」とは NULで終端される「文字」のシーケンス です。
    「バッファ」とはメモリー上のアドレス範囲を指す用語です。
    サクラエディタはバイナリファイルを編集できることになってるので、InsTextなどのドキュメントデータ挿入系関数には「バッファ」が渡される構えがあったほうが良いと思います。
  2. ファイルパスなど内部仕様による制限が存在している
    内部仕様には「意図的に仕様化してあるもの」と「既存実装を追認したもの」がある認識です。
    ファイルパスの260文字制約は後者で、CreateFileAがMAX_PATHを超えるパスを扱えないことに起因した制約です。
    issueの雰囲気から「内部仕様に合わせてチェック入れようぜ!」が結論に見えるんですが、「内部仕様=正しい」ではなさそうなので入れようとするチェックによっては「ん?」になる気がします。
  3. 一切チェックされずバッファがコピーされたり強制終了したりする
    チェックしてエラーになったらどうするか?を決める(=レビュアーに理解してもらう)のが大変そうです。
    どうするか?の選択肢を2つ以上思いつかないので、決められないんじゃないかと思います。
    選択肢=コピーする前にメッセージボックスを出して中断し、関数呼出を失敗させる。

意図的に放置していたわけですが、3週間誰も反応してないことが対応の難しさを物語っている気がします。

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

No branches or pull requests

2 participants