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? 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. -- Common sense is what tells you that the world is flat. /// Bram Moolenaar -- [email protected] -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ download, build and distribute -- http://www.A-A-P.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org /// -- 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
