Dominique Pellé <[email protected]> wrote:
> Christian Brabandt wrote:
>
>> Bram,
>> I tend to abort long running z= commands by pressing <Ctrl-C>. This
>> randomly crashed Vim for me, but until now, I couldn't reliably
>> reproduce it, possibly it is timing dependent or influenced by some
>> setting, I don't know.
>>
>> However, I could just get a stacktrace as well as some valgrind logfiles
>> (see attached tar.xz file). Both logfiles are not very helpful. It seems
>> like Vim is allocating a lot of memory and finally gets killed. The
>> problem lies in the the add_sound_suggest() function and it seems, wlen
>> runs over. When gdb caught SEGV, it was at some value of above 21000.
>>
>> Here is a patch, that makes sure, wlen doesn't increment over MAXWLEN
>> and fixes it for me. So please include it.
>>
>>
>> diff --git a/src/spell.c b/src/spell.c
>> --- a/src/spell.c
>> +++ b/src/spell.c
>> @@ -13382,6 +13382,10 @@
>> wordcount = 0;
>> for (;;)
>> {
>> + if (wlen > MAXWLEN)
>> + /* safety check */
>> + break;
>> +
>> i = 1;
>> if (wordcount == orgnr && byts[n + 1] == NUL)
>> break; /* found end of word */
>
>
> Trying to press CTRL-C right after z= as you describe, it
> did not take me long to get a crash as well using vim-7.4.92
> on Linux x86_84. This is the error I got with valgrind, which
> is different than yours:
>
> ==3900== Memcheck, a memory error detector
> ==3900== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
> ==3900== Using Valgrind-3.9.0.SVN and LibVEX; rerun with -h for copyright info
> ==3900== Command: ./vim spell.c
> ==3900== Parent PID: 3839
> ==3900==
> ==3900== Invalid read of size 1
> ==3900== at 0x56CE37: spell_edit_score (spell.c:15072)
> ==3900== by 0x571913: suggest_trie_walk (spell.c:13504)
> ==3900== by 0x568365: spell_find_suggest (spell.c:13277)
> ==3900== by 0x566F3E: spell_suggest (spell.c:10245)
> ==3900== by 0x4DDD8D: nv_zet (normal.c:5272)
> ==3900== by 0x4D4460: normal_cmd (normal.c:1200)
> ==3900== by 0x5C1872: main_loop (main.c:1329)
> ==3900== by 0x5C0DF1: main (main.c:1020)
> ==3900== Address 0xf00000001 is not stack'd, malloc'd or (recently) free'd
> ==3900==
>
> It happened with spelllang=fr, when pressing z= on the
> word x (only one letter x).
>
> It does not happen all the time, but it's easy to reproduce
> after trying several times, always with the same stack trace
> as above and with the same invalid address 0xf00000001
>
> Code where it crashes:
>
> 15048 static int
> 15049 spell_edit_score(slang, badword, goodword)
> .....
> 15069 for (p = badword, badlen = 0; *p != NUL; )
> 15070 wbadword[badlen++] = mb_cptr2char_adv(&p);
>
> It must be p which is wrong (i.e. parameter badword).
>
>
> Trying also with clang-3.4 address sanitizer and undefined
> sanitizer (clang -fsanituze=address,undefined) shows a
> stack buffer overflow (such stack overflows can't be detected
> by valgrind but are detected by the address sanitizer):
>
> ==4531==ERROR: AddressSanitizer: stack-buffer-overflow on address
> 0x7fff0f0404ba at pc 0x1a05fd3 bp 0x7fff0f03f970 sp 0x7fff0f03f968
> WRITE of size 1 at 0x7fff0f0404ba thread T0
> #0 0x1a05fd2 in add_sound_suggest /home/pel/sb/vim/src/spell.c:13429
> #1 0x19c7ab7 in suggest_trie_walk /home/pel/sb/vim/src/spell.c:11729
> #2 0x19b6121 in suggest_try_soundalike /home/pel/sb/vim/src/spell.c:13277
> #3 0x198b7c5 in spell_suggest_intern /home/pel/sb/vim/src/spell.c:10876
> #4 0x1947750 in spell_find_suggest /home/pel/sb/vim/src/spell.c:10711
> #5 0x193d3b8 in spell_suggest /home/pel/sb/vim/src/spell.c:10245
> #6 0x127a543 in nv_zet /home/pel/sb/vim/src/normal.c:5272
> #7 0x119fb5b in normal_cmd /home/pel/sb/vim/src/normal.c:1200
> #8 0x1fcbb64 in main_loop /home/pel/sb/vim/src/main.c:1329
> #9 0x1faa79c in main /home/pel/sb/vim/src/main.c:1020
> #10 0x7fba1dd10de4 in __libc_start_main
> /build/buildd/eglibc-2.17/csu/libc-start.c:260
> #11 0x4a880c in _start (/home/pel/sb/vim/src/vim+0x4a880c)
>
> Address 0x7fff0f0404ba is located in stack of thread T0 at offset 794 in frame
> #0 0x1a0273f in add_sound_suggest /home/pel/sb/vim/src/spell.c:13328
>
> This frame has 26 object(s):
> [32, 40) 'su.addr'
> [96, 104) 'goodword.addr'
> [160, 164) 'score.addr'
> [224, 232) 'lp.addr'
> [288, 296) 'slang'
> [352, 356) 'sfwordnr'
> [416, 424) 'nrline'
> [480, 484) 'orgnr'
> [544, 794) 'theword' <== Memory access at offset 794 overflows this
> variable
> [832, 836) 'i'
> [896, 900) 'wlen'
> [960, 968) 'byts'
> [1024, 1032) 'idxs'
> [1088, 1092) 'n'
> [1152, 1156) 'wordcount'
> [1216, 1220) 'wc'
> [1280, 1284) 'goodscore'
> [1344, 1352) 'hash'
> [1408, 1416) 'hi'
> [1472, 1480) 'sft'
> [1536, 1540) 'bc'
> [1600, 1604) 'gc'
> [1664, 1668) 'limit'
> [1728, 1978) 'cword'
> [2016, 2024) 'p'
> [2080, 2084) 'flags'
> HINT: this may be a false positive if your program uses some custom
> stack unwind mechanism or swapcontext
> (longjmp and C++ exceptions *are* supported)
> SUMMARY: AddressSanitizer: stack-buffer-overflow
> /home/pel/sb/vim/src/spell.c:13429 add_sound_suggest
> Shadow bytes around the buggy address:
> 0x100061e00040: 00 f4 f4 f4 f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2
> 0x100061e00050: 00 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2
> 0x100061e00060: 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2
> 0x100061e00070: 04 f4 f4 f4 f2 f2 f2 f2 00 00 00 00 00 00 00 00
> 0x100061e00080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> =>0x100061e00090: 00 00 00 00 00 00 00[02]f2 f2 f2 f2 04 f4 f4 f4
> 0x100061e000a0: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4
> 0x100061e000b0: f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2 04 f4 f4 f4
> 0x100061e000c0: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 04 f4 f4 f4
> 0x100061e000d0: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4
> 0x100061e000e0: f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4
> Shadow byte legend (one shadow byte represents 8 application bytes):
> Addressable: 00
> Partially addressable: 01 02 03 04 05 06 07
> Heap left redzone: fa
> Heap right redzone: fb
> Freed heap region: fd
> Stack left redzone: f1
> Stack mid redzone: f2
> Stack right redzone: f3
> Stack partial redzone: f4
> Stack after return: f5
> Stack use after scope: f8
> Global redzone: f9
> Global init order: f6
> Poisoned by user: f7
> ASan internal: fe
> ==4531==ABORTING
>
> 13322 static void
> 13323 add_sound_suggest(su, goodword, score, lp)
> 13324 suginfo_T *su;
> 13325 char_u *goodword;
> 13326 int score; /* soundfold score */
> 13327 langp_T *lp;
> 13328 {
> 13329 slang_T *slang = lp->lp_slang; /* language for sound folding */
> 13330 int sfwordnr;
> 13331 char_u *nrline;
> 13332 int orgnr;
> 13333 char_u theword[MAXWLEN];
> 13334 int i;
> 13335 int wlen;
> 13336 char_u *byts;
> 13337 idx_T *idxs;
> 13338 int n;
> 13339 int wordcount;
> 13340 int wc;
> 13341 int goodscore;
> 13342 hash_T hash;
> 13343 hashitem_T *hi;
> 13344 sftword_T *sft;
> 13345 int bc, gc;
> 13346 int limit;
> ......
> 13429 theword[wlen++] = byts[n + i];
> 13430 n = idxs[n + i];
>
> So it looks theword[wlen++] writes more than 250 bytes
> in a 250 byte array in the stack.
>
> But in your patch, I think that the line "if (wlen > MAXWLEN)"
> should really be ""if (wlen >= MAXWLEN - 1)". That seems to
> fix it:
>
> $ hg diff spell.c
> diff -r 0c37f66b4f3b src/spell.c
> --- a/src/spell.c Thu Nov 14 05:48:46 2013 +0100
> +++ b/src/spell.c Mon Nov 18 22:28:25 2013 +0100
> @@ -13402,6 +13402,10 @@
> wordcount = 0;
> for (;;)
> {
> + if (wlen >= MAXWLEN - 1)
> + /* safety check */
> + break;
> +
> i = 1;
> if (wordcount == orgnr && byts[n + 1] == NUL)
> break; /* found end of word */
>
>
> The question remains: what is wlen so large when
> pressing CTRL-C? I do not know yet.
Although the above patch fixes it in practice for me,
I now see that the safety should actually be
even stricter, since in the for (...) loop, there is
this statement at spell.c:13416:
STRCPY(theword + wlen, "BAD");
... which writes 4 bytes "BAD\0" from &theword[wlen]
if executed.
But it it's odd that it writes "BAD\0" becase then at line 13416
because it gets immediately overwritten with a \0
after the loop at line 13433, so it becomes "\0AD\0".
That does not make sense to me:
13403 for (;;)
13404 {
13405 i = 1;
13406 if (wordcount == orgnr && byts[n + 1] == NUL)
13407 break; /* found end of word */
13408
13409 if (byts[n + 1] == NUL)
13410 ++wordcount;
13411
13412 /* skip over the NUL bytes */
13413 for ( ; byts[n + i] == NUL; ++i)
13414 if (i > byts[n]) /* safety check */
13415 {
13416 STRCPY(theword + wlen, "BAD");
13417 goto badword;
13418 }
13419
13420 /* One of the siblings must have the word. */
13421 for ( ; i < byts[n]; ++i)
13422 {
13423 wc = idxs[idxs[n + i]]; /* nr of words under this byte */
13424 if (wordcount + wc > orgnr)
13425 break;
13426 wordcount += wc;
13427 }
13428
13429 theword[wlen++] = byts[n + i];
13430 n = idxs[n + i];
13431 }
13432 badword:
13433 theword[wlen] = NUL;
Perhaps a better patch is something like this,
but I admit that I don't understand this piece code:
$ hg diff spell.c
diff -r 0c37f66b4f3b src/spell.c
--- a/src/spell.c Thu Nov 14 05:48:46 2013 +0100
+++ b/src/spell.c Mon Nov 18 22:55:02 2013 +0100
@@ -13402,6 +13402,10 @@
wordcount = 0;
for (;;)
{
+ if (wlen > MAXWLEN - 4)
+ /* safety check */
+ break;
+
i = 1;
if (wordcount == orgnr && byts[n + 1] == NUL)
break; /* found end of word */
@@ -13414,6 +13418,7 @@
if (i > byts[n]) /* safety check */
{
STRCPY(theword + wlen, "BAD");
+ wlen += 3;
goto badword;
}
Regards
Dominique
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
