[patch] valgrind uninitialized memory in test_terminal3

[Dominique Pellé]

Hi

In runtime/doc/todo.txt, I see this item:

Conditional jump or move depends on uninitialised value(s)
==2819005== at 0x2E9134: jump_to_mouse (mouse.c:2015)
==2819005== by 0x2E69E6: do_mouse (mouse.c:702)
==2819005== by 0x2E95C2: nv_mouse (mouse.c:2166)

If we add the valgrind option --track-origins=yes then we can
see where the uninitialized memory comes from:

==10697== Conditional jump or move depends on uninitialised value(s)
==10697== at 0x2A2225: jump_to_mouse (mouse.c:2015)
==10697== by 0x2A0544: do_mouse (mouse.c:702)
==10697== by 0x2AA3A6: normal_cmd (normal.c:937)
==10697== by 0x225F0C: exec_normal (ex_docmd.c:0)
==10697== by 0x1FFF49: f_feedkeys (evalfunc.c:4352)
==10697== by 0x1FD6EC: call_internal_func (evalfunc.c:2989)
==10697== by 0x3A734C: call_func (userfunc.c:3683)
==10697== by 0x3A684F: get_func_tv (userfunc.c:1839)
==10697== by 0x3AEC52: ex_call_inner (userfunc.c:5577)
==10697== by 0x3AEC52: ex_call (???:5901)
==10697== by 0x21E132: do_one_cmd (ex_docmd.c:2569)
==10697== by 0x21E132: do_cmdline (???:990)
==10697== by 0x3A896F: call_user_func (userfunc.c:2947)
==10697== by 0x3A896F: call_user_func_check (???:3109)
==10697== by 0x3A7273: call_func (userfunc.c:3665)
==10697== by 0x3A684F: get_func_tv (userfunc.c:1839)
==10697== by 0x3AEC52: ex_call_inner (userfunc.c:5577)
==10697== by 0x3AEC52: ex_call (???:5901)
==10697== by 0x21E132: do_one_cmd (ex_docmd.c:2569)
==10697== by 0x21E132: do_cmdline (???:990)
==10697== by 0x1F9878: ex_execute (eval.c:6817)
==10697== by 0x21E132: do_one_cmd (ex_docmd.c:2569)
==10697== by 0x21E132: do_cmdline (???:990)
==10697== by 0x3A896F: call_user_func (userfunc.c:2947)
==10697== by 0x3A896F: call_user_func_check (???:3109)
==10697== by 0x3A7273: call_func (userfunc.c:3665)
==10697== by 0x3A684F: get_func_tv (userfunc.c:1839)
==10697== by 0x3AEC52: ex_call_inner (userfunc.c:5577)
==10697== by 0x3AEC52: ex_call (???:5901)
==10697== by 0x21E132: do_one_cmd (ex_docmd.c:2569)
==10697== by 0x21E132: do_cmdline (???:990)
==10697== by 0x3308ED: do_source_ext (scriptfile.c:1667)
==10697== by 0x32FE55: do_source (scriptfile.c:1811)
==10697== by 0x32FE55: cmd_source (???:1163)
==10697== by 0x21E132: do_one_cmd (ex_docmd.c:2569)
==10697== by 0x21E132: do_cmdline (???:990)
==10697== by 0x441BD8: exe_commands (main.c:3139)
==10697== by 0x441BD8: vim_main2 (???:781)
==10697== by 0x440B9A: main (main.c:432)

==10697== Uninitialised value was created by a heap allocation
==10697== at 0x40367BA: malloc (vg_replace_malloc.c:393)
==10697== by 0x1A5477: lalloc (alloc.c:246)
==10697== by 0x329CC9: screenalloc (screen.c:2711)
==10697== by 0x32AD3D: screenclear (screen.c:2981)
==10697== by 0x3775FB: set_shellsize (term.c:3436)
==10697== by 0x3767EA: set_termname (term.c:2063)
==10697== by 0x4406CC: main (main.c:368)

So the uninitialized memory comes from screen.c:2711:

2711 new_ScreenCols = LALLOC_MULT(colnr_T, (Rows + 1) * Columns);

This one-line patch fixes it:

diff --git a/src/screen.c b/src/screen.c
index 27f484837..17a6fd24e 100644
--- a/src/screen.c
+++ b/src/screen.c
@@ -2708,7 +2708,7 @@ retry:
if (enc_dbcs == DBCS_JPNU)
new_ScreenLines2 = LALLOC_MULT(schar_T, (Rows + 1) * Columns);
new_ScreenAttrs = LALLOC_MULT(sattr_T, (Rows + 1) * Columns);
- new_ScreenCols = LALLOC_MULT(colnr_T, (Rows + 1) * Columns);
+ new_ScreenCols = LALLOC_CLEAR_MULT(colnr_T, (Rows + 1) * Columns);
new_LineOffset = LALLOC_MULT(unsigned, Rows);
new_LineWraps = LALLOC_MULT(char_u, Rows);
new_TabPageIdxs = LALLOC_MULT(short, Columns);

However, I am not sure that the fix is correct as
the function screenalloc(...) has a doclear parameter,
so maybe it's the caller that should have called
screenalloc(TRUE)?

Regards
Dominique

[Bram Moolenaar]

Dominique wrote:

> In runtime/doc/todo.txt, I see this item:
>
> Conditional jump or move depends on uninitialised value(s)
> ==2819005== at 0x2E9134: jump_to_mouse (mouse.c:2015)
> ==2819005== by 0x2E69E6: do_mouse (mouse.c:702)
> ==2819005== by 0x2E95C2: nv_mouse (mouse.c:2166)
>
> If we add the valgrind option --track-origins=yes then we can
> see where the uninitialized memory comes from:
>
> ==10697== Conditional jump or move depends on uninitialised value(s)
> ==10697== at 0x2A2225: jump_to_mouse (mouse.c:2015)
> ==10697== by 0x2A0544: do_mouse (mouse.c:702)
> ==10697== by 0x2AA3A6: normal_cmd (normal.c:937)
> ==10697== by 0x225F0C: exec_normal (ex_docmd.c:0)
> ==10697== by 0x1FFF49: f_feedkeys (evalfunc.c:4352)

[...]

Thanks for looking into this. The warning would be for this line:

if (col_from_screen >= 0)

Which was set here:

// Only use ScreenCols[] after the window was redrawn. Mainly matters
// for tests, a user would not click before redrawing.
// Do not use when 'virtualedit' is active.
if (curwin->w_redr_type <= UPD_VALID_NO_UPDATE && !virtual_active())
col_from_screen = ScreenCols[off];

Apparently the condition doesn't always work. Clearing the whole block
of allocated memory is not cheap, but it won't happen often anyway.

--
I AM THANKFUL...
...for all the complaining I hear about the government
because it means we have freedom of speech.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///