Bram Moolenaar wrote:
> Dominique Pelle wrote:
>
>> 3 remarks about vim/src/undo.c (at changeset: 271a5907f944):
>>
>> (1) Using persistent-undo, I notice once in a while the following memory
>> leak, but I have not found the way to reproduce it all the time:
>>
>> ==3371== 10 bytes in 2 blocks are definitely lost in loss record 23 of 116
>> ==3371== at 0x4024F70: malloc (vg_replace_malloc.c:236)
>> ==3371== by 0x8114F98: lalloc (misc2.c:919)
>> ==3371== by 0x81BCC06: u_read_undo (undo.c:983)
>> ==3371== by 0x80C5359: readfile (fileio.c:2591)
>> ==3371== by 0x8053976: open_buffer (buffer.c:132)
>> ==3371== by 0x8098ABE: do_ecmd (ex_cmds.c:3658)
>> ==3371== by 0x80AEF7B: do_exedit (ex_docmd.c:7620)
>> ==3371== by 0x80AEC38: ex_edit (ex_docmd.c:7516)
>> ==3371== by 0x80A782C: do_one_cmd (ex_docmd.c:2639)
>> ==3371== by 0x80A5105: do_cmdline (ex_docmd.c:1108)
>> ==3371== by 0x812AD39: nv_colon (normal.c:5226)
>> ==3371== by 0x81245C3: normal_cmd (normal.c:1188)
>> ==3371== by 0x80E7938: main_loop (main.c:1216)
>> ==3371== by 0x80E742F: main (main.c:960)
>>
>> It's this alloc in undo.c:
>>
>> 983 array = (char_u **)U_ALLOC_LINE(
>> 984 (unsigned)(sizeof(char_u *) *
>> uep->ue_size));
>>
>> Looking at the undo.c code, memory seems to be properly freed
>> but I might be missing something since Valgrind does not generally
>> give spurious leak messages.
>
> The logic is not easy to follow. Also, there are hardly any checks for
> the undo file to contain invalid information. I think it's good to
> assume that every value read from the file might be wrong.
>
> I think we need to add more checks that what is read back from the undo
> file is correct. E.g., when "num_head" is too small the code would
> gladly store pointers beyond the end of uhp_table[].
>
>
>> (2) In vim/src/undo.c I see at line 92:
>>
>> 91 /* See below: use malloc()/free() for memory management. */
>> 92 #define U_USE_MALLOC 1
>>
>> Whether U_USE_MALLOC is defined or not selects different
>> implementations of U_FREE_LINE and U_ALLOC_LINE.
>>
>> Is the implementation when U_USE_MALLOC is undefined
>> still meant to be used? Or can it be removed?
>>
>> I'm asking because if I comment out the line #define U_USE_MALLOC 1
>> at line undo.c:92, then Vim quickly crashes when using persistent-undo.
>> In other words, persistent undo only works when U_USE_MALLOC is defined.
>
> Can you see why it crashes? Perhaps it allocates a block that is too
> big?
If I just recompile Vim with "#define U_USE_MALLOC 1" commented out
(without any patch) as follows:
/* See below: use malloc()/free() for memory management. */
/*#define U_USE_MALLOC 1*/
Then the following command is enough to make Vim crash:
$ vim -u NONE --noplugin -c 'wundo! foo' -c 'rundo foo'
Vim: Caught deadly signal SEGV
Vim: Finished.
Segmentation fault
Program received signal SIGSEGV, Segmentation fault.
0x081bf48c in u_free_line (ptr=0x8264f90 "", keep=0) at undo.c:2846
2846 if ((nextb = curbuf->b_mb_current->mb_next) != NULL
(gdb) bt
#0 0x081bf48c in u_free_line (ptr=0x8264f90 "", keep=0) at undo.c:2846
#1 0x081bc5f0 in u_read_undo (name=0x8264e06 "foo",
hash=0xbfffed9c
"\343\260\304B\230\374\034\024\232\373\364\310\231o\271$'\256A\344d\233\223L\244\225\231\033xR\270U")
at undo.c:1094
#2 0x080afc34 in ex_rundo (eap=0xbfffedfc) at ex_docmd.c:8481
#3 0x080a70dd in do_one_cmd (cmdlinep=0xbfffefb0, sourcing=1,
cstack=0xbfffefb8, fgetline=0,
cookie=0x0) at ex_docmd.c:2639
#4 0x080a49b6 in do_cmdline (cmdline=0xbffff6a3 "rundo foo",
getline=0, cookie=0x0, flags=11)
at ex_docmd.c:1108
#5 0x080a4070 in do_cmdline_cmd (cmd=0xbffff6a3 "rundo foo") at ex_docmd.c:714
#6 0x080e935d in exe_commands (parmp=0xbffff364) at main.c:2750
#7 0x080e6b3a in main (argc=8, argv=0xbffff4e4) at main.c:880
(gdb) list
2841 if (curbuf->b_mb_current == NULL || mp < (minfo_T
*)curbuf->b_mb_current)
2842 {
2843 curbuf->b_mb_current = curbuf->b_block_head.mb_next;
2844 curbuf->b_m_search = NULL;
2845 }
2846 if ((nextb = curbuf->b_mb_current->mb_next) != NULL
2847 && (minfo_T
*)nextb < mp)
2848 {
2849 curbuf->b_mb_current = nextb;
2850 curbuf->b_m_search = NULL;
(gdb) p curbuf
$1 = (buf_T *) 0x8234dd0
(gdb) p curbuf->b_mb_current
$2 = (mblock_T *) 0x0
So accessing curbuf->b_mb_current->mb_next dereferences NULL.
I have not had time to look at how u_alloc_line() and u_free_line()
work. Using vim_malloc() and vim_free() is certainly simpler. malloc()
and free() are also quite optimized. So I don't see the need
for a special implementation.
> I don't think that anybody compiles Vim with U_USE_MALLOC undefined.
> It's probably safe to remove this old code now.
>
>> (3) When U_USE_MALLOC is defined (default behavior), I see that 1 byte
>> is added to every allocations at line 117:
>>
>> 115 #ifdef U_USE_MALLOC
>> 116 # define U_FREE_LINE(ptr) vim_free(ptr)
>> 117 # define U_ALLOC_LINE(size) lalloc((long_u)((size) + 1), FALSE)
>> 118 #else
>>
>> This extra 1 byte is odd since it's not needed everywhere U_ALLOC_LINE(...)
>> is used. I think it's better if the caller is responsible for adding +1
>> when needed to avoid wasting memory. Attached patch does that.
>>
>> Maybe this patch also fixes the leak (1) but I'm not sure since I don't
>> know how to reproduce it all the time. At least, I have not seen the
>> leak so far after applying the attached patch.
>
> I don't think it fixes the leak. It does make it a little bit more
> efficient.
>
> Please test the code with this patch for a while. I'll include it
> later.
I'll keep using the patch. So far no problem to report with it.
I'll have more time during the weekend to stress test persistent undo
with & without the patch. It would be nice to find a way to reproduce
the leak.
-- 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
