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

vimLoadLibの修正について検討する #253

Closed
k-takata opened this Issue Sep 15, 2012 · 16 comments

Comments

Projects
None yet
3 participants
@k-takata
Member

k-takata commented Sep 15, 2012

https://groups.google.com/d/topic/vim_dev/1g5QM5Cy-mA/discussion で、encの設定によっては、iconvをロードしようとした際に、vimLoadLibの中で呼んでいる関数がiconvを使おうとするために、スタックオーバーフローで落ちるという話が出ています。
そこに添付されているパッチを見ると、[GS]etCurrentDirectory() を使うように変更しているのですが、まず [GS]etCurrentDirectoryW() を試すべきではないかと思うのですが、どうでしょうか。

diff -r 68fd19007be0 src/os_win32.c
--- a/src/os_win32.c    Tue Sep 11 16:48:42 2012 +0200
+++ b/src/os_win32.c    Sun Sep 16 00:37:03 2012 +0900
@@ -288,18 +288,37 @@
 vimLoadLib(char *name)
 {
     HINSTANCE dll = NULL;
-    char old_dir[MAXPATHL];
+    char old_dir[_MAX_PATH];

     if (exe_path == NULL)
    get_exe_name();
-    if (exe_path != NULL && mch_dirname(old_dir, MAXPATHL) == OK)
+    if (exe_path != NULL)
     {
-   /* Change directory to where the executable is, both to make sure we
-    * find a .dll there and to avoid looking for a .dll in the current
-    * directory. */
-   mch_chdir(exe_path);
-   dll = LoadLibrary(name);
-   mch_chdir(old_dir);
+#ifdef FEAT_MBYTE
+   WCHAR old_dirw[_MAX_PATH];
+
+   if (GetCurrentDirectoryW(_MAX_PATH, old_dirw) != 0)
+   {
+       /* Change directory to where the executable is, both to make
+        * sure we find a .dll there and to avoid looking for a .dll
+        * in the current directory. */
+       SetCurrentDirectory(exe_path);
+       dll = LoadLibrary(name);
+       SetCurrentDirectoryW(old_dirw);
+       return dll;
+   }
+   /* Retry with non-wide function (for Windows 98). */
+   if (GetLastError() == ERROR_CALL_NOT_IMPLEMENTED)
+#endif
+       if (GetCurrentDirectory(_MAX_PATH, old_dir) != 0)
+       {
+       /* Change directory to where the executable is, both to make
+        * sure we find a .dll there and to avoid looking for a .dll
+        * in the current directory. */
+       SetCurrentDirectory(exe_path);
+       dll = LoadLibrary(name);
+       SetCurrentDirectory(old_dir);
+       }
     }
     return dll;
 }

カレントディレクトリを exe_path に設定するところで、SetCurrentDirectoryW() を使っていないところがちょっと気になるところですが、get_exe_name() がW系APIを使っていないので、これはこうするしかないかなと思います。

あるいは、SetDllDirectory() を動的にロードして、それが使えるならカレントディレクトリを一時的に変更するのではなく、それを使った方がいいのではという気もしますが。

@koron

This comment has been minimized.

Member

koron commented Sep 15, 2012

まずポイントは以下の関数をWindowsネイティブのものに置き換えなければならないということ

  • mch_dirname
  • mch_chdir

そうしないとvimLoadLib("iconv.dll")な無限再起呼び出しができてしまう。

これを踏まえて元のパッチの本当の問題点は exe_pathの中身の文字コードを考慮せずにSetCurrentDirectoryに渡している という点に集約される。ただこれはexe_pathがTCHARで正しく実装されていれば問題ではない。単にUNICODEモードでコンパイルすればよいだけ。

@k-takata さん版は…上記の問題を解消していない。W系を使うということにメリットはない。Win98をサポートする意味は無いし、仮にサポートするとしてもW系が存在しない(はずだったよね?)から起動できない可能性がある。あと後半はA系を使わないと、UNICODEモードでコンパイルしたときは後半部分がまったく無意味になってしまう。

@k-takata: なぜWでやる必要があると考えたのか、その理由を教えてください。

@koron koron closed this Sep 15, 2012

@koron koron reopened this Sep 15, 2012

@ynkdir

This comment has been minimized.

Member

ynkdir commented Sep 15, 2012

:cd /path/to/カレントコードページが持ってないユニコード文字/
:call libcall('a.dll', 'b', 'c')

こういう場合にWでないと元のディレクトリに復帰できないですね。

先にvim_devでコメントしてしまいましたがなんか余計なことしてすいません。

@koron

This comment has been minimized.

Member

koron commented Sep 16, 2012

いいんじゃないですかw

ただあのコード自体、Win2K SP4以降であれば要らないんですよね。DLLの読み込み優先順位変わってますから。

@k-takata

This comment has been minimized.

Member

k-takata commented Sep 16, 2012

@ynkdir いえ、お構いなくw このパッチの意図はその通りです。

単にUNICODEモードでコンパイルすればよいだけ。

現状の os_win32.c はそういう作りにはなっていないです。FEAT_MBYTEで切っている部分を見ると、A系APIで処理すべき部分にAはついていません。

