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

Re: (patch) fix for out of bound memory access when indenting function

Expand Messages
  • Bram Moolenaar
    ... Thanks for locating this problem. I think it s best to always make sure the cursor position is valid. So every time curwin- w_cursor.lnum is changed the
    Message 1 of 2 , Feb 24, 2008
    • 0 Attachment
      Dominique Pelle wrote:

      > Valgrind memory checker detects out of bounds memory
      > access in Vim-7.1.262:
      >
      > ==6247== Invalid read of size 1
      > ==6247== at 0x811EF81: utf_head_off (mbyte.c:2486)
      > ==6247== by 0x8174860: findmatchlimit (search.c:2009)
      > ==6247== by 0x810D6C8: find_start_brace (misc1.c:5868)
      > ==6247== by 0x810FD4F: get_c_indent (misc1.c:7364)
      > ==6247== by 0x8131010: op_reindent (ops.c:684)
      > ==6247== by 0x8124556: do_pending_operator (normal.c:1943)
      > ==6247== by 0x8122F29: normal_cmd (normal.c:1178)
      > ==6247== by 0x80E5DAD: main_loop (main.c:1181)
      > ==6247== by 0x80E58FD: main (main.c:940)
      > ==6247== Address 0x586C105 is 5 bytes after a block of size 4,096 alloc'd
      > ==6247== at 0x4022765: malloc (vg_replace_malloc.c:149)
      > ==6247== by 0x81134D0: lalloc (misc2.c:862)
      > ==6247== by 0x81133F2: alloc (misc2.c:761)
      > ==6247== by 0x80F3175: mf_alloc_bhdr (memfile.c:977)
      > ==6247== by 0x80F2780: mf_new (memfile.c:399)
      > ==6247== by 0x80F8735: ml_new_data (memline.c:3167)
      > ==6247== by 0x80F7121: ml_append_int (memline.c:2405)
      > ==6247== by 0x80F86DF: ml_flush_line (memline.c:3144)
      > ==6247== by 0x80F6AB8: ml_get_buf (memline.c:2111)
      > ==6247== by 0x80F69D1: ml_get_curline (memline.c:2053)
      > ==6247== by 0x8130FED: op_reindent (ops.c:680)
      > ==6247== by 0x8124556: do_pending_operator (normal.c:1943)
      > ==6247== by 0x8122F29: normal_cmd (normal.c:1178)
      > ==6247== by 0x80E5DAD: main_loop (main.c:1181)
      > ==6247== by 0x80E58FD: main (main.c:940)
      > (more errors follow)
      >
      > Steps to reproduce:
      >
      > 1/ use a 1 line ~/.vimrc file with
      >
      > set expandtab
      >
      > 2/ run vim with valgrind and open vim's source file spell.c:
      >
      > $ valgrind ./vim vim7/src/spell.c 2> valgrind.log
      >
      > 3/ visual select entire function suggest_trie_walk()
      >
      > 4/ press '=' command to indent/format selected function.
      >
      > 6/ Observe that valgrind reports above errors
      >
      > Here is the code in search.c:
      >
      > 2007 #ifdef FEAT_MBYTE
      > 2008 if (has_mbyte)
      > !!!2009 pos.col -= (*mb_head_off)(linep, linep + pos.col);
      > 2010 #endif
      >
      > If add a debug printf() before call to (*mb_head_off)(...)
      > at search.c:2009, I see:
      >
      > pos.col=75 STRLEN(linep)=69 linep=[
      > else if (sp->ts_isdiff == DIFF_INSERT]
      >
      > Notice that pos.col=75 is well beyond the length of the line (69)
      > so utf_head_off(...) will access byte beyond the current line.
      >
      > If you don't have valgrind, you can put an assert to verify:
      > assert(pos.col <= STRLEN(linep) (assert will fail).
      >
      > Function findmatchlimit() is using both curwin->w_cursor.col
      > and curwin->w_cursor.lnum. However, curwin->w_cusor.col may
      > not always be initialized properly before calling
      > findmatchlimit(). In misc1.c, there are several places
      > which modify curwin->w_cursor.lnum without updating
      > curwin->w_cursor.col (which is maybe ok as long as
      > curwin->w_cursor.col is not used). But if curwin->w_cursor.col
      > is used, then curwin->w_cursor.col can very well go beyond
      > the end of the line.
      >
      > I found one place where adding initialization of
      > 'curwin->w_cursor.col' fixes the problem.
      >
      > I attach a one line patch but I'm far from being sure that
      > this is the right way to fix the problem.
      >
      > I'm using vim-7.1.262 (huge) on Linux x86 with utf8 locale.

      Thanks for locating this problem.

      I think it's best to always make sure the cursor position is valid. So
      every time curwin->w_cursor.lnum is changed the column must be set too.
      Using zero when incrementing/decrementing the line number. Perhaps it's
      set a bit too often then, but that's better than making a mistake with
      assuming the column is valid while it isn't.

      --
      I started out with nothing, and I still have most of it.
      -- Michael Davis -- "Tonight Show"

      /// 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.