patch 9.0.0013: reproducing memory access errors can be difficult

Problem:    Reproducing memory access errors can be difficult.
Solution:   When testing, copy each line to allocated memory, so that valgrind
            can detect accessing memory before and/or after it.  Fix uncovered
            problems.

runtime/doc/testing.txt

@@ -268,6 +268,9 @@ test_override({name}, {val})				*test_override()*
 		Current supported values for {name} are:
 
 		{name}	     effect when {val} is non-zero ~
+		alloc_lines  make a copy of every buffer line into allocated
+			     memory, so that memory access errors can be found
+			     by valgrind
 		autoload     `import autoload` will load the script right
 			     away, not postponed until an item is used
 		char_avail   disable the char_avail() function
@@ -287,7 +290,8 @@ test_override({name}, {val})				*test_override()*
 		uptime 	     overrules sysinfo.uptime
 		vterm_title  setting the window title by a job running in a
 			     terminal window
-		ALL	     clear all overrides ({val} is not used)
+		ALL	     clear all overrides, except alloc_lines ({val} is
+			     not used)
 
 		"starting" is to be used when a test should behave like
 		startup was done.  Since the tests are run by sourcing a

src/change.c

@@ -1535,13 +1535,17 @@ open_line(
 			    {
 				// End of C comment, indent should line up
 				// with the line containing the start of
-				// the comment
+				// the comment.
 				curwin->w_cursor.col = (colnr_T)(p - ptr);
 				if ((pos = findmatch(NULL, NUL)) != NULL)
 				{
 				    curwin->w_cursor.lnum = pos->lnum;
 				    newindent = get_indent();
+				    break;
 				}
+				// this may make "ptr" invalid, get it again
+				ptr = ml_get(curwin->w_cursor.lnum);
+				p = ptr + curwin->w_cursor.col;
 			    }
 			}
 		    }

src/cindent.c

@@ -2794,8 +2794,6 @@ get_c_indent(void)
 			    break;
 			}
 
-			l = ml_get_curline();
-
 			// If we're in a comment or raw string now, skip to
 			// the start of it.
 			trypos = ind_find_start_CORS(NULL);
@@ -2806,6 +2804,8 @@ get_c_indent(void)
 			    continue;
 			}
 
+			l = ml_get_curline();
+
 			// Skip preprocessor directives and blank lines.
 			if (cin_ispreproc_cont(&l, &curwin->w_cursor.lnum,
 								    &amount))
@@ -2905,8 +2905,6 @@ get_c_indent(void)
 					      < ourscope - FIND_NAMESPACE_LIM)
 				break;
 
-			    l = ml_get_curline();
-
 			    // If we're in a comment or raw string now, skip
 			    // to the start of it.
 			    trypos = ind_find_start_CORS(NULL);
@@ -2917,6 +2915,8 @@ get_c_indent(void)
 				continue;
 			    }
 
+			    l = ml_get_curline();
+
 			    // Skip preprocessor directives and blank lines.
 			    if (cin_ispreproc_cont(&l, &curwin->w_cursor.lnum,
 								    &amount))
@@ -3196,11 +3196,16 @@ get_c_indent(void)
 				    && trypos->col < tryposBrace->col)))
 			trypos = NULL;
 
+		    l = ml_get_curline();
+
 		    // If we are looking for ',', we also look for matching
 		    // braces.
-		    if (trypos == NULL && terminated == ','
+		    if (trypos == NULL && terminated == ',')
-					      && find_last_paren(l, '{', '}'))
+		    {
-			trypos = find_start_brace();
+			if (find_last_paren(l, '{', '}'))
+			    trypos = find_start_brace();
+			l = ml_get_curline();
+		    }
 
 		    if (trypos != NULL)
 		    {
@@ -3233,6 +3238,7 @@ get_c_indent(void)
 			    --curwin->w_cursor.lnum;
 			    curwin->w_cursor.col = 0;
 			}
+			l = ml_get_curline();
 		    }
 
 		    // Get indent and pointer to text for current line,

src/edit.c

@@ -5013,7 +5013,7 @@ ins_tab(void)
 		    mch_memmove(newp + col, ptr + i,
 					   curbuf->b_ml.ml_line_len - col - i);
 
