[patch] fixed access to freed memory when redirecting :reg to register
Dominique Pellé
I can reproduce a bug (use of freed memory) in Vim-7.2.284
on Linux x86 as follows:
1/ Start vim with valgrind:
$ cd vim7/src
$ valgrind --leak-check=yes \
--num-callers=50 ./vim --noplugin -u NONE 2> vg.log
2/ Enter the 2 following Ex commands:
:redir @"
:reg
3/ Observe in vg.log the following errors as soon as :reg is being
executed:
==13408== Invalid read of size 1
==13408== at 0x40276F8: memmove (mc_replace_strmem.c:517)
==13408== by 0x813CDA1: str_to_reg (ops.c:6157)
==13408== by 0x813CB6F: write_reg_contents_ex (ops.c:6052)
==13408== by 0x813C9E1: write_reg_contents (ops.c:5981)
==13408== by 0x8104C0E: redir_write (message.c:3046)
==13408== by 0x8103034: msg_puts_attr_len (message.c:1803)
==13408== by 0x8102715: msg_outtrans_len_attr (message.c:1402)
==13408== by 0x810243D: msg_outtrans_len (message.c:1291)
==13408== by 0x81398A3: ex_display (ops.c:4013)
==13408== by 0x80A7548: do_one_cmd (ex_docmd.c:2627)
==13408== by 0x80A4D7F: do_cmdline (ex_docmd.c:1096)
==13408== by 0x8090CB8: call_user_func (eval.c:21292)
==13408== by 0x807CE17: call_func (eval.c:8123)
==13408== by 0x807CA5B: get_func_tv (eval.c:7969)
==13408== by 0x8078DF3: eval7 (eval.c:5021)
==13408== by 0x80786FC: eval6 (eval.c:4688)
==13408== by 0x80782E8: eval5 (eval.c:4504)
==13408== by 0x8077839: eval4 (eval.c:4199)
==13408== by 0x8077691: eval3 (eval.c:4111)
==13408== by 0x807751D: eval2 (eval.c:4040)
==13408== by 0x807734D: eval1 (eval.c:3965)
==13408== by 0x80772B4: eval0 (eval.c:3922)
==13408== by 0x8073AC5: ex_let (eval.c:1837)
==13408== by 0x80A7548: do_one_cmd (ex_docmd.c:2627)
==13408== by 0x80A4D7F: do_cmdline (ex_docmd.c:1096)
==13408== by 0x80A2FE3: do_source (ex_cmds2.c:3116)
==13408== by 0x80EA44D: source_startup_scripts (main.c:2778)
==13408== by 0x80E74DB: main (main.c:563)
==13408== Address 0x548108c is 4 bytes inside a block of size 32 free'd
==13408== at 0x4024E5A: free (vg_replace_malloc.c:323)
==13408== by 0x8116C67: vim_free (misc2.c:1639)
==13408== by 0x813CD7A: str_to_reg (ops.c:6155)
==13408== by 0x813CB6F: write_reg_contents_ex (ops.c:6052)
==13408== by 0x813C9E1: write_reg_contents (ops.c:5981)
==13408== by 0x8104C0E: redir_write (message.c:3046)
==13408== by 0x8103034: msg_puts_attr_len (message.c:1803)
==13408== by 0x8102715: msg_outtrans_len_attr (message.c:1402)
==13408== by 0x810243D: msg_outtrans_len (message.c:1291)
==13408== by 0x81398A3: ex_display (ops.c:4013)
==13408== by 0x80A7548: do_one_cmd (ex_docmd.c:2627)
==13408== by 0x80A4D7F: do_cmdline (ex_docmd.c:1096)
==13408== by 0x8090CB8: call_user_func (eval.c:21292)
==13408== by 0x807CE17: call_func (eval.c:8123)
==13408== by 0x807CA5B: get_func_tv (eval.c:7969)
==13408== by 0x8078DF3: eval7 (eval.c:5021)
==13408== by 0x80786FC: eval6 (eval.c:4688)
==13408== by 0x80782E8: eval5 (eval.c:4504)
==13408== by 0x8077839: eval4 (eval.c:4199)
==13408== by 0x8077691: eval3 (eval.c:4111)
==13408== by 0x807751D: eval2 (eval.c:4040)
==13408== by 0x807734D: eval1 (eval.c:3965)
==13408== by 0x80772B4: eval0 (eval.c:3922)
==13408== by 0x8073AC5: ex_let (eval.c:1837)
==13408== by 0x80A7548: do_one_cmd (ex_docmd.c:2627)
==13408== by 0x80A4D7F: do_cmdline (ex_docmd.c:1096)
==13408== by 0x80A2FE3: do_source (ex_cmds2.c:3116)
==13408== by 0x80EA44D: source_startup_scripts (main.c:2778)
==13408== by 0x80E74DB: main (main.c:563)
(several more errors follow after that)
The bug happens because function ex_display() is printing
all registers and while doing so, a register can be modified
if output is redirected to register (causing access to freed
memory).
Attached patch fixes it by making function ex_display()
output a copy of the register. Please review it.
I noticed this issue when trying Tony's .vimrc available at:
http://vim.wikia.com/wiki/User:Tonymec/vimrc
The bug happens in function TestForX() in his vimrc file.
Cheers
-- Dominique
Bram Moolenaar
Dominique Pelle wrote:
Thanks!
--
No children may attend school with their breath smelling of "wild onions."
[real standing law in West Virginia, United States of America]
/// Bram Moolenaar -- Br...@Moolenaar.net -- 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 ///
Bram Moolenaar
Dominique Pelle wrote:
The patch introduces quite a bit of new code, copies text around and
allocates memory.
How about this solution instead:
*** ../vim-7.2.289/src/ops.c 2009-11-03 16:44:04.000000000 +0100
--- src/ops.c 2009-11-11 16:13:33.000000000 +0100
***************
*** 3990,3995 ****
--- 3990,4001 ----
}
else
yb = &(y_regs[i]);
+
+ if (name == vim_tolower(redir_reg)
+ || (redir_reg == '"' && yb == y_previous))
+ continue; /* do not list register being written to, the
+ * pointer can be freed */
+
if (yb->y_array != NULL)
{
msg_putchar('\n');
--
Mynd you, m00se bites Kan be pretty nasti ...
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
Dominique Pellé
Your patch is simpler (which is better) and I verified that it
also avoids access to freed memory.
My patch and your patch have different outcomes though.
Using the following script...
:let @a = "foobar"
:redir @a
:sil reg a
:redir END
:echo @a
With your patch, the echo statements prints one line:
--- Registers ---
With my patch, the echo statement prints 2 lines:
--- Registers ---
"a ^J--- Registers ---
I don't mind about the difference. In fact not outputting
the redirected register (as in your patch) is better in my
opinion since content of register @a has already been
lost as soon as we do ":redir @a".
Cheers
-- Dominique
Bram Moolenaar
Dominique Pelle wrote:
Thanks for checking the patch. Yes, the register written to is omitted
from the list. I think that's somewhat better than listing a partly
filled register. Definitely better than crashing.
--
SOLDIER: What? A swallow carrying a coconut?
ARTHUR: It could grip it by the husk ...
Dominique Pellé
Hi
I just realize that there is a problem with your proposed patch:
vim-tiny fails to compile.
ops.c: In function ‘ex_display’:
ops.c:3995: error: ‘redir_reg’ undeclared (first use in this function)
We need to add #ifdef FEAT_EVAL
RCS file: /cvsroot/vim/vim7/src/ops.c,v
retrieving revision 1.77
diff -c -r1.77 ops.c
*** src/ops.c 11 Nov 2009 16:22:28 -0000 1.77
--- src/ops.c 14 Nov 2009 19:51:22 -0000
***************
*** 3991,3996 ****
--- 3991,4004 ----
}
else
yb = &(y_regs[i]);
+
+ #ifdef FEAT_EVAL
+ if (name == vim_tolower(redir_reg)
+ || (redir_reg == '"' && yb == y_previous))
+ continue; /* do not list register being written to, the
+ * pointer can be freed */
+ #endif
Bram Moolenaar
Dominique Pelle wrote:
Yes. I found after running my regular tests. Sorry I didn't mention
this.
--
Bad programs can be written in any language.