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

[PATCH] Fixes segfault situation in pum_redraw

Expand Messages
  • Francisco Lopes
    I ve applied the following patch to pum_redraw function to avoid rare segfaults I was suffering. diff -r 92c9748e0ccb src/popupmnu.c ... +++ b/src/popupmnu.c
    Message 1 of 8 , Oct 21, 2013
    • 0 Attachment
      I've applied the following patch to pum_redraw function to avoid rare segfaults I was suffering.

      diff -r 92c9748e0ccb src/popupmnu.c
      --- a/src/popupmnu.c Sun Oct 06 17:46:56 2013 +0200
      +++ b/src/popupmnu.c Mon Oct 21 12:29:31 2013 -0200
      @@ -295,6 +295,8 @@
      for (i = 0; i < pum_height; ++i)
      {
      idx = i + pum_first;
      + if(idx >= pum_height)
      + break;
      attr = (idx == pum_selected) ? attr_select : attr_norm;

      /* prepend a space if there is room */

      I've discovered the situation while debugging https://github.com/Valloric/YouCompleteMe/issues/593 but I'm unable to explain all the characteristics that leads to the segfault situation (that happens only occasionally). I just verified that idx was getting beyond pum_height in the last step of the loop, and this was a problem when accessing pum_array[idx], for example, referencing invalid memory.

      --
      --
      You received this message from the "vim_dev" maillist.
      Do not top-post! Type your reply below the text you are replying to.
      For more information, visit http://www.vim.org/maillist.php

      ---
      You received this message because you are subscribed to the Google Groups "vim_dev" group.
      To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
      For more options, visit https://groups.google.com/groups/opt_out.
    • Francisco Lopes
      specifically while debugging, pum_first was 1 making idx beyond arrays bounds. -- -- You received this message from the vim_dev maillist. Do not top-post!
      Message 2 of 8 , Oct 21, 2013
      • 0 Attachment
        specifically while debugging, pum_first was 1 making idx beyond arrays bounds.

        --
        --
        You received this message from the "vim_dev" maillist.
        Do not top-post! Type your reply below the text you are replying to.
        For more information, visit http://www.vim.org/maillist.php

        ---
        You received this message because you are subscribed to the Google Groups "vim_dev" group.
        To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
        For more options, visit https://groups.google.com/groups/opt_out.
      • Bram Moolenaar
        ... Thanks for the patch. It would be good if you at least have an idea why this happens. Perhaps pum_first is changed inside the loop? -- Two cows are
        Message 3 of 8 , Oct 21, 2013
        • 0 Attachment
          Francisco Lopes wrote:

          > I've applied the following patch to pum_redraw function to avoid rare segfaults I was suffering.
          >
          > diff -r 92c9748e0ccb src/popupmnu.c
          > --- a/src/popupmnu.c Sun Oct 06 17:46:56 2013 +0200
          > +++ b/src/popupmnu.c Mon Oct 21 12:29:31 2013 -0200
          > @@ -295,6 +295,8 @@
          > for (i = 0; i < pum_height; ++i)
          > {
          > idx = i + pum_first;
          > + if(idx >= pum_height)
          > + break;
          > attr = (idx == pum_selected) ? attr_select : attr_norm;
          >
          > /* prepend a space if there is room */
          >
          > I've discovered the situation while debugging
          > https://github.com/Valloric/YouCompleteMe/issues/593 but I'm unable to
          > explain all the characteristics that leads to the segfault situation
          > (that happens only occasionally). I just verified that idx was getting
          > beyond pum_height in the last step of the loop, and this was a problem
          > when accessing pum_array[idx], for example, referencing invalid
          > memory.

          Thanks for the patch. It would be good if you at least have an idea why
          this happens. Perhaps pum_first is changed inside the loop?


          --
          Two cows are standing together in a field. One asks the other:
          "So what do you think about this Mad Cow Disease?"
          The other replies: "That doesn't concern me. I'm a helicopter."

          /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
          /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
          \\\ an exciting new programming language -- http://www.Zimbu.org ///
          \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

          --
          --
          You received this message from the "vim_dev" maillist.
          Do not top-post! Type your reply below the text you are replying to.
          For more information, visit http://www.vim.org/maillist.php

          ---
          You received this message because you are subscribed to the Google Groups "vim_dev" group.
          To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
          For more options, visit https://groups.google.com/groups/opt_out.
        • Francisco Lopes
          ... I really didn t seek quite deep to figure out how the situation got setup, but, it seems clear there s a problem with that coding since it uses idx to
          Message 4 of 8 , Oct 21, 2013
          • 0 Attachment
            Bram Moolenaar wrote:
            > Francisco Lopes wrote:
            >
            >
            >
            > > I've applied the following patch to pum_redraw function to avoid rare segfaults I was suffering.
            >
            > >
            >
            > > diff -r 92c9748e0ccb src/popupmnu.c
            >
            > > --- a/src/popupmnu.c Sun Oct 06 17:46:56 2013 +0200
            >
            > > +++ b/src/popupmnu.c Mon Oct 21 12:29:31 2013 -0200
            >
            > > @@ -295,6 +295,8 @@
            >
            > > for (i = 0; i < pum_height; ++i)
            >
            > > {
            >
            > > idx = i + pum_first;
            >
            > > + if(idx >= pum_height)
            >
            > > + break;
            >
            > > attr = (idx == pum_selected) ? attr_select : attr_norm;
            >
            > >
            >
            > > /* prepend a space if there is room */
            >
            > >
            >
            > > I've discovered the situation while debugging
            >
            > > https://github.com/Valloric/YouCompleteMe/issues/593 but I'm unable to
            >
            > > explain all the characteristics that leads to the segfault situation
            >
            > > (that happens only occasionally). I just verified that idx was getting
            >
            > > beyond pum_height in the last step of the loop, and this was a problem
            >
            > > when accessing pum_array[idx], for example, referencing invalid
            >
            > > memory.
            >
            >
            >
            > Thanks for the patch. It would be good if you at least have an idea why
            >
            > this happens. Perhaps pum_first is changed inside the loop?
            >

            I really didn't seek quite deep to figure out how the situation got setup, but, it seems clear there's a problem with that coding since it uses idx to index stuff but it should go beyond pum_height anytime pum_first is greater than 0.

            Sorry for not digging it depply.

            Regards,
            Francisco Lopes.

            --
            --
            You received this message from the "vim_dev" maillist.
            Do not top-post! Type your reply below the text you are replying to.
            For more information, visit http://www.vim.org/maillist.php

            ---
            You received this message because you are subscribed to the Google Groups "vim_dev" group.
            To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
            For more options, visit https://groups.google.com/groups/opt_out.
          • Francisco Lopes
            some more info originally from https://github.com/Valloric/YouCompleteMe/issues/618#issuecomment-27265698: @Valloric my real advice is that, they should run
            Message 5 of 8 , Oct 28, 2013
            • 0 Attachment
              some more info originally from
              https://github.com/Valloric/YouCompleteMe/issues/618#issuecomment-27265698:

              @Valloric my real advice is that, they should run it,
              as soon as this get fixed upstream,
              this is a plain bug as can be checked in the vim thread.
              I've debugged it a bit more and got more info,
              I've figured out syntastic was related and indeed,
              disabling it avoided the buggy situation, the thing is
              that while tabbing a `:redraw` is issued from syntastic's `s:Redraw()`
              and also, the displaying of the popup menu is issued through
              the insert completion functionality. The mess happens in
              `popupmnu.c`, where there's a global variable `pum_first`,
              this pum_first gets changed both by the drawing issued by
              insert completion and by a `:redraw` (from `s:Redraw()`)
              that ends up calling `void pum_redraw()`.

              At some point, because of this:

              if (pum_first > pum_selected - context)
              {
              /* scroll down */
              pum_first = pum_selected - context;
              if (pum_first < 0)
              pum_first = 0;
              }

              `pum_first` starts to get values greater than 0, and keeps
              this value, once `pum_redraw` is called, the segmentation
              fault situation is triggered as the `idx` variable used for
              indexing an array in `pum_redraw` will start to index beyond
              array bounds.

              So, a lame workaround would be to avoid extra redraws that
              ends up inherenting state from other popup menu drawing
              until this state (the `pum_first` global variable) gets
              reset to its default state.

              I can only say this is messy, and can only blame Vim codebase.

              --
              --
              You received this message from the "vim_dev" maillist.
              Do not top-post! Type your reply below the text you are replying to.
              For more information, visit http://www.vim.org/maillist.php

              ---
              You received this message because you are subscribed to the Google Groups "vim_dev" group.
              To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
              For more options, visit https://groups.google.com/groups/opt_out.
            • Bram Moolenaar
              ... It appears the problem is that pum_first and pum_height are out of sync. pum_first is corrected only after the code that causes a redraw for the preview
              Message 6 of 8 , Nov 1, 2013
              • 0 Attachment
                francisco Lopes wrote:

                > some more info originally from
                > https://github.com/Valloric/YouCompleteMe/issues/618#issuecomment-27265698:
                >
                > @Valloric my real advice is that, they should run it,
                > as soon as this get fixed upstream,
                > this is a plain bug as can be checked in the vim thread.
                > I've debugged it a bit more and got more info,
                > I've figured out syntastic was related and indeed,
                > disabling it avoided the buggy situation, the thing is
                > that while tabbing a `:redraw` is issued from syntastic's `s:Redraw()`
                > and also, the displaying of the popup menu is issued through
                > the insert completion functionality. The mess happens in
                > `popupmnu.c`, where there's a global variable `pum_first`,
                > this pum_first gets changed both by the drawing issued by
                > insert completion and by a `:redraw` (from `s:Redraw()`)
                > that ends up calling `void pum_redraw()`.
                >
                > At some point, because of this:
                >
                > if (pum_first > pum_selected - context)
                > {
                > /* scroll down */
                > pum_first = pum_selected - context;
                > if (pum_first < 0)
                > pum_first = 0;
                > }
                >
                > `pum_first` starts to get values greater than 0, and keeps
                > this value, once `pum_redraw` is called, the segmentation
                > fault situation is triggered as the `idx` variable used for
                > indexing an array in `pum_redraw` will start to index beyond
                > array bounds.
                >
                > So, a lame workaround would be to avoid extra redraws that
                > ends up inherenting state from other popup menu drawing
                > until this state (the `pum_first` global variable) gets
                > reset to its default state.
                >
                > I can only say this is messy, and can only blame Vim codebase.

                It appears the problem is that pum_first and pum_height are out of sync.
                pum_first is corrected only after the code that causes a redraw for the
                preview window.

                Please verify the patch below also fixes the crash. It's better in the
                sense that it corrects pum_first, instead of displaying the wrong items
                and then aborting when going past the end of pum_array.

                *** ../vim-7.4.052/src/popupmnu.c 2011-08-17 18:04:28.000000000 +0200
                --- src/popupmnu.c 2013-11-02 04:01:06.000000000 +0100
                ***************
                *** 282,287 ****
                --- 282,291 ----
                int round;
                int n;

                + /* Never display more than we have */
                + if (pum_first > pum_size - pum_height)
                + pum_first = pum_size - pum_height;
                +
                if (pum_scrollbar)
                {
                thumb_heigth = pum_height * pum_height / pum_size;
                ***************
                *** 672,681 ****
                #endif
                }

                - /* Never display more than we have */
                - if (pum_first > pum_size - pum_height)
                - pum_first = pum_size - pum_height;
                -
                if (!resized)
                pum_redraw();

                --- 676,681 ----


                --
                BEDEVERE: How do you know so much about swallows?
                ARTHUR: Well you have to know these things when you're a king, you know.
                "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

                /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
                /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
                \\\ an exciting new programming language -- http://www.Zimbu.org ///
                \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

                --
                --
                You received this message from the "vim_dev" maillist.
                Do not top-post! Type your reply below the text you are replying to.
                For more information, visit http://www.vim.org/maillist.php

                ---
                You received this message because you are subscribed to the Google Groups "vim_dev" group.
                To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
                For more options, visit https://groups.google.com/groups/opt_out.
              • Francisco Lopes
                ... Yes this patch also fixes the issue, but, I d just like to warn that, in my opinion, the statement assigning to idx continues not making sense. The loop
                Message 7 of 8 , Nov 6, 2013
                • 0 Attachment
                  Bram Moolenaar wrote:
                  >
                  > It appears the problem is that pum_first and pum_height are out of sync.
                  >
                  > pum_first is corrected only after the code that causes a redraw for the
                  >
                  > preview window.
                  >
                  >
                  >
                  > Please verify the patch below also fixes the crash. It's better in the
                  >
                  > sense that it corrects pum_first, instead of displaying the wrong items
                  >
                  > and then aborting when going past the end of pum_array.
                  >
                  >
                  >
                  > *** ../vim-7.4.052/src/popupmnu.c 2011-08-17 18:04:28.000000000 +0200
                  >
                  > --- src/popupmnu.c 2013-11-02 04:01:06.000000000 +0100
                  >
                  > ***************
                  >
                  > *** 282,287 ****
                  >
                  > --- 282,291 ----
                  >
                  > int round;
                  >
                  > int n;
                  >
                  >
                  >
                  > + /* Never display more than we have */
                  >
                  > + if (pum_first > pum_size - pum_height)
                  >
                  > + pum_first = pum_size - pum_height;
                  >
                  > +
                  >
                  > if (pum_scrollbar)
                  >
                  > {
                  >
                  > thumb_heigth = pum_height * pum_height / pum_size;
                  >
                  > ***************
                  >
                  > *** 672,681 ****
                  >
                  > #endif
                  >
                  > }
                  >
                  >
                  >
                  > - /* Never display more than we have */
                  >
                  > - if (pum_first > pum_size - pum_height)
                  >
                  > - pum_first = pum_size - pum_height;
                  >
                  > -
                  >
                  > if (!resized)
                  >
                  > pum_redraw();
                  >
                  >
                  >
                  > --- 676,681 ----
                  >

                  Yes this patch also fixes the issue, but, I'd just like to warn that, in my opinion, the statement assigning to idx continues not making sense. The loop goes up to a maximum value, and the internal loop index shouldn't be beyond this, so, idx only makes sense when pum_first is zero, hence only when idx == i.

                  --
                  --
                  You received this message from the "vim_dev" maillist.
                  Do not top-post! Type your reply below the text you are replying to.
                  For more information, visit http://www.vim.org/maillist.php

                  ---
                  You received this message because you are subscribed to the Google Groups "vim_dev" group.
                  To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
                  For more options, visit https://groups.google.com/groups/opt_out.
                • Bram Moolenaar
                  ... You are forgetting about the situation where pum_size pum_height. -- I love deadlines. I especially like the whooshing sound they make as they go
                  Message 8 of 8 , Nov 6, 2013
                  • 0 Attachment
                    Francisco Lopes wrote:

                    > Bram Moolenaar wrote:
                    > >
                    > > It appears the problem is that pum_first and pum_height are out of sync.
                    > > pum_first is corrected only after the code that causes a redraw for the
                    > > preview window.
                    > >
                    > > Please verify the patch below also fixes the crash. It's better in the
                    > > sense that it corrects pum_first, instead of displaying the wrong items
                    > > and then aborting when going past the end of pum_array.
                    > >
                    > > *** ../vim-7.4.052/src/popupmnu.c 2011-08-17 18:04:28.000000000 +0200
                    > > --- src/popupmnu.c 2013-11-02 04:01:06.000000000 +0100
                    > > ***************
                    > > *** 282,287 ****
                    > > --- 282,291 ----
                    > > int round;
                    > > int n;
                    > >
                    > > + /* Never display more than we have */
                    > > + if (pum_first > pum_size - pum_height)
                    > > + pum_first = pum_size - pum_height;
                    > > +
                    > > if (pum_scrollbar)
                    > > {
                    > > thumb_heigth = pum_height * pum_height / pum_size;
                    > > ***************
                    > > *** 672,681 ****
                    > > #endif
                    > > }
                    > >
                    > > - /* Never display more than we have */
                    > > - if (pum_first > pum_size - pum_height)
                    > > - pum_first = pum_size - pum_height;
                    > > -
                    > > if (!resized)
                    > > pum_redraw();
                    > >
                    > > --- 676,681 ----
                    >
                    > Yes this patch also fixes the issue, but, I'd just like to warn that,
                    > in my opinion, the statement assigning to idx continues not making
                    > sense. The loop goes up to a maximum value, and the internal loop
                    > index shouldn't be beyond this, so, idx only makes sense when
                    > pum_first is zero, hence only when idx == i.

                    You are forgetting about the situation where pum_size > pum_height.

                    --
                    "I love deadlines. I especially like the whooshing sound they
                    make as they go flying by."
                    -- Douglas Adams

                    /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
                    /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
                    \\\ an exciting new programming language -- http://www.Zimbu.org ///
                    \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

                    --
                    --
                    You received this message from the "vim_dev" maillist.
                    Do not top-post! Type your reply below the text you are replying to.
                    For more information, visit http://www.vim.org/maillist.php

                    ---
                    You received this message because you are subscribed to the Google Groups "vim_dev" group.
                    To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
                    For more options, visit https://groups.google.com/groups/opt_out.
                  Your message has been successfully submitted and would be delivered to recipients shortly.