-		    if (curbuf->b_ml.ml_flags & ML_LINE_DIRTY)
+		    if (curbuf->b_ml.ml_flags & (ML_LINE_DIRTY | ML_ALLOCATED))
 			vim_free(curbuf->b_ml.ml_line_ptr);
 		    curbuf->b_ml.ml_line_ptr = newp;
 		    curbuf->b_ml.ml_line_len -= i;
@@ -5232,10 +5232,10 @@ ins_copychar(linenr_T lnum)
     }
 
     // try to advance to the cursor column
+    validate_virtcol();
     temp = 0;
     line = ptr = ml_get(lnum);
     prev_ptr = ptr;
-    validate_virtcol();
     while ((colnr_T)temp < curwin->w_virtcol && *ptr != NUL)
     {
 	prev_ptr = ptr;

src/globals.h

@@ -1654,6 +1654,7 @@ EXTERN int  reset_term_props_on_termresponse INIT(= FALSE);
 EXTERN int  disable_vterm_title_for_testing INIT(= FALSE);
 EXTERN long override_sysinfo_uptime INIT(= -1);
 EXTERN int  override_autoload INIT(= FALSE);
+EXTERN int  ml_get_alloc_lines INIT(= FALSE);
 
 EXTERN int  in_free_unref_items INIT(= FALSE);
 #endif

src/memline.c

@@ -858,7 +858,8 @@ ml_close(buf_T *buf, int del_file)
     if (buf->b_ml.ml_mfp == NULL)		// not open
 	return;
     mf_close(buf->b_ml.ml_mfp, del_file);	// close the .swp file
-    if (buf->b_ml.ml_line_lnum != 0 && (buf->b_ml.ml_flags & ML_LINE_DIRTY))
+    if (buf->b_ml.ml_line_lnum != 0
+		      && (buf->b_ml.ml_flags & (ML_LINE_DIRTY | ML_ALLOCATED)))
 	vim_free(buf->b_ml.ml_line_ptr);
     vim_free(buf->b_ml.ml_stack);
 #ifdef FEAT_BYTEOFF
@@ -2620,7 +2621,6 @@ ml_get_buf(
 	    --recursive;
 	}
 	ml_flush_line(buf);
-	buf->b_ml.ml_flags &= ~ML_LINE_DIRTY;
 errorret:
 	STRCPY(questions, "???");
 	buf->b_ml.ml_line_len = 4;
@@ -2686,17 +2686,44 @@ errorret:
 	buf->b_ml.ml_line_ptr = (char_u *)dp + start;
 	buf->b_ml.ml_line_len = len;
 	buf->b_ml.ml_line_lnum = lnum;
-	buf->b_ml.ml_flags &= ~ML_LINE_DIRTY;
+	buf->b_ml.ml_flags &= ~(ML_LINE_DIRTY | ML_ALLOCATED);
     }
     if (will_change)
+    {
 	buf->b_ml.ml_flags |= (ML_LOCKED_DIRTY | ML_LOCKED_POS);
+#ifdef FEAT_EVAL
+	if (ml_get_alloc_lines && (buf->b_ml.ml_flags & ML_ALLOCATED))
+	    // can't make the change in the data block
+	    buf->b_ml.ml_flags |= ML_LINE_DIRTY;
+#endif
+    }
 
+#ifdef FEAT_EVAL
+    if (ml_get_alloc_lines
+		 && (buf->b_ml.ml_flags & (ML_LINE_DIRTY | ML_ALLOCATED)) == 0)
+    {
+	char_u *p = alloc(buf->b_ml.ml_line_len);
+
+	// make sure the text is in allocated memory
+	if (p != NULL)
+	{
+	    memmove(p, buf->b_ml.ml_line_ptr, buf->b_ml.ml_line_len);
+	    buf->b_ml.ml_line_ptr = p;
+	    buf->b_ml.ml_flags |= ML_ALLOCATED;
+	    if (will_change)
+		// can't make the change in the data block
+		buf->b_ml.ml_flags |= ML_LINE_DIRTY;
+	}
+    }
+#endif
     return buf->b_ml.ml_line_ptr;
 }
 
 /*
  * Check if a line that was just obtained by a call to ml_get
  * is in allocated memory.
+ * This ignores ML_ALLOCATED to get the same behavior as without the test
+ * override.
  */
     int
 ml_line_alloced(void)