exe_pathの文字コードは現在のコードページのもののようです。get_exec_name() の中で、GetModuleFileName() を使って取得した後、文字コードの変換はしていませんので。
少なくとも実行ファイルがUnicodeパス上に無ければちゃんと動くはずなので、Unicodeパス上に置いた場合も考慮すべきか?という話になります。(ちゃんと考慮する場合、直すべき場所はここだけではない気がしますが。)

仮にサポートするとしてもW系が存在しない(はずだったよね?)

Win9xも、W系のシンボルだけは用意されています。中身はエラーを返すだけになっていますが。

ただあのコード自体、Win2K SP4以降であれば要らないんですよね。DLLの読み込み優先順位変わってますから。

指定されたDLLがたまたまカレントディレクトリにはあって、それ以外のDLL読み込みパスには無い場合には、カレントディレクトリのものがロードされてしまいますので良くないと思います。

@k-takata

This comment has been minimized.

Member

k-takata commented Sep 16, 2012

@koron

This comment has been minimized.

Member

koron commented Sep 16, 2012

ただあのコード自体、Win2K SP4以降であれば要らないんですよね。DLLの読み込み優先順位変わってますから。

指定されたDLLがたまたまカレントディレクトリにはあって、それ以外のDLL読み込みパスには無い場合には、カレントディレクトリのものがロードされてしまいますので良くないと思います。

それはまた別の仕組みで防ぐべきでは? カレントに正しいDLLがある状況で読み込めないのも逆に困りますから。

@koron

This comment has been minimized.

Member

koron commented Sep 16, 2012

少なくとも実行ファイルがUnicodeパス上に無ければちゃんと動くはずなので、Unicodeパス上に置いた場合も考慮すべきか?という話になります。(ちゃんと考慮する場合、直すべき場所はここだけではない気がしますが。)

それですよね。やるなら全部やらないと意味を成さない。このケースで言えばexe自体が現在のCPで表せない場所に置いてあった場合にはそもそも動かないんで、やるだけ無駄じゃないですか?

@koron

This comment has been minimized.

Member

koron commented Sep 16, 2012

こういうのってvimに限らないですけど、将来へ引き継ぐべきではない負の遺産が、どんどん蓄積していきますよね…

@koron

This comment has been minimized.

Member

koron commented Sep 16, 2012

今Vimのソース眺めてて思ったんだけど mch_chdir を SetCurrentDirectory で置き換えるのって大丈夫なのかしら? mch_chdir は crt の関数をいくつか叩いている。

とりあえずすぐに思いつく怪しいケースは、exeのある場所とカレントディレクトリが異なるドライブだった場合、くらい。逆にこのケースが動くなら、当面の問題はないと判断して良さげ。

@koron

This comment has been minimized.

Member

koron commented Sep 16, 2012

mch_chdir を SetCurrentDirectory で置き換えるのって大丈夫なのかしら?

論理的には、今回のケースでは問題無いと推定される。WIN32API側の操作がCRT側に副作用を及ぼすケースは見たことがない。逆に作用がCRT側に伝播しなくて困るケースはよく知られている(環境変数など)。パッチの内容は、WIN32API上の作用を利用後に即消している。つまりWIN32APIによる作用を残すケースではない。

@k-takata

This comment has been minimized.

Member

k-takata commented Sep 16, 2012

とりあえずすぐに思いつく怪しいケースは、exeのある場所とカレントディレクトリが異なるドライブだった場合

Windowsはドライブごとにカレントディレクトリを持っているので、exeのあるドライブのカレントディレクトリがexeのあるディレクトリを指すようになってしまいますが、それはパッチを当てる前のコードでも同じですし、恐らく問題はないのではないかと。

mch_chdir では _chdrive を呼んでドライブを変更していますが、VC の _chdrive のソースを見ると、SetCurrentDirectory を呼んでいるだけだったりしますし。

@koron

This comment has been minimized.

Member

koron commented Sep 16, 2012

純粋な興味から、確認だけ。

Windowsはドライブごとにカレントディレクトリを持っているので、exeのあるドライブのカレントディレクトリがexeのあるディレクトリを指すように

LoadLibrary が読み込む対象としている カレントディレクトリ って、正確には カレントドライブのカレントディレクトリ ってことですよね? 非カレントドライブのカレントディレクトリからも探すぜ、みたいな鬼畜仕様でないことを切に願うw

@k-takata

This comment has been minimized.

Member

k-takata commented Sep 16, 2012

さすがにそんな鬼畜仕様ではなかったと思いますwww

@k-takata

This comment has been minimized.

Member

k-takata commented Sep 21, 2012

todo.txtに入りました。

Win32: patch for current directory, "loading iof conv". (Ken Takata, 2012 Sep
15)

@k-takata k-takata closed this Sep 21, 2012

@k-takata

This comment has been minimized.

Member

k-takata commented Dec 26, 2012

7.3.707で取り込まれていますので、ラベルをfixedに変更しました。
https://groups.google.com/d/topic/vim_dev/bTJP7zVGBVI/discussion

@koron

This comment has been minimized.

Member

koron commented Dec 26, 2012

👍 @k-takata

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment