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.

diff --git a/runtime/doc/testing.txt b/runtime/doc/testing.txt
index 1252ce3081d549d9937102085fc5c1343599ded9..6998a6edc28970c4ae1a3dd6bb702be94d1c3e13 100644 (file)
--- a/runtime/doc/testing.txt
+++ b/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

diff --git a/src/change.c b/src/change.c
index ed1f3a302b1d1de0ccde1644b15f1ac34c4db235..2bb6388e5af90b3427c7ac458927b06b8ac70004 100644 (file)
--- a/src/change.c
+++ b/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;
                            }
                        }
                    }

diff --git a/src/cindent.c b/src/cindent.c
index 27e8a7b276295eab49343766422227d248460654..48ddca5320fbde37a47ce0a000589bdb8367de72 100644 (file)
--- a/src/cindent.c
+++ b/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 == ','
-                                             && find_last_paren(l, '{', '}'))
-                       trypos = find_start_brace();
+                   if (trypos == NULL && terminated == ',')
+                   {
+                       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,

diff --git a/src/edit.c b/src/edit.c
index a8e695c91c49e649bafe193f706f3cd7954dac35..2009be1377462258026cac6bd7c32d7e05e895c6 100644 (file)
--- a/src/edit.c
+++ b/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;

diff --git a/src/globals.h b/src/globals.h
index 888f6e95ddab2425305451a7e1eb286b7f839c60..ad563e4e9625e535c15e9ffbec9c2ff75dac4cfd 100644 (file)
--- a/src/globals.h
+++ b/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

diff --git a/src/memline.c b/src/memline.c
index 83aa2c68d5ff3896bfeff4ad36e121f338fa9682..2f73477b5f7fb94f0248601b5540c87ba8bffd71 100644 (file)
--- a/src/memline.c
+++ b/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
-       vim_free(curbuf->b_ml.ml_line_ptr);     // free it
+    if (curbuf->b_ml.ml_flags & (ML_LINE_DIRTY | ML_ALLOCATED))
+       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;
 }
 

diff --git a/src/netbeans.c b/src/netbeans.c
index b69a3cb539c89dedd80a5d7fda8ff44d69a8059a..5c4527948ea30aa834fba51c026abfb5c4aafbc8 100644 (file)
--- a/src/netbeans.c
+++ b/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)
     {

diff --git a/src/normal.c b/src/normal.c
index 4788bc2e818a82f8464aada7764b5a40caae7922..72ebcd5386c0c1ed01e90899fad61e0f725f407e 100644 (file)
--- a/src/normal.c
+++ b/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);
                    }

diff --git a/src/ops.c b/src/ops.c
index b9308789e19eb792b6d5b8da6e5582184386a02d..fc499543fa0ff3402c1e43069998fca87e1ac2dc 100644 (file)
--- a/src/ops.c
+++ b/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);
            }

diff --git a/src/structs.h b/src/structs.h
index 9e409281415a30565456731f94794472f18fd6a8..a537ccb8fdc0c2215b1818bd83edda8ddc065a46 100644 (file)
--- a/src/structs.h
+++ b/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_LINE_DIRTY  2       // cached line was changed and allocated
-#define ML_LOCKED_DIRTY        4       // ml_locked was changed
-#define ML_LOCKED_POS  8       // ml_locked needs positive block number
+#define ML_EMPTY       0x01    // empty buffer
+#define ML_LINE_DIRTY  0x02    // cached line was changed and allocated
+#define ML_LOCKED_DIRTY        0x04    // ml_locked was changed
+#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

diff --git a/src/testdir/runtest.vim b/src/testdir/runtest.vim
index c7d5704e9a01632f6dcd65f6a46ff9aece66a91b..ce60c728e6015f35c9a448c6ba55be730e97f592 100644 (file)
--- a/src/testdir/runtest.vim
+++ b/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

diff --git a/src/testdir/test_breakindent.vim b/src/testdir/test_breakindent.vim
index 8f8f2c4f1b2f6516991f85e5d2792dca72971ce4..6fc4181d65d040df6dd54675486f61990116c8ef 100644 (file)
--- a/src/testdir/test_breakindent.vim
+++ b/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 :(

diff --git a/src/testdir/test_edit.vim b/src/testdir/test_edit.vim
index 4c5c957beba95ec7d9d7a876e4214b1c77c0fc0b..6dc627271394b51811426f11323eb4d9f35f1df9 100644 (file)
--- a/src/testdir/test_edit.vim
+++ b/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))})

diff --git a/src/textprop.c b/src/textprop.c
index 9198665b7cdfdb6e236b9bb5403b8c946b545152..2924192a4d71053dbefeddc96c14c8d1615de84c 100644 (file)
--- a/src/textprop.c
+++ b/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;
     }

diff --git a/src/version.c b/src/version.c
index 116113bbb67f873b92e0584a183bf9e1237c849b..17a25cf01565a0a92930fd9120592d1bd932fbf5 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -735,6 +735,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    13,
 /**/
     12,
 /**/