@@ -3409,6 +3436,8 @@ ml_replace(linenr_T lnum, char_u *line, int copy)
  * "len_arg" is the length of the text, excluding NUL.
  * If "has_props" is TRUE then "line_arg" includes the text properties and
  * "len_arg" includes the NUL of the text.
+ * When "copy" is TRUE copy the text into allocated memory, otherwise
+ * "line_arg" must be allocated and will be consumed here.
  */
     int
 ml_replace_len(
@@ -3454,7 +3483,6 @@ ml_replace_len(
     {
 	// another line is buffered, flush it
 	ml_flush_line(curbuf);
-	curbuf->b_ml.ml_flags &= ~ML_LINE_DIRTY;
 
 #ifdef FEAT_PROP_POPUP
 	if (curbuf->b_has_textprop && !has_props)
@@ -3488,8 +3516,8 @@ ml_replace_len(
     }
 #endif
 
-    if (curbuf->b_ml.ml_flags & ML_LINE_DIRTY)	// same line allocated
+    if (curbuf->b_ml.ml_flags & (ML_LINE_DIRTY | ML_ALLOCATED))
-	vim_free(curbuf->b_ml.ml_line_ptr);	// free it
+	vim_free(curbuf->b_ml.ml_line_ptr);	// free allocated line
 
     curbuf->b_ml.ml_line_ptr = line;
     curbuf->b_ml.ml_line_len = len;
@@ -4064,7 +4092,10 @@ ml_flush_line(buf_T *buf)
 
 	entered = FALSE;
     }
+    else if (buf->b_ml.ml_flags & ML_ALLOCATED)
+	vim_free(buf->b_ml.ml_line_ptr);
 
+    buf->b_ml.ml_flags &= ~(ML_LINE_DIRTY | ML_ALLOCATED);
     buf->b_ml.ml_line_lnum = 0;
 }
 

src/netbeans.c

@@ -2741,13 +2741,15 @@ netbeans_inserted(
     if (nbbuf->insertDone)
 	nbbuf->modified = 1;
 
+    // send the "insert" EVT
+    newtxt = alloc(newlen + 1);
+    vim_strncpy(newtxt, txt, newlen);
+
+    // Note: this may make "txt" invalid
     pos.lnum = linenr;
     pos.col = col;
     off = pos2off(bufp, &pos);
 
-    // send the "insert" EVT
-    newtxt = alloc(newlen + 1);
-    vim_strncpy(newtxt, txt, newlen);
     p = nb_quote(newtxt);
     if (p != NULL)
     {

src/normal.c

@@ -5120,6 +5120,8 @@ n_swapchar(cmdarg_T *cap)
 			count = (int)STRLEN(ptr) - pos.col;
 			netbeans_removed(curbuf, pos.lnum, pos.col,
 								 (long)count);
+			// line may have been flushed, get it again
+			ptr = ml_get(pos.lnum);
 			netbeans_inserted(curbuf, pos.lnum, pos.col,
 							&ptr[pos.col], count);
 		    }

src/ops.c

@@ -1273,6 +1273,8 @@ op_tilde(oparg_T *oap)
 
 		netbeans_removed(curbuf, pos.lnum, bd.textcol,
 							    (long)bd.textlen);
+		// get the line again, it may have been flushed
+		ptr = ml_get_buf(curbuf, pos.lnum, FALSE);
 		netbeans_inserted(curbuf, pos.lnum, bd.textcol,
 						&ptr[bd.textcol], bd.textlen);
 	    }
@@ -1322,6 +1324,8 @@ op_tilde(oparg_T *oap)
 		    ptr = ml_get_buf(curbuf, pos.lnum, FALSE);
 		    count = (int)STRLEN(ptr) - pos.col;
 		    netbeans_removed(curbuf, pos.lnum, pos.col, (long)count);
+		    // get the line again, it may have been flushed
+		    ptr = ml_get_buf(curbuf, pos.lnum, FALSE);
 		    netbeans_inserted(curbuf, pos.lnum, pos.col,
 							&ptr[pos.col], count);
 		    pos.col = 0;
@@ -1330,6 +1334,8 @@ op_tilde(oparg_T *oap)
 		ptr = ml_get_buf(curbuf, pos.lnum, FALSE);
 		count = oap->end.col - pos.col + 1;
 		netbeans_removed(curbuf, pos.lnum, pos.col, (long)count);
+		// get the line again, it may have been flushed
+		ptr = ml_get_buf(curbuf, pos.lnum, FALSE);
 		netbeans_inserted(curbuf, pos.lnum, pos.col,
 							&ptr[pos.col], count);
 	    }

