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

patch to fix uninitialised memory access in utf_off2cells()

Expand Messages
  • Dominique Pelle
    Hi Valgrind memory checker reports the following error: ==10724== Conditional jump or move depends on uninitialized value(s) ==10724== at 0x810A1EB:
    Message 1 of 4 , Aug 21, 2007
    View Source
    • 0 Attachment
      Hi

      Valgrind memory checker reports the following error:

      ==10724== Conditional jump or move depends on uninitialized value(s)
      ==10724== at 0x810A1EB: utf_off2cells (mbyte.c:1338)
      ==10724== by 0x815265C: screen_puts_len (screen.c:6326)
      ==10724== by 0x80EE251: screen_puts_mbyte (message.c:1684)
      ==10724== by 0x80EE6FE: msg_puts_display (message.c:1907)
      ==10724== by 0x80EE4B7: msg_puts_attr_len (message.c:1819)
      ==10724== by 0x80EE3F9: msg_puts_attr (message.c:1767)
      ==10724== by 0x80EE2E5: msg_puts (message.c:1717)
      ==10724== by 0x81A4A3A: version_msg (version.c:1184)
      ==10724== by 0x81A499C: list_version (version.c:1151)
      ==10724== by 0x81A46DC: ex_version (version.c:876)
      ==10724== by 0x809A1FB: do_one_cmd (ex_docmd.c:2622)
      ==10724== by 0x8097B18: do_cmdline (ex_docmd.c:1100)
      ==10724== by 0x80971E6: do_cmdline_cmd (ex_docmd.c:706)
      ==10724== by 0x80D752C: exe_commands (main.c:2638)
      ==10724== by 0x80D54F6: main (main.c:874)

      Error can be reproduced by simply running the ":version"
      command with the following conditions:
      - vim must be built with multi-byte support (error does not
      happen without multi-byte support)
      - terminal must be small (4 lines or less). Error does
      not happen in terminal larger than 4 lines.

      In a small xterm 80 columns x 4 lines, I can get the
      error with the following command:

      valgrind --num-callers=18 ./vim -u NONE -c ':version' 2> vg.log

      The relevant code is:

      mbyte.c:

      1334 int
      1335 utf_off2cells(off)
      1336 unsigned off;
      1337 {
      !!1338 return ScreenLines[off + 1] == 0 ? 2 : 1;
      1339 }

      By adding some printf, I can see that 'off' parameter is
      initialized, but ScreenLines[off + 1] is uninitialized
      when accessed at line mbyte.c:1338.

      Adding some printf(), I can see that when error happens,
      off value is 319, so line mbyte.c:1338 accesses ScreenLines[320]
      which is uninitialized. 320 happens to match the size of my
      small terminal 80x4 = 320).

      From what I understand (I could be wrong), some Unicode characters
      may occupies two cells on the screen. I'm not sure what happens
      if such character was in the last cell of the screen (bottom
      right of terminal) or if such character was in the last column
      of any row of the screen. It looks like function utf_off2cells()
      accesses one cell beyond the screen to check for double cell
      Unicode characters.

      I also see that function screen_char_2() which writes into the
      screen ScreenLines[] handles last character of the screen
      specially to avoid scroll up (it avoids writing in last
      character).

      I can fix the error reported by valgrind with the attached
      patch, but I do not understand screen.c enough to say whether
      it's the correct approach or not.

      I can also reproduce the same kind of error in a bigger terminal
      when running cscope queries which result in lots of matches such
      as ":cs find e e" with cscope database of vim source tree for
      example (then pressing spaces several times to see results).
      Using a 80x24 terminal, the error then happens with off=1919
      (which again corresponds to number of characters in terminal
      (80x24 = 1920):

      ==21069== Conditional jump or move depends on uninitialised value(s)
      ==21069== at 0x8101643: utf_off2cells (mbyte.c:1338)
      ==21069== by 0x8147550: screen_puts_len (screen.c:6326)
      ==21069== by 0x80E5958: screen_puts_mbyte (message.c:1684)
      ==21069== by 0x80E5D3F: msg_puts_display (message.c:1907)
      ==21069== by 0x80E5B86: msg_puts_attr_len (message.c:1819)
      ==21069== by 0x80E51D1: msg_outtrans_len_attr (message.c:1383)
      ==21069== by 0x80E5AA6: msg_puts_long_len_attr (message.c:1756)
      ==21069== by 0x80E5A0E: msg_puts_long_attr (message.c:1737)
      ==21069== by 0x80CFF6B: cs_print_tags_priv (if_cscope.c:1987)
      ==21069== by 0x80CF477: cs_manage_matches (if_cscope.c:1626)
      ==21069== by 0x80CD228: cs_print_tags (if_cscope.c:291)
      ==21069== by 0x8181DCB: do_tag (tag.c:599)
      ==21069== by 0x80CE872: cs_find_common (if_cscope.c:1158)
      ==21069== by 0x80CE2E9: cs_find (if_cscope.c:970)
      ==21069== by 0x80CCED2: do_cscope_general (if_cscope.c:137)
      ==21069== by 0x80CCF01: do_cscope (if_cscope.c:152)
      ==21069== by 0x80964AF: do_one_cmd (ex_docmd.c:2622)
      ==21069== by 0x8093E3D: do_cmdline (ex_docmd.c:1100)
      ==21069== by 0x810ADD5: nv_colon (normal.c:5168)

      I use vim-7.1 (patches 1-87) configured with
      --enable-multibyte and --enable-cscope compiled
      without optimization (-g -O0) on Linux x86.

      Attached: patch-uninit-mem-access-utf_off2cells.txt

      -- Dominique

      --~--~---------~--~----~------------~-------~--~----~
      You received this message from the "vim_dev" maillist.
      For more information, visit http://www.vim.org/maillist.php
      -~----------~----~----~----~------~----~------~--~---
    • Bram Moolenaar
      ... That should not happen. The last cell in the line can never contain a double-width character. Thus the check should not be done there. ... It s not
      Message 2 of 4 , Aug 22, 2007
      View Source
      • 0 Attachment
        Dominique Pelle wrote:

        > Valgrind memory checker reports the following error:
        >
        > ==10724== Conditional jump or move depends on uninitialized value(s)
        > ==10724== at 0x810A1EB: utf_off2cells (mbyte.c:1338)
        > ==10724== by 0x815265C: screen_puts_len (screen.c:6326)
        > ==10724== by 0x80EE251: screen_puts_mbyte (message.c:1684)
        > ==10724== by 0x80EE6FE: msg_puts_display (message.c:1907)
        > ==10724== by 0x80EE4B7: msg_puts_attr_len (message.c:1819)
        > ==10724== by 0x80EE3F9: msg_puts_attr (message.c:1767)
        > ==10724== by 0x80EE2E5: msg_puts (message.c:1717)
        > ==10724== by 0x81A4A3A: version_msg (version.c:1184)
        > ==10724== by 0x81A499C: list_version (version.c:1151)
        > ==10724== by 0x81A46DC: ex_version (version.c:876)
        > ==10724== by 0x809A1FB: do_one_cmd (ex_docmd.c:2622)
        > ==10724== by 0x8097B18: do_cmdline (ex_docmd.c:1100)
        > ==10724== by 0x80971E6: do_cmdline_cmd (ex_docmd.c:706)
        > ==10724== by 0x80D752C: exe_commands (main.c:2638)
        > ==10724== by 0x80D54F6: main (main.c:874)
        >
        > Error can be reproduced by simply running the ":version"
        > command with the following conditions:
        > - vim must be built with multi-byte support (error does not
        > happen without multi-byte support)
        > - terminal must be small (4 lines or less). Error does
        > not happen in terminal larger than 4 lines.
        >
        > In a small xterm 80 columns x 4 lines, I can get the
        > error with the following command:
        >
        > valgrind --num-callers=18 ./vim -u NONE -c ':version' 2> vg.log
        >
        > The relevant code is:
        >
        > mbyte.c:
        >
        > 1334 int
        > 1335 utf_off2cells(off)
        > 1336 unsigned off;
        > 1337 {
        > !!1338 return ScreenLines[off + 1] == 0 ? 2 : 1;
        > 1339 }
        >
        > By adding some printf, I can see that 'off' parameter is
        > initialized, but ScreenLines[off + 1] is uninitialized
        > when accessed at line mbyte.c:1338.
        >
        > Adding some printf(), I can see that when error happens,
        > off value is 319, so line mbyte.c:1338 accesses ScreenLines[320]
        > which is uninitialized. 320 happens to match the size of my
        > small terminal 80x4 = 320).
        >
        > From what I understand (I could be wrong), some Unicode characters
        > may occupies two cells on the screen. I'm not sure what happens
        > if such character was in the last cell of the screen (bottom
        > right of terminal) or if such character was in the last column
        > of any row of the screen. It looks like function utf_off2cells()
        > accesses one cell beyond the screen to check for double cell
        > Unicode characters.

        That should not happen. The last cell in the line can never contain a
        double-width character. Thus the check should not be done there.

        > I also see that function screen_char_2() which writes into the
        > screen ScreenLines[] handles last character of the screen
        > specially to avoid scroll up (it avoids writing in last
        > character).
        >
        > I can fix the error reported by valgrind with the attached
        > patch, but I do not understand screen.c enough to say whether
        > it's the correct approach or not.

        It's not actually correct, although it probably works. You check if you
        don't go past the end of the screen, but the check needed is to verify
        we don't go past the end of the line. Since the first cell in the next
        line is never the second half of a double-width character, your check
        would also work.

        Perhaps you can add a "max_off" variable, set to LineOffset[row] +
        screen_Columns. Then avoid calling mb_off2cells(off) when off is
        max_off - 2 or more.

        --
        The term "free software" is defined by Richard M. Stallman as
        being software that isn't necessarily for free. Confusing?
        Let's call it "Stallman software" then!
        -- Bram Moolenaar

        /// 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
        ... With your indications, I modified my original patch. My previous patch actually did not fix all accesses to uninitialized memory. More errors were
        Message 3 of 4 , Aug 22, 2007
        View Source
        • 0 Attachment
          On 8/22/07, Bram Moolenaar <Bram@...> wrote:

          > Perhaps you can add a "max_off" variable, set to LineOffset[row] +
          > screen_Columns. Then avoid calling mb_off2cells(off) when off is
          > max_off - 2 or more.

          With your indications, I modified my original patch. My previous
          patch actually did not fix all accesses to uninitialized memory.
          More errors were reported when writing characters that occupy two
          cells at the bottom right of the screen. An easy way to reproduce
          it was to type ":" to enter Ex mode, and then type some text till the
          final cell of the screen. Valgrind then reported errors. I tried
          with single cell characters (ASCII characters for example) as well
          as double cell characters (Chinese characters for example).

          Since the boolean expressions was already quite complicated, I did
          not feel like making it even more complicated by adding some (off
          < max_off - 1) as well as (off < max_off - 2). So I broke the
          boolean expression into a couple of 'if'. I find it easier to
          read it that way. I hope that's OK.

          I attach my new patch. Many things in screen.c are still
          mysterious to me, so definitely review it to make sure it does not
          break anything.

          Attached: patch-uninit-mem-access-utf_off2cells-2.txt

          Cheers
          -- Dominique

          --~--~---------~--~----~------------~-------~--~----~
          You received this message from the "vim_dev" maillist.
          For more information, visit http://www.vim.org/maillist.php
          -~----------~----~----~----~------~----~------~--~---
        • Bram Moolenaar
          ... Thanks for looking into the details. I ll check that this is the best way to do this. There might be other mb_off2cells() calls that have the same
          Message 4 of 4 , Aug 23, 2007
          View Source
          • 0 Attachment
            Dominique Pelle wrote:

            > On 8/22/07, Bram Moolenaar <Bram@...> wrote:
            >
            > > Perhaps you can add a "max_off" variable, set to LineOffset[row] +
            > > screen_Columns. Then avoid calling mb_off2cells(off) when off is
            > > max_off - 2 or more.
            >
            > With your indications, I modified my original patch. My previous
            > patch actually did not fix all accesses to uninitialized memory.
            > More errors were reported when writing characters that occupy two
            > cells at the bottom right of the screen. An easy way to reproduce
            > it was to type ":" to enter Ex mode, and then type some text till the
            > final cell of the screen. Valgrind then reported errors. I tried
            > with single cell characters (ASCII characters for example) as well
            > as double cell characters (Chinese characters for example).
            >
            > Since the boolean expressions was already quite complicated, I did
            > not feel like making it even more complicated by adding some (off
            > < max_off - 1) as well as (off < max_off - 2). So I broke the
            > boolean expression into a couple of 'if'. I find it easier to
            > read it that way. I hope that's OK.
            >
            > I attach my new patch. Many things in screen.c are still
            > mysterious to me, so definitely review it to make sure it does not
            > break anything.
            >
            > Attached: patch-uninit-mem-access-utf_off2cells-2.txt

            Thanks for looking into the details. I'll check that this is the best
            way to do this. There might be other mb_off2cells() calls that have the
            same problem, perhaps passing the maximum and checking it inside the
            function avoids all of them at one go, at the cost of passing an extra
            argument.

            --
            You can tune a file system, but you can't tuna fish
            -- man tunefs

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