マクロの Python 対応 #1866
マクロの Python 対応 #1866
Conversation
|
✅ Build sakura 1.0.4182 completed (commit 175294c129 by @beru) |
| for (size_t i = 0; i < _countof(symbols); ++i) { | ||
| auto& s = symbols[i]; | ||
| auto sym = ::GetProcAddress(s_hModule, s.name); | ||
| *(void**)s.ptr = sym; |
There was a problem hiding this comment.
キャストでエラーが出ています。
| *(void**)s.ptr = sym; | |
| *(FARPROC*)s.ptr = sym; |
There was a problem hiding this comment.
MinGW のビルドでエラーが起きていますね。
2022-10-02T16:38:46.0385436Z D:/a/1/s/sakura_core/macro/CPythonMacroManager.cpp: In member function 'virtual bool CPythonMacroManager::ExecKeyMacro(CEditView*, int) const':
2022-10-02T16:38:46.0717050Z D:/a/1/s/sakura_core/macro/CPythonMacroManager.cpp:933:34: error: invalid conversion from 'long long int (*)()' to 'void*' [-fpermissive]
2022-10-02T16:38:46.1208526Z 933 | *(void**)s.ptr = sym;
2022-10-02T16:38:46.1213459Z | ^~~
2022-10-02T16:38:46.1242539Z | |
2022-10-02T16:38:46.1277863Z | long long int (*)()
うーん、右辺の関数ポインタをキャスト無しで void* に暗黙的に 変換 代入 するのは無しだよって事かな。。
There was a problem hiding this comment.
ff2b219 で右辺を明示的に void* にキャストするようにコードを変更しました。
今使っている環境にMinGWを入れていないので（まぁ入れればいいんですが…。）、ビルドエラーが取れるかはCIのログで確認したいと思います…。
| ::VariantInit(&vtArgs[i]); | ||
| if (varType == VT_BSTR) { | ||
| const char* str = PyUnicode_AsUTF8(arg); | ||
| SysString S(str, (int)strlen(str)); |
There was a problem hiding this comment.
strlen(str)がセキュリティ問題を含んでいます。
strがNUL終端されていない場合、アクセスしてはいけない領域を読み込んでしまいます。
There was a problem hiding this comment.
PyUnicode_AsUTF8AndSize を使って取得したサイズを使うようにすれば良さそうですね。
https://docs.python.org/3/c-api/unicode.html#c.PyUnicode_AsUTF8AndSize
There was a problem hiding this comment.
ff2b219 で PyUnicode_AsUTF8AndSize を使うように変更しました。
この関数は Python 3.3 で追加されたようで 3.10 から Stable API になったようです。
自分は Python 3.9.6 をインストールしていて python3.dll にPATHが通っている環境でしか動作確認していないので、環境によっては正しく動作しないかもしれません。
k-takataさんが #1327 (comment) してくれたように公式版のPythonのインストール先はレジストリから取れるようなので、ユーザーに優しくするならPATHが通っていない環境でも実行できるようにするべきかもしれません。
| ::VariantInit(&vtArgs[i]); | ||
| if (varType == VT_BSTR) { | ||
| Py_ssize_t sz = 0; | ||
| const char* str = PyUnicode_AsUTF8AndSize(arg, &sz); |
There was a problem hiding this comment.
エラー時に NULL が返ると記載されていますが、それの考慮が出来ていません。
https://docs.python.org/3/c-api/unicode.html#c.PyUnicode_AsUTF8AndSize
There was a problem hiding this comment.
現時点の更新された記述では PyUnicode_AsWideCharString を使うようにしていますが、戻り値をちゃんと見ていないのは同様です。エラー処理をちゃんと書くように今後直します。。
|
❌ Build sakura 1.0.4183 failed (commit 4bed86e126 by @beru) |
| { | ||
| static HMODULE s_hModule; | ||
| if (!s_hModule) { | ||
| s_hModule = LoadLibrary(L"python3.dll"); |
There was a problem hiding this comment.
codefactor が Possible use of current directory (CWE-829) の問題報告をしています。
https://www.codefactor.io/repository/github/sakura-editor/sakura/pull/1866
DLLプリロード攻撃に該当するようです。
サクラエディタでは LoadLibraryExedir 関数が用意されていて、これはDLLインジェクション対策としてカレントディレクトリをEXEのディレクトリに変えてから LoadLibrary APIを呼び出すようにしています。
サクラエディタがDLLインジェクション対策をしないとどういう問題が起きうるのかをあまり分かっていませんが、ユーザー環境に不正な python3.dll ファイルが置かれている場所からサクラエディタを起動されると、不正なコードが実行される可能性が生じるので良くないという事のような気がします。
そうなるとやはり #1327 (comment) に書かれているように公式版のPythonのインストール先はレジストリから取ったり、サクラエディタの設定情報としてPythonのDLLをどこからLoadするかを持つようにする必要が出てきそうです。
There was a problem hiding this comment.
12ba7b8 で LoadLibrary の代わりに LoadLibraryExedir を使うように変更しました。
There was a problem hiding this comment.
LoadLibrary関数が失敗した場合にエラーメッセージが表示されないので、Pythonスクリプトを実行したのかしていないのか判断がしにくい事に気づきました。例えば、64bit版のPythonをインストールした環境でWin32のサクラエディタを起動しても、python3.dll の読み込みが出来ません。
という事で 5bc588b で失敗時にエラーメッセージを表示する処理を追加しました。
| if (varType == VT_BSTR) { | ||
| Py_ssize_t sz = 0; | ||
| const char* str = PyUnicode_AsUTF8AndSize(arg, &sz); | ||
| SysString S(str, (int)sz); |
There was a problem hiding this comment.
strはchar*ですがUTF-8、SysStringのchar*版はANSI/CP932ではありませんか。
There was a problem hiding this comment.
指摘されるまで意識出来てませんでしたが、確かにそうですね（地域設定の Beta: Use Unicode UTF-8 for worldwide language support を指定した時の挙動はよくわからない）。
sakura/sakura_core/_os/OleTypes.h
Lines 34 to 40 in b0b7b83
MultiByteToWideChar に CP_UTF8 を指定して UTF-8 から UTF-16LE に変換出来るようですが、そのWindowsAPIを使うべきか、それとも内部実装の CUtf8::UTF8ToUnicode を使うべきかはちょっと判断が付きません。後者はいったん CNativeW 型と CNativeA 型を使う必要が出そうですね。
There was a problem hiding this comment.
handleCommand の方だと PyUnicode_AsWideCharString を使っていますね。それを使う方がシンプルに済みそうです。というか戻り値を返すかどうかという違いなので引数の処理は共通化出来そう…。。
追記 : std::wstring と VARIANT という違いが存在
| std::vector<PyMethodDef> g_commandDescs; | ||
| std::vector<std::string> g_functionNames; | ||
| std::vector<PyMethodDef> g_functionDescs; | ||
| CEditView* g_pEditView; |
There was a problem hiding this comment.
#1327 (comment) で usagisita さんから指摘されている点として、グローバル変数 g_pEditView を使ってるけど再入可能性を考慮していないっぽいよという事ですが、はい考慮していませんでした。。
There was a problem hiding this comment.
bc086a4 でグローバル変数 g_pEditView を使うのを止めて、&GetEditWnd().GetActiveView() で CEditView* を取るようにしました。
|
|
||
| g_pEditView = EditView; | ||
|
|
||
| PyObject* pCode = Py_CompileString(m_str.c_str(), to_achar(m_pszPath), Py_file_input); |
There was a problem hiding this comment.
m_strはUTF-8だと思いますが、to_achar(m_pszPath)はCP932でいいんでしょうか。
Pythonのドキュメントを軽く見ただけでは、これがどのように解釈されるのか分からないので、なんとも言えませんが。
There was a problem hiding this comment.
良く分からなかったのでPythonのソースコードを読んでみました。
ドキュメントに書かれているように Py_CompileString は Py_CompileStringExFlags のラッパーで
Py_CompileStringExFlags の filename 引数のコメントに decoded from the filesystem encoding と書かれていました。
実装では PyUnicode_DecodeFSDefault を呼んでdecodeしているようです。
https://docs.python.org/ja/3/c-api/unicode.html#c.PyUnicode_DecodeFSDefault
https://docs.python.org/ja/3/glossary.html#term-filesystem-encoding-and-error-handler
https://docs.python.org/ja/3/glossary.html#term-locale-encoding
という事で多分今のままでも良いのかもしれませんが、テストはしておらず良く分かりません。
There was a problem hiding this comment.
少し前のバージョンからWindowsでもfilesystem encodingはUTF-8になったはずです。
>>> import sys; print(sys.getfilesystemencoding())
utf-8
There was a problem hiding this comment.
情報ありがとうございます。
自分の環境でも同じ結果になる事が確認出来ました。
という事で CPythonMacroManager::LoadKeyMacro メソッドで WideCharToMultiByte(CP_UTF8 を使って事前に UTF-8 文字列を作るようにします。 （m_pszPath メンバ変数は std::string m_path とかに変えて）
There was a problem hiding this comment.
filesystem encodingがutf-8になったのは3.6からのよう。
https://peps.python.org/pep-0529/
全てのデフォルトがutf-8になるのは3.15かららしいですが。
https://methane.hatenablog.jp/entry/2022/04/26/Python_3.15%E3%81%8B%E3%82%89%E3%83%87%E3%83%95%E3%82%A9%E3%83%AB%E3%83%88%E3%81%AE%E3%82%A8%E3%83%B3%E3%82%B3%E3%83%BC%E3%83%87%E3%82%A3%E3%83%B3%E3%82%B0%E3%81%8CUTF-8%E3%81%AB%E3%81%AA%E3%82%8A
There was a problem hiding this comment.
3.6 より前のバージョンへの対応はとりあえず考えない事にします。
引数の filename はエラーメッセージとかに使われるようです。Python実行のエラーメッセージの取得は PyErr_Fetch を使う事で出来ました。https://stackoverflow.com/a/1418703
しかしサクラエディタのマクロ実行のエラーを出力するウィンドウが無さそうなのでどこにエラーメッセージを出すか悩みます。WSHマクロではどうしているかを調べてみようと思います。
There was a problem hiding this comment.
サンプルの tools/macro/CopyDirPath/CopyDirPath.js ファイルをコピーしてからエラーが起きるように変更して（CopyDirPath.js.txt (拡張子偽装)）実行してみました。すると下記のメッセージボックスが出ました。
どこで出ているかを確認してみたところ、CWSHMacroManager::ExecKeyMacro から呼び出される CWSHClient::Execute から呼び出す Parser->ParseScriptText の呼び出しのところでエラー用のコールバック処理が呼び出されてメッセージボックスが表示されていることが分かりました。
という事でエラー時には同じようにメッセージボックスでエラー内容を表示してあげる必要がありそうです。
| // static | ||
| CMacroManagerBase* CPythonMacroManager::Creator(const WCHAR* FileExt) | ||
| { | ||
| if (wcscmp( FileExt, L"py" ) == 0) { |
There was a problem hiding this comment.
_wcsicmp 関数を使うように更新しました。
|
|
||
| BOOL CPythonMacroManager::LoadKeyMacro(HINSTANCE hInstance, const WCHAR* pszPath) | ||
| { | ||
| FILE* f = _wfopen(pszPath, L"rb"); |
There was a problem hiding this comment.
一応メモとして、ファイルオープンのエラーチェックぅは欲しいですね。
There was a problem hiding this comment.
そのエラーチェックは必要ですね。修正します。
|
✅ Build sakura 1.0.4184 completed (commit 9f6b5024cf by @beru) |
|
✅ Build sakura 1.0.4185 completed (commit 2496668abb by @beru) |
| wide2utf8(m_strPath, pszPath); | ||
| long sz = _filelength(_fileno(f)); | ||
| m_strMacro.resize(sz); | ||
| fread(&m_strMacro[0], 1, sz, f); |
There was a problem hiding this comment.
やってみないでいうのもなんなのですが、BOMがあっても動くのでしょうか。
あと他のタイプのマクロは規定がSJISでUTF-8はBOM付のときだけ有効ですね。
他の実装では読み込み時にBOMがデータから削除されていたと思います。
もっとも2022年の現代からするとPython実装では規定でUTF-8で構わないと思います。
There was a problem hiding this comment.
BOMの事を考えていなかったですが確認が必要ですね。
There was a problem hiding this comment.
動作確認してみたところ、BOMがあると動きませんでした。
なのでUTF-8 BOM付きのpyファイルを読んでも動くように対策を入れようと思います。
なお本来は色々な文字エンコーディングのファイルを読めるテキストエディタなので、仮にファイル読み込み処理をうまく再利用出来るならそうするのが良いと思いますが、（多分出来なそうなので）限定した文字エンコーディングのファイルのみに対応という事になると思います。
|
✅ Build sakura 1.0.4186 completed (commit 2c648564d8 by @beru) |
|
✅ Build sakura 1.0.4187 completed (commit ee22350f6b by @beru) |
|
✅ Build sakura 1.0.4188 completed (commit c73651a02d by @beru) |
|
SonarCloud Quality Gate failed.
|
|
✅ Build sakura 1.0.4189 completed (commit eaad38d484 by @beru) |
|
✅ Build sakura 1.0.4204 completed (commit bae93df920 by @beru) |
|
✅ Build sakura 1.0.4205 completed (commit a7b366d02a by @beru) |
|
仕様の疑問点です。 import SakuraEditor
#SakuraEditor.FileNew()
|
|
CIのチェックに関して。
結局、何をもってOKにしますか？になりそう。 |
|
@berryzplus さん
Python では行コメントに # 文字を使います。shell script といっしょですね。 |
|
@berryzplus さん
おおまかには、Pythonスクリプトで書いたサクラエディタのマクロを実行できる環境が整った段階でOKじゃないかなと思います。 動作確認に関しては、どのバージョンのPythonから動作するか、マクロの関数とコマンドが一通りの問題無く動くのかの確認等をした方が良さそうですが、これはPR作成者がまず行うべきですね。 ユーザーが使う際にサクラエディタのインストールだけでなく追加でPythonのインストールも必要になるので、少し導入に敷居があるかもしれないですね。 |
これは「どっちがいい？」と聞いてます。 「やりたい」「やりたくない」「どっちでもいい」があって、 かなり面倒だと思うので「やらん」でOKです。
これはコードを読み違えてたのでスルーしてもらって大丈夫です。
「マクロの関数とコマンドが一通り問題なく動く」を確認する方法をどうしましょう。 python + appium でRPAできないか検討してみてますが、
CIでのテスト、開発時に行うテストに限定して考えれば、 |
私個人の意見としては「どっちでもいい」です。そういえばマクロのPython対応をするとなるとヘルプの更新も必要になりそうですね。
手動確認を考えていました。テスト結果のEvidenceが自己申告になりますが…。 Python + Appium でGUI自動操作して確認は出来ると思いますが、それを使ったテストケースの用意とパスがPRのマージに必要、という条件を設定してほしくはないです。個人的には、新しいハードルを追加で用意してそれをクリアしないと駄目、というように決めるのはPRがマージされるペースが落ちてしまうのでやってほしくないです。
CIでのテストに関しては、CIのサービスが用意している最新のPythonを使うので良さそうですね。 開発時に行うテストに関してはPython必須にはしたくないです。 サクラエディタのインストーラーにPythonランタイムを組み込むことは考えていません。Portable Winpythonを使う事で可能かもしれませんが、容量が無駄に増えてしまうのは好ましくありません。 |
はい。では「やらない」で。
ヘルプは今回触らないで良い気がします。
何を確認したかが残れば、手動確認でよいです。 このPRがマージされるまでに機能テストのPR(python必須)を出すかも知れませんが、連携させる必要はないと思ってます。 |
|
✅ Build sakura 1.0.4219 completed (commit 4498c98ac3 by @beru) |
|
✅ Build sakura 1.0.4220 completed (commit 252c50badd by @beru) |
|
SonarCloud Quality Gate failed.
|
|
✅ Build sakura 1.0.4221 completed (commit d9ffc5fa5d by @beru) |
make PyObjectPtr noncopyable auto call Py_XDECREF show error message when LodLibrary fails CPythonMacroManager.cpp で g_pEditView グローバル変数を使うのを止める 警告除去 remove unused utf8_to_utf16le function show Python error message with MessageBox use LoadLibraryExedir function use PyErr_Fetch to get error message string detect and erase UTF-8 BOM from fread string in CPythonMacroManager::LoadKeyMacro use UTF-8 encoding string for filename argument of Py_CompileString function add error checks update handleFunction to use PyUnicode_AsWideCharString instead of PyUnicode_AsUTF8AndSize use case-insensitive string comparison for python file extension add file open error check in CPythonMacroManager::LoadKeyMacro method call GetProcAddress only after DLL is loaded use PyUnicode_AsUTF8AndSize instead of PyUnicode_AsUTF8 so that vulnerable strlen can be avoided explicitly cast function pointer to void* before copying it to void* type variable マクロの Python 対応
|
|
✅ Build sakura 1.0.4369 completed (commit 7b297adaab by @beru) |
6 participants
PR の目的
サクラエディタのマクロを Python で書けるようにする事が目的です。
カテゴリ
PR の背景
サクラエディタは WSH(JScript, VBScript)マクロ と PPAマクロが使えます。PPAマクロは 32bit 版のみ対応です。
最近Pythonが広く使われるようになっているので、それに対応したくなりました。
このPRでは
LoadLibraryしたDLLからGetProcAddressを使って明示的なリンクでシンボルを探して使用する事で、ビルド環境にPythonが無くてもビルド出来るようにしています。PR のメリット
PR のデメリット (トレードオフとかあれば)
仕様・動作説明
メニューの
ツール>名前を指定してマクロ実行を選ぶとファイルダイアログが表示されますが、そこで拡張子が*.pyの Python ファイルを選べるようにしました。Python スクリプト側では
import SakuraEditorと記述してそのモジュールの関数を使ってサクラエディタのコマンドやマクロ関数を呼び出す事が出来ます。テスト内容
開発時には上記のようなPythonスクリプトのファイルを指定して実行確認を行いました。
PR の影響範囲
マクロ関連
関連 issue, PR
#1327
参考資料
https://docs.python.org/3/extending/embedding.html