src/structs.h

@@ -756,10 +756,11 @@ typedef struct memline
     int		ml_stack_top;	// current top of ml_stack
     int		ml_stack_size;	// total number of entries in ml_stack
 
-#define ML_EMPTY	1	// empty buffer
+#define ML_EMPTY	0x01	// empty buffer
-#define ML_LINE_DIRTY	2	// cached line was changed and allocated
+#define ML_LINE_DIRTY	0x02	// cached line was changed and allocated
-#define ML_LOCKED_DIRTY	4	// ml_locked was changed
+#define ML_LOCKED_DIRTY	0x04	// ml_locked was changed
-#define ML_LOCKED_POS	8	// ml_locked needs positive block number
+#define ML_LOCKED_POS	0x08	// ml_locked needs positive block number
+#define ML_ALLOCATED	0x10	// ml_line_ptr is an allocated copy
     int		ml_flags;
 
     colnr_T	ml_line_len;	// length of the cached line, including NUL

src/testdir/runtest.vim

@@ -154,6 +154,10 @@ endif
 " Prepare for calling test_garbagecollect_now().
 let v:testing = 1
 
+" By default, copy each buffer line into allocated memory, so that valgrind can
+" detect accessing memory before and after it.
+call test_override('alloc_lines', 1)
+
 " Support function: get the alloc ID by name.
 function GetAllocId(name)
   exe 'split ' . s:srcdir . '/alloc.h'
@@ -182,7 +186,7 @@ func RunTheTest(test)
   " mode message.
   set noshowmode
 
-  " Clear any overrides.
+  " Clear any overrides, except "alloc_lines".
   call test_override('ALL', 0)
 
   " Some tests wipe out buffers.  To be consistent, always wipe out all

src/testdir/test_breakindent.vim

@@ -10,7 +10,9 @@ CheckOption breakindent
 source view_util.vim
 source screendump.vim
 
-let s:input ="\tabcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOP"
+func SetUp()
+  let s:input ="\tabcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOP"
+endfunc
 
 func s:screen_lines(lnum, width) abort
   return ScreenLines([a:lnum, a:lnum + 2], a:width)
@@ -714,6 +716,9 @@ func Test_breakindent20_cpo_n_nextpage()
 endfunc
 
 func Test_breakindent20_list()
