Vim7.3.973でsearch("[^\x01-\x7e]", 'n')でCaught deadly signal ABRT #386

Closed
itchyny opened this Issue May 19, 2013 · 60 comments

Comments

Projects
None yet
7 participants

itchyny commented May 19, 2013

OS: Mac OS X
Vim 7.3 1-973 --with-features=huge --enable-multibyte
Vim 7.3.1-967ではこの問題は生じない

再現方法

 $ src/vim -u NONE --noplugin -c "echo search(\"[^\\x01-\\x7e]\", 'n')"

(このsearchは, http://www.kawaz.jp/pukiwiki/?vim#cb691f26 のものです)

エラー内容

Vim: Caught deadly signal ABRT
Vim: Finished.
zsh: abort      src/vim -u NONE --noplugin -c "echo search(\"[^\\x01-\\x7e]\", 'n')"

期待する挙動: エラー無しに0と表示, 落ちない.

 $ src/vim -u NONE --noplugin -c "echo search(\"[\\x01-\\x7e]\", 'n')"

(^抜き)だと, SEGVになります.

Owner

koron commented May 19, 2013

原因は 970 ですねわかりますw

Owner

koron commented May 20, 2013

970 は阿鼻叫喚な感じやね。

Owner

thinca commented May 20, 2013

本家の方でも落ちる報告は何件か来ているようですが、具体的な落ちるコードを示している人はまだ居ない?

Owner

koron commented May 20, 2013

パッチ書いた人はいるみたいw

Owner

thinca commented May 20, 2013

お、ほんとだ。

Owner

mattn commented May 20, 2013

あきらかになんかのバッファが足りてないね。

:echo search("[^\x48-\x7e]", 'n')

だとokで

:echo search("[^\x47-\x7e]", 'n')

だとng

Owner

thinca commented May 20, 2013

これ、テストも足した方が良さそうな気がしますね。

Owner

mattn commented May 20, 2013

NFA_STACK_SIZE の値が少ないっぽい。

Owner

mattn commented May 20, 2013

ところで

$ vim regexp.c

しただけで落ちますね。かわいいですね。

Owner

mattn commented May 20, 2013

レンジでありあえるパターンを生成しているようなのだけど、そのスタックが足りないっぽい。
(まだ未確定)
とりあえずこれで落ちなくはなったが。

Owner

mattn commented May 20, 2013

$ vim arabic.c でも落ちるね。

Owner

mattn commented May 20, 2013

なんか根本的にアタリ方を間違ってる気がしてる。

Member

Shougo commented May 20, 2013

これではうかつにVimをアップデートできないですね……。

Owner

mattn commented May 20, 2013

むー。

で arabic.c も落ちなくはなったけど、なんか根本マズってるのかな。

itchyny commented May 20, 2013

:echo search("[^A-z]")

のほうが報告しやすい

Owner

thinca commented May 20, 2013

全然試せてないんですが、現状 vim_dev に出ているパッチはこの件とは別件ってことですかね?
だとしたらこっちはこっちで報告した方が良いかもしれませんね。

Owner

koron commented May 20, 2013

いまこんなことを提案するか迷い中

7.3 は stable と認識されている。
970 はかなり unstable だから revert すべき。
7.3からは revert した上で 7.4a (alpha or beta) を unstable でリリースして
その上で 730 を brush up しよう。

どうだろう?

Owner

koron commented May 20, 2013

とりあえず一報だけはしておきました。

https://groups.google.com/d/msg/vim_dev/niZ0QTP1ucw/AG0yxPkueWYJ

@itchyny Thanks! その例、使わせてもらいました。

Member

Shougo commented May 20, 2013

7.3 は stable と認識されている。
970 はかなり unstable だから revert すべき。
7.3からは revert した上で 7.4a (alpha or beta) を unstable でリリースして
その上で 730 を brush up しよう。

はい。その提案に賛成です。テスト不足なので、730は一度revertするべきでしょうね……。

Owner

h-east commented May 20, 2013

暫定対策
.vimrc に set re=1 と書けばNFA正規表現エンジンを使わなくなるので落ちなくなります。

Owner

k-takata commented May 20, 2013

いまこんなことを提案するか迷い中

提案するに +1

Owner

koron commented May 20, 2013

set re=1 と書けばNFA正規表現エンジンを使わなくなる

こっちをデフォにするのでも良いかもね。

Owner

thinca commented May 20, 2013

提案に +1

こっちをデフォにするのでも良いかもね。

設定変えただけでクラッシュする恐れがあるのはさすがにつらいです。

Owner

k-takata commented May 20, 2013

2152行目辺り、lallocしたサイズとは無関係にstack_endを設定しているのが気になります。
lallocのサイズに、NFA_STACK_SIZE 足せばいいのかな?

    /* Allocate space for the stack. Max states on the stack : nstate */
    stack = (Frag_T *) lalloc((nstate + 1)*sizeof(Frag_T), TRUE);
    stackp = stack;
    stack_end = stack + NFA_STACK_SIZE;

後は、PUSH(), st_push() がスタックがいっぱいになってもエラーを返さないのが気になります。

NFA_SPLIT = -1024 は、NFA_STACK_SIZE とは関係なさそう。普通の文字と区別できるように、NFA_* な enum がすべて負の値になっていればよいのではないかと予想。

Owner

mattn commented May 20, 2013

後は、PUSH(), st_push() がスタックがいっぱいになってもエラーを返さないのが気になります。

そこが問題みたいです。ただ、通常あるケースなので「それってどうなの」と思ってます。

Owner

koron commented May 20, 2013

エラー検出して、エラーがあったら古い正規表現に切り替えるとかできないかしら?

Owner

mattn commented May 20, 2013

valgrantd してもむずいなー。

Owner

mattn commented May 20, 2013

そもそも正規表現が前と同じ動きをしてない。

Owner

thinca commented May 20, 2013

そもそも互換性がないんですね…。だとするとなおさら revert して貰わないと、既存のスクリプトに大きな影響が出ますね。

Owner

mattn commented May 20, 2013

新しいエンジンだと \= が動いてないっぽい。

Owner

thinca commented May 20, 2013

実装漏れですかね。

Owner

mattn commented May 20, 2013

twitvim で URL のハイライトが970未満と以上だと異なりますね。

Member

Shougo commented May 20, 2013

Vim 7.4でも入るか怪しくなって来ましたね……。Vim本体に正規表現のテストってあるのでしょうか?
それに通るかどうか確かめる or 正規表現のテストを追加するというのはどうでしょう。

Owner

mattn commented May 20, 2013

diff -r 0917206e7317 src/regexp_nfa.c
--- a/src/regexp_nfa.c  Sun May 19 22:31:18 2013 +0200
+++ b/src/regexp_nfa.c  Mon May 20 16:45:04 2013 +0900
@@ -15,8 +15,6 @@
 #define        NFA_BRACES_MAXLIMIT         10
 /* For allocating space for the postfix representation */
 #define        NFA_POSTFIX_MULTIPLIER      (NFA_BRACES_MAXLIMIT + 2)*2
-/* Size of stack, used when converting the postfix regexp into NFA */
-#define        NFA_STACK_SIZE          1024

 enum
 {
@@ -238,7 +236,7 @@
    return FAIL;
     vim_memset(post_start, 0, postfix_size);
     post_ptr = post_start;
-    post_end = post_start + postfix_size;
+    post_end = post_start + nstate_max;
     nfa_has_zend = FALSE;

     regcomp_start(expr, re_flags);
@@ -2152,7 +2150,7 @@
    /* Allocate space for the stack. Max states on the stack : nstate */
    stack = (Frag_T *) lalloc((nstate + 1)*sizeof(Frag_T), TRUE);
    stackp = stack;
-   stack_end = stack + NFA_STACK_SIZE;
+   stack_end = stack + (nstate + 1);
     }

     for (p = postfix; p < end; ++p)

とりあえず落ちないようにはなりました。

Owner

mattn commented May 20, 2013

でもこのレベルの間違い(構造体ポインタに + する値がオフセットじゃなくてバイト数になってた)が入ってるとしたら、結構恐ろしいコードかもw

Owner

mattn commented May 20, 2013

ひとまず誰か試して貰えませんか?
これとtwitvimのハイライトがおかしいのは別な気がするので、こっちからやっつけたい。

Owner

mattn commented May 20, 2013

webapi#json もこのバグのおかげでおかしくなって post できねぇw

Owner

mattn commented May 20, 2013

影響でかすぎるなこのバグ

Owner

mattn commented May 20, 2013

[[:cntrl:]] の動きが変わった様だ。

Owner

mattn commented May 20, 2013

マルチバイト文字が [[:cntrl:]] になってるwwwwwwww

Owner

k-takata commented May 20, 2013

とりあえず落ちないようにはなりました。

stack_end のサイズが一致していないのは、上に書いたとおり気付いていましたが、post_end は気付きませんでした。
(手元だと、stack_end の修正だけで落ちないようになっていたので。)

マルチバイト文字が [[:cntrl:]] になってるwwwwwwww

ひぃ!

Owner

koron commented May 20, 2013

ちょっと new regexp engine のいけてないところ一覧にできませんか?
それを突きつけて revert かデフォルトオフかの選択を迫るべきでしょう。

Owner

k-takata commented May 20, 2013

Vim本体に正規表現のテストってあるのでしょうか?

test64 が正規表現のテストです。今回のパッチで一緒に test64 も強化されていますし、さらにマルチバイトの正規表現のテストとして test95 が追加されています。
ただ、それでもテストが全然足りないので、syntaxファイル作者に対してサンプルをたくさんくれーと言っているのが現在の状況です。

Owner

k-takata commented May 20, 2013

test64にマルチバイトのテストが1件残ってるなあ。test95に移動が必要。
さらにtest95は enc=cp932 などだとテストが通らない。

Owner

mattn commented May 20, 2013

あー、webapi#json が動かないのは \uXXXX を使ってるからか。だいぶ困る。

Member

Shougo commented May 20, 2013

test64 が正規表現のテストです。今回のパッチで一緒に test64 も強化されていますし、さらにマルチバイトの正規表現のテストとして test95 が追加されています。
ただ、それでもテストが全然足りないので、syntaxファイル作者に対してサンプルをたくさんくれーと言っているのが現在の状況です。

ああ、そういうことなんですね。理解しました。neocomplcache等でも正規表現は多用しているので、かなり影響が出そうです。

Owner

mattn commented May 20, 2013

ちょっと new regexp engine のいけてないところ一覧にできませんか?
それを突きつけて revert かデフォルトオフかの選択を迫るべきでしょう。

  • マルチバイト文字が [[:cntrl:]]
  • 落ちる(暫定パッチあり)
  • 幾らかのプラグインで誤動作が見られる
Owner

k-takata commented May 20, 2013

[[:alpha:]] とかの判定もおかしいですね。
check_char_class() の中で、マルチバイト文字もお構いなしに isalpha() などに放り込んでる。

Owner

koron commented May 20, 2013

@mattn Bram が 975 で修正したっぽいが… なんかコレだとダメっぽくない?
https://groups.google.com/d/msg/vim_dev/G5z61R4XcXA/QksltkyRKyUJ

Owner

k-takata commented May 20, 2013

post_endの方だけ修正されててstack_endが直ってないですね。落ちました。

Owner

k-takata commented May 20, 2013

regexp_nfa.c 1094行目のこれは何だろう。

            if (*regparse == 'n' || *regparse == 'n')
Owner

koron commented May 20, 2013

post_endの方だけ修正されててstack_endが直ってないですね。落ちました。

落ちるパターン、教えてください。
975 のスレッドに直接投げてくれても良いです。

regexp_nfa.c 1094行目のこれは何だろう。

www

Owner

k-takata commented May 20, 2013

:echo search("[^A-z]") で落ちます。(Win7)

Owner

k-takata commented May 20, 2013

あれ?間違ったかも。

Owner

k-takata commented May 20, 2013

がーん、古いVimが起動しっぱなしでビルドがエラーになってた。落ちなくなりました。
でもやっぱり stack_end も何らかの修正がいるのではないかなあ。

Owner

koron commented May 20, 2013

:echo search("[^A-z]") で落ちます。

試してみたけど落ちなかった (´・ω・`) debug.log にこんなん入ってたから、NFAに回ってない。

NFA engine could not handle "[^A-z]"

itchyny commented May 20, 2013

私の手元(Mac OS X)では, 976まで来てこのissueの最初のsearch()は落ちなくなりました.

以下全部落ちないです.

 $ src/vim -u NONE --noplugin -c "echo search(\"[^\\x01-\\x7e]\", 'n')"
 $ src/vim -u NONE --noplugin -c "echo search(\"[\\x01-\\x7e]\", 'n')"
:echo search("[^A-z]")

しかし, mattnさんのご指摘によると, まだ既存の挙動と異なる部分があるようです.
このissueはどうしますか?

  • タイトルを変えて, ここで継続する
  • 一旦閉じて, new regex engine関連の新しいissueを立てる. 「regexp_nfa.cの問題点と改善スレッド」
Owner

koron commented May 20, 2013

ええい、もうなるようになれ… (´・ω・`)

Owner

koron commented May 20, 2013

976まで来てこのissueの最初のsearch()は落ちなくなりました.

なのでこちらは閉じます。

一旦閉じて, new regex engine関連の新しいissueを立てる.

#390 で続きをやります。

koron closed this May 20, 2013

itchyny commented May 20, 2013

ありがとうございました

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