Loading ...
Sorry, an error occurred while loading the content.

Patch to fix access to freed memory in spelling checker feature

Expand Messages
  • Dominique Pelle
    Hi Valgrind memory checker detects that Vim is using freed memory in the spelling checker code: ==8692== Invalid read of size 1 ==8692== at 0x8199D87:
    Message 1 of 4 , Feb 3, 2008
    • 0 Attachment
      Hi

      Valgrind memory checker detects that Vim is using freed memory
      in the spelling checker code:

      ==8692== Invalid read of size 1
      ==8692== at 0x8199D87: spell_to_word_end (spell.c:15854)
      ==8692== by 0x8166424: win_line (screen.c:3054)
      ==8692== by 0x8163AD5: win_update (screen.c:1765)
      ==8692== by 0x8161C34: update_screen (screen.c:522)
      ==8692== by 0x8064F1B: ins_redraw (edit.c:1474)
      ==8692== by 0x8063B96: edit (edit.c:688)
      ==8692== by 0x812322A: normal_cmd (normal.c:1327)
      ==8692== by 0x80E5D2D: main_loop (main.c:1181)
      ==8692== by 0x80E587D: main (main.c:940)
      ==8692== Address 0x50615F0 is 40 bytes inside a block of size 96 free'd
      ==8692== at 0x402237F: free (vg_replace_malloc.c:233)
      ==8692== by 0x8113F55: vim_free (misc2.c:1580)
      ==8692== by 0x80F864A: ml_flush_line (memline.c:3149)
      ==8692== by 0x80F69FE: ml_get_buf (memline.c:2111)
      ==8692== by 0x80F68BB: ml_get (memline.c:2034)
      ==8692== by 0x818D567: check_need_cap (spell.c:10271)
      ==8692== by 0x817C828: spell_move_to (spell.c:2098)
      ==8692== by 0x81663D0: win_line (screen.c:3048)
      ==8692== by 0x8163AD5: win_update (screen.c:1765)
      ==8692== by 0x8161C34: update_screen (screen.c:522)
      ==8692== by 0x8064F1B: ins_redraw (edit.c:1474)
      ==8692== by 0x8063B96: edit (edit.c:688)
      ==8692== by 0x812322A: normal_cmd (normal.c:1327)
      ==8692== by 0x80E5D2D: main_loop (main.c:1181)
      ==8692== by 0x80E587D: main (main.c:940)

      (and then follows several more errors)

      Here is the relevant code in screen.c:

      2927 line = ml_get_buf(wp->w_buffer, lnum, FALSE);
      2928 ptr = line;
      ....
      ....
      3037 #ifdef FEAT_SPELL
      3038 /* When spell checking a word we need to figure out the
      start of the
      3039 * word and if it's badly spelled or not. */
      3040 if (has_spell)
      3041 {
      3042 int len;
      3043 hlf_T spell_hlf = HLF_COUNT;
      3044
      3045 pos = wp->w_cursor;
      3046 wp->w_cursor.lnum = lnum;
      3047 wp->w_cursor.col = (colnr_T)(ptr - line);
      !!3048 len = spell_move_to(wp, FORWARD, TRUE, TRUE, &spell_hlf);
      3049 if (len == 0 || (int)wp->w_cursor.col > ptr - line)
      3050 {
      3051 /* no bad word found at line start, don't check
      until end of a
      3052 * word */
      3053 spell_hlf = HLF_COUNT;
      !!3054 word_end = (int)(spell_to_word_end(ptr,
      wp->w_buffer) - line + 1);
      3055 }

      Error happens inside call of spell_to_word_end() at screen.c:3054
      because ptr is dereferenced inside spell_to_word_end() but is pointing
      to memory which has already been freed (bug!).

      ptr was previously freed just a few lines above, as a side effect
      of calling spell_move_to(...) 6 lines above at screen.c:3048, because
      spell_move_to(...) may in some cases call ml_get(...) which invalidates
      previous pointer returned by previous call of ml_get(...). So call
      to spell_move_to invalidates line and ptr which were initialized at
      line 2927/2928 in screen.c.

      Here is how I can reproduce the bug:

      $ valgrind vim -u NONE -c 'set nowrap|set spell|start' -s
      spell-access-freed-mem.vim 2> valgrind.log

      spell-testcase is a small file generated randomly which triggers this bug:
      http://dominique.pelle.free.fr/spell-access-freed-mem.vim

      But happens only if terminal is small enough (80x25 or smaller)

      I attach a patch which fixes the bug but please review it. Maybe there
      is a better solution.

      I'm using vim-7.1 (Patches 1-242) on Linux x86 in a gnome-terminal, built
      with "configure --with-feature=huge", without optimizations (-O0 -g).

      -- Dominique

      --~--~---------~--~----~------------~-------~--~----~
      You received this message from the "vim_dev" maillist.
      For more information, visit http://www.vim.org/maillist.php
      -~----------~----~----~----~------~----~------~--~---
    • Bram Moolenaar
      ... Hmm, ml_get() shouldn t free the pointer, it should be getting the same line. ... This requires more investigation. I ll do that later. -- Often you re
      Message 2 of 4 , Feb 6, 2008
      • 0 Attachment
        Dominique Pelle wrote:

        > Valgrind memory checker detects that Vim is using freed memory
        > in the spelling checker code:
        >
        > ==8692== Invalid read of size 1
        > ==8692== at 0x8199D87: spell_to_word_end (spell.c:15854)
        > ==8692== by 0x8166424: win_line (screen.c:3054)
        > ==8692== by 0x8163AD5: win_update (screen.c:1765)
        > ==8692== by 0x8161C34: update_screen (screen.c:522)
        > ==8692== by 0x8064F1B: ins_redraw (edit.c:1474)
        > ==8692== by 0x8063B96: edit (edit.c:688)
        > ==8692== by 0x812322A: normal_cmd (normal.c:1327)
        > ==8692== by 0x80E5D2D: main_loop (main.c:1181)
        > ==8692== by 0x80E587D: main (main.c:940)
        > ==8692== Address 0x50615F0 is 40 bytes inside a block of size 96 free'd
        > ==8692== at 0x402237F: free (vg_replace_malloc.c:233)
        > ==8692== by 0x8113F55: vim_free (misc2.c:1580)
        > ==8692== by 0x80F864A: ml_flush_line (memline.c:3149)
        > ==8692== by 0x80F69FE: ml_get_buf (memline.c:2111)
        > ==8692== by 0x80F68BB: ml_get (memline.c:2034)
        > ==8692== by 0x818D567: check_need_cap (spell.c:10271)
        > ==8692== by 0x817C828: spell_move_to (spell.c:2098)
        > ==8692== by 0x81663D0: win_line (screen.c:3048)
        > ==8692== by 0x8163AD5: win_update (screen.c:1765)
        > ==8692== by 0x8161C34: update_screen (screen.c:522)
        > ==8692== by 0x8064F1B: ins_redraw (edit.c:1474)
        > ==8692== by 0x8063B96: edit (edit.c:688)
        > ==8692== by 0x812322A: normal_cmd (normal.c:1327)
        > ==8692== by 0x80E5D2D: main_loop (main.c:1181)
        > ==8692== by 0x80E587D: main (main.c:940)
        >
        > (and then follows several more errors)
        >
        > Here is the relevant code in screen.c:
        >
        > 2927 line = ml_get_buf(wp->w_buffer, lnum, FALSE);
        > 2928 ptr = line;
        > ....
        > ....
        > 3037 #ifdef FEAT_SPELL
        > 3038 /* When spell checking a word we need to figure out the
        > start of the
        > 3039 * word and if it's badly spelled or not. */
        > 3040 if (has_spell)
        > 3041 {
        > 3042 int len;
        > 3043 hlf_T spell_hlf = HLF_COUNT;
        > 3044
        > 3045 pos = wp->w_cursor;
        > 3046 wp->w_cursor.lnum = lnum;
        > 3047 wp->w_cursor.col = (colnr_T)(ptr - line);
        > !!3048 len = spell_move_to(wp, FORWARD, TRUE, TRUE, &spell_hlf);
        > 3049 if (len == 0 || (int)wp->w_cursor.col > ptr - line)
        > 3050 {
        > 3051 /* no bad word found at line start, don't check
        > until end of a
        > 3052 * word */
        > 3053 spell_hlf = HLF_COUNT;
        > !!3054 word_end = (int)(spell_to_word_end(ptr,
        > wp->w_buffer) - line + 1);
        > 3055 }
        >
        > Error happens inside call of spell_to_word_end() at screen.c:3054
        > because ptr is dereferenced inside spell_to_word_end() but is pointing
        > to memory which has already been freed (bug!).
        >
        > ptr was previously freed just a few lines above, as a side effect
        > of calling spell_move_to(...) 6 lines above at screen.c:3048, because
        > spell_move_to(...) may in some cases call ml_get(...) which invalidates
        > previous pointer returned by previous call of ml_get(...). So call
        > to spell_move_to invalidates line and ptr which were initialized at
        > line 2927/2928 in screen.c.

        Hmm, ml_get() shouldn't free the pointer, it should be getting the same
        line.

        > Here is how I can reproduce the bug:
        >
        > $ valgrind vim -u NONE -c 'set nowrap|set spell|start' -s
        > spell-access-freed-mem.vim 2> valgrind.log
        >
        > spell-testcase is a small file generated randomly which triggers this bug:
        > http://dominique.pelle.free.fr/spell-access-freed-mem.vim
        >
        > But happens only if terminal is small enough (80x25 or smaller)
        >
        > I attach a patch which fixes the bug but please review it. Maybe there
        > is a better solution.
        >
        > I'm using vim-7.1 (Patches 1-242) on Linux x86 in a gnome-terminal, built
        > with "configure --with-feature=huge", without optimizations (-O0 -g).

        This requires more investigation. I'll do that later.

        --
        Often you're less important than your furniture. If you think about it, you
        can get fired but your furniture stays behind, gainfully employed at the
        company that didn't need _you_ anymore.
        (Scott Adams - The Dilbert principle)

        /// Bram Moolenaar -- Bram@... -- 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.
        For more information, visit http://www.vim.org/maillist.php
        -~----------~----~----~----~------~----~------~--~---
      • Dominique Pelle
        ... Looking at the code, call to spell_move_to(...) may invalidate the line pointer, since it may call ml_get() with the previous line when it calls
        Message 3 of 4 , Feb 9, 2008
        • 0 Attachment
          On 2/6/08, Bram Moolenaar <Bram@...> wrote:
          >
          > Dominique Pelle wrote:
          >
          > > Valgrind memory checker detects that Vim is using freed memory
          > > in the spelling checker code:
          ...

          > Hmm, ml_get() shouldn't free the pointer, it should be getting the same
          > line.


          Looking at the code, call to spell_move_to(...) may invalidate
          the line pointer, since it may call ml_get() with the previous line
          when it calls check_need_cap(...) at line spell.c:10271:

          10247 check_need_cap(lnum, col)
          10248 linenr_T lnum;
          10249 colnr_T col;
          10250 {
          ....
          10263 if ((int)(skipwhite(line) - line) >= (int)col)
          10264 {
          10265 /* At start of line, check if previous line is empty
          or sentence
          10266 * ends there. */
          10267 if (lnum == 1)
          10268 need_cap = TRUE;
          10269 else
          10270 {
          !!!10271 line = ml_get(lnum - 1);
          10272 if (*skipwhite(line) == NUL)
          10273 need_cap = TRUE;


          I also found a much simpler test case to trigger this bug:

          1/ start vim with:
          valgrind vim -u NONE -c 'set nowrap|set spell' 2> valgrind.log

          2/ press i (to enter insert mode)

          3/ press <NL> (to go to the second line)

          4/ type: aaaaaaaa.... (etc) until reaching slightly
          beyond the full width of the terminal.

          5/ Observe that valgrind complains (access to freed memory)
          shortly after typing enough 'a' to reach beyond the width of
          the terminal.

          6/ Also observe that text aaaa... (etc) get highlighted in red
          (as spelling mistake) as soon as reaching the full width of the
          terminal. Normally vim does not highlight a word a spelling
          error until a word separator has been typed (space for
          example) but somehow reaching the right side of the terminal
          when in "nowrap" mode causes Vim to start highlighting the
          word as spelling error. I'm not sure whether it's the symptom
          of the same bug or if it's another bug.

          I'm using Vim-7.1.244 on Linux x86 built with
          "configure --with-features=huge" without optimizations (-O0 -g)
          in a gnome-terminal.

          -- Dominique

          --~--~---------~--~----~------------~-------~--~----~
          You received this message from the "vim_dev" maillist.
          For more information, visit http://www.vim.org/maillist.php
          -~----------~----~----~----~------~----~------~--~---
        • Bram Moolenaar
          ... Thanks for the extra info. I ll look into it later. -- hundred-and-one symptoms of being an internet addict: 10. And even your night dreams are in HTML.
          Message 4 of 4 , Feb 9, 2008
          • 0 Attachment
            Dominique Pelle wrote:

            > > > Valgrind memory checker detects that Vim is using freed memory
            > > > in the spelling checker code:
            > ...
            >
            > > Hmm, ml_get() shouldn't free the pointer, it should be getting the same
            > > line.
            >
            >
            > Looking at the code, call to spell_move_to(...) may invalidate
            > the line pointer, since it may call ml_get() with the previous line
            > when it calls check_need_cap(...) at line spell.c:10271:
            >
            > 10247 check_need_cap(lnum, col)
            > 10248 linenr_T lnum;
            > 10249 colnr_T col;
            > 10250 {
            > ....
            > 10263 if ((int)(skipwhite(line) - line) >= (int)col)
            > 10264 {
            > 10265 /* At start of line, check if previous line is empty
            > or sentence
            > 10266 * ends there. */
            > 10267 if (lnum == 1)
            > 10268 need_cap = TRUE;
            > 10269 else
            > 10270 {
            > !!!10271 line = ml_get(lnum - 1);
            > 10272 if (*skipwhite(line) == NUL)
            > 10273 need_cap = TRUE;
            >
            >
            > I also found a much simpler test case to trigger this bug:
            >
            > 1/ start vim with:
            > valgrind vim -u NONE -c 'set nowrap|set spell' 2> valgrind.log
            >
            > 2/ press i (to enter insert mode)
            >
            > 3/ press <NL> (to go to the second line)
            >
            > 4/ type: aaaaaaaa.... (etc) until reaching slightly
            > beyond the full width of the terminal.
            >
            > 5/ Observe that valgrind complains (access to freed memory)
            > shortly after typing enough 'a' to reach beyond the width of
            > the terminal.
            >
            > 6/ Also observe that text aaaa... (etc) get highlighted in red
            > (as spelling mistake) as soon as reaching the full width of the
            > terminal. Normally vim does not highlight a word a spelling
            > error until a word separator has been typed (space for
            > example) but somehow reaching the right side of the terminal
            > when in "nowrap" mode causes Vim to start highlighting the
            > word as spelling error. I'm not sure whether it's the symptom
            > of the same bug or if it's another bug.
            >
            > I'm using Vim-7.1.244 on Linux x86 built with
            > "configure --with-features=huge" without optimizations (-O0 -g)
            > in a gnome-terminal.

            Thanks for the extra info. I'll look into it later.

            --
            hundred-and-one symptoms of being an internet addict:
            10. And even your night dreams are in HTML.

            /// Bram Moolenaar -- Bram@... -- 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.
            For more information, visit http://www.vim.org/maillist.php
            -~----------~----~----~----~------~----~------~--~---
          Your message has been successfully submitted and would be delivered to recipients shortly.