+  " FIXME - this should not matter
+  call test_override('alloc_lines', 0)
+
   call s:test_windows('setl breakindent breakindentopt= linebreak')
   " default:
   call setline(1, ['  1.  Congress shall make no law',
@@ -830,6 +835,9 @@ func Test_breakindent20_list()
   let lines = s:screen_lines2(1, 6, 20)
   call s:compare_lines(expect, lines)
   call s:close_windows('set breakindent& briopt& linebreak& list& listchars& showbreak&')
+
+  " FIXME - this should not matter
+  call test_override('alloc_lines', 1)
 endfunc
 
 " The following used to crash Vim. This is fixed by 8.2.3391.
@@ -873,15 +881,20 @@ func Test_cursor_position_with_showbreak()
 endfunc
 
 func Test_no_spurious_match()
+  " FIXME - fails under valgrind - this should not matter - timing issue?
+  call test_override('alloc_lines', 0)
+
   let s:input = printf('- y %s y %s', repeat('x', 50), repeat('x', 50))
   call s:test_windows('setl breakindent breakindentopt=list:-1 formatlistpat=^- hls')
   let @/ = '\%>3v[y]'
   redraw!
   call searchcount().total->assert_equal(1)
+
   " cleanup
   set hls&vim
-  let s:input = "\tabcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOP"
   bwipeout!
+  " FIXME - this should not matter
+  call test_override('alloc_lines', 1)
 endfunc
 
 func Test_no_extra_indent()
@@ -945,8 +958,6 @@ func Test_no_extra_indent()
 endfunc
 
 func Test_breakindent_column()
-  " restore original
-  let s:input ="\tabcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOP"
   call s:test_windows('setl breakindent breakindentopt=column:10')
   redraw!
   " 1) default: does not indent, too wide :(

src/testdir/test_edit.vim

@@ -1860,6 +1860,9 @@ func Test_edit_insertmode_ex_edit()
   call writefile(lines, 'Xtest_edit_insertmode_ex_edit')
 
   let buf = RunVimInTerminal('-S Xtest_edit_insertmode_ex_edit', #{rows: 6})
+  " Somehow this can be very slow with valgrind. A separate TermWait() works
+  " better than a longer time with WaitForAssert() (why?)
+  call TermWait(buf, 1000)
   call WaitForAssert({-> assert_match('^-- INSERT --\s*$', term_getline(buf, 6))})
   call term_sendkeys(buf, "\<C-B>\<C-L>")
   call WaitForAssert({-> assert_notmatch('^-- INSERT --\s*$', term_getline(buf, 6))})

src/textprop.c

@@ -287,7 +287,7 @@ prop_add_one(
 					    props + i * sizeof(textprop_T),
 					    sizeof(textprop_T) * (proplen - i));
 
-	if (buf->b_ml.ml_flags & ML_LINE_DIRTY)
+	if (buf->b_ml.ml_flags & (ML_LINE_DIRTY | ML_ALLOCATED))
 	    vim_free(buf->b_ml.ml_line_ptr);
 	buf->b_ml.ml_line_ptr = newtext;
 	buf->b_ml.ml_line_len += sizeof(textprop_T);
@@ -564,7 +564,7 @@ set_text_props(linenr_T lnum, char_u *props, int len)
     mch_memmove(newtext, text, textlen);
     if (len > 0)
 	mch_memmove(newtext + textlen, props, len);
-    if (curbuf->b_ml.ml_flags & ML_LINE_DIRTY)
+    if (curbuf->b_ml.ml_flags & (ML_LINE_DIRTY | ML_ALLOCATED))
 	vim_free(curbuf->b_ml.ml_line_ptr);
     curbuf->b_ml.ml_line_ptr = newtext;
     curbuf->b_ml.ml_line_len = textlen + len;
@@ -698,6 +698,8 @@ f_prop_clear(typval_T *argvars, typval_T *rettv UNUSED)
 		// need to allocate the line now
 		if (newtext == NULL)
 		    return;
+		if (buf->b_ml.ml_flags & ML_ALLOCATED)
+		    vim_free(buf->b_ml.ml_line_ptr);
 		buf->b_ml.ml_line_ptr = newtext;
 		buf->b_ml.ml_flags |= ML_LINE_DIRTY;
 	    }
@@ -1273,6 +1275,8 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)
 			    return;
 			mch_memmove(newptr, buf->b_ml.ml_line_ptr,
 							buf->b_ml.ml_line_len);
+			if (buf->b_ml.ml_flags & ML_ALLOCATED)
+			    vim_free(buf->b_ml.ml_line_ptr);
 			buf->b_ml.ml_line_ptr = newptr;
 			buf->b_ml.ml_flags |= ML_LINE_DIRTY;
 
@@ -1766,8 +1770,13 @@ adjust_prop_columns(
 	colnr_T newlen = (int)textlen + wi * (colnr_T)sizeof(textprop_T);
 
 	if ((curbuf->b_ml.ml_flags & ML_LINE_DIRTY) == 0)
-	    curbuf->b_ml.ml_line_ptr =
+	{
-				 vim_memsave(curbuf->b_ml.ml_line_ptr, newlen);
+	    char_u *p = vim_memsave(curbuf->b_ml.ml_line_ptr, newlen);
+
+	    if (curbuf->b_ml.ml_flags & ML_ALLOCATED)
+		vim_free(curbuf->b_ml.ml_line_ptr);
+	    curbuf->b_ml.ml_line_ptr = p;
+	}
 	curbuf->b_ml.ml_flags |= ML_LINE_DIRTY;
 	curbuf->b_ml.ml_line_len = newlen;
     }

src/version.c

@@ -735,6 +735,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    13,
 /**/
     12,
 /**/