Re: Patch 8.1.0261
Yegappan wrote: > >> In all the places where qf_restore_list() is currently invoked, there > >> is a call to > >> qflist_valid() to check whether the list with the specified id is > >> present or not. > >> If the list is not present, then qf_restore_list() is not called. But > >> in the future, > >> someone might call qf_restore_list() without first checking whether the > >> list > >> is still present in the stack or not. > > > > Yes, so the current construction is brittle, if someone makes changes > > and forgets to call qflist_valid() then things break. That's basically > > what Coverity complains about: unless you are careful qf_curlist becomes > > negative. > > > > We should move qflist_valid() into qf_restore_list(), and let it return > > FAIL when the list isn't valid. > > > > There is no need to call qflist_valid() in qf_restore_list() as the qf_id2nr() > function (called by qf_restore_list()) already iterates through all the lists > in the stack. > > > > > Then the caller must deal with the > > failure. This also applies to qf_jump_first() indirectly. > > > > Can you make it work like that? > > I am attaching a patch that does this. Thanks! -- BRIDGEKEEPER: What is your favorite colour? GAWAIN: Blue ... No yelloww! "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD /// Bram Moolenaar -- b...@moolenaar.net -- 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+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: Patch 8.1.0261
Hi Bram,
On Fri, Aug 10, 2018 at 7:45 AM, Bram Moolenaar wrote:
>
> Yegappan wrote:
>
>> On Thu, Aug 9, 2018 at 12:52 PM, Bram Moolenaar wrote:
>> >
>> > Patch 8.1.0261
>> > Problem:Coverity complains about a negative array index.
>> > Solution: When qf_id2nr() cannot find the list then don't set qf_curlist.
>> > Files: src/quickfix.c
>> >
>> >
>> > *** ../vim-8.1.0260/src/quickfix.c 2018-08-09 21:19:15.774436077 +0200
>> > --- src/quickfix.c 2018-08-09 21:49:36.929501416 +0200
>> > ***
>> > *** 4339,4352
>> > }
>> >
>> > /*
>> >* Jump to the first entry if there is one.
>> >*/
>> > static void
>> > qf_jump_first(qf_info_T *qi, int_u save_qfid, int forceit)
>> > {
>> > ! // If autocommands changed the current list, then restore it
>> > ! if (qi->qf_lists[qi->qf_curlist].qf_id != save_qfid)
>> > ! qi->qf_curlist = qf_id2nr(qi, save_qfid);
>> >
>> > // Autocommands might have cleared the list, check for it
>> > if (!qf_list_empty(qi, qi->qf_curlist))
>> > --- 4335,4365
>> > }
>> >
>> > /*
>> > + * If the current list is not "save_qfid" and we can find the list with
>> > that ID
>> > + * then make it the current list.
>> > + * This is used when autocommands may have changed the current list.
>> > + */
>> > + static void
>> > + qf_restore_list(qf_info_T *qi, int_u save_qfid)
>> > + {
>> > + int curlist;
>> > +
>> > + if (qi->qf_lists[qi->qf_curlist].qf_id != save_qfid)
>> > + {
>> > + curlist = qf_id2nr(qi, save_qfid);
>> > + if (curlist >= 0)
>> > + qi->qf_curlist = curlist;
>> > + // else: what if the list can't be found?
>> >
>>
>> In all the places where qf_restore_list() is currently invoked, there
>> is a call to
>> qflist_valid() to check whether the list with the specified id is
>> present or not.
>> If the list is not present, then qf_restore_list() is not called. But
>> in the future,
>> someone might call qf_restore_list() without first checking whether the list
>> is still present in the stack or not.
>
> Yes, so the current construction is brittle, if someone makes changes
> and forgets to call qflist_valid() then things break. That's basically
> what Coverity complains about: unless you are careful qf_curlist becomes
> negative.
>
> We should move qflist_valid() into qf_restore_list(), and let it return
> FAIL when the list isn't valid.
>
There is no need to call qflist_valid() in qf_restore_list() as the qf_id2nr()
function (called by qf_restore_list()) already iterates through all the lists
in the stack.
>
> Then the caller must deal with the
> failure. This also applies to qf_jump_first() indirectly.
>
> Can you make it work like that?
>
I am attaching a patch that does this.
- Yegappan
--
--
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
qfempty.diff
Description: Binary data
Re: Patch 8.1.0261
Yegappan wrote:
> On Thu, Aug 9, 2018 at 12:52 PM, Bram Moolenaar wrote:
> >
> > Patch 8.1.0261
> > Problem:Coverity complains about a negative array index.
> > Solution: When qf_id2nr() cannot find the list then don't set qf_curlist.
> > Files: src/quickfix.c
> >
> >
> > *** ../vim-8.1.0260/src/quickfix.c 2018-08-09 21:19:15.774436077 +0200
> > --- src/quickfix.c 2018-08-09 21:49:36.929501416 +0200
> > ***
> > *** 4339,4352
> > }
> >
> > /*
> >* Jump to the first entry if there is one.
> >*/
> > static void
> > qf_jump_first(qf_info_T *qi, int_u save_qfid, int forceit)
> > {
> > ! // If autocommands changed the current list, then restore it
> > ! if (qi->qf_lists[qi->qf_curlist].qf_id != save_qfid)
> > ! qi->qf_curlist = qf_id2nr(qi, save_qfid);
> >
> > // Autocommands might have cleared the list, check for it
> > if (!qf_list_empty(qi, qi->qf_curlist))
> > --- 4335,4365
> > }
> >
> > /*
> > + * If the current list is not "save_qfid" and we can find the list with
> > that ID
> > + * then make it the current list.
> > + * This is used when autocommands may have changed the current list.
> > + */
> > + static void
> > + qf_restore_list(qf_info_T *qi, int_u save_qfid)
> > + {
> > + int curlist;
> > +
> > + if (qi->qf_lists[qi->qf_curlist].qf_id != save_qfid)
> > + {
> > + curlist = qf_id2nr(qi, save_qfid);
> > + if (curlist >= 0)
> > + qi->qf_curlist = curlist;
> > + // else: what if the list can't be found?
> >
>
> In all the places where qf_restore_list() is currently invoked, there
> is a call to
> qflist_valid() to check whether the list with the specified id is
> present or not.
> If the list is not present, then qf_restore_list() is not called. But
> in the future,
> someone might call qf_restore_list() without first checking whether the list
> is still present in the stack or not.
Yes, so the current construction is brittle, if someone makes changes
and forgets to call qflist_valid() then things break. That's basically
what Coverity complains about: unless you are careful qf_curlist becomes
negative.
We should move qflist_valid() into qf_restore_list(), and let it return
FAIL when the list isn't valid. Then the caller must deal with the
failure. This also applies to qf_jump_first() indirectly.
Can you make it work like that?
--
Shaw's Principle: Build a system that even a fool can use, and
only a fool will want to use it.
/// Bram Moolenaar -- b...@moolenaar.net -- 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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Re: Patch 8.1.0261
Hi Bram,
On Thu, Aug 9, 2018 at 12:52 PM, Bram Moolenaar wrote:
>
> Patch 8.1.0261
> Problem:Coverity complains about a negative array index.
> Solution: When qf_id2nr() cannot find the list then don't set qf_curlist.
> Files: src/quickfix.c
>
>
> *** ../vim-8.1.0260/src/quickfix.c 2018-08-09 21:19:15.774436077 +0200
> --- src/quickfix.c 2018-08-09 21:49:36.929501416 +0200
> ***
> *** 4339,4352
> }
>
> /*
>* Jump to the first entry if there is one.
>*/
> static void
> qf_jump_first(qf_info_T *qi, int_u save_qfid, int forceit)
> {
> ! // If autocommands changed the current list, then restore it
> ! if (qi->qf_lists[qi->qf_curlist].qf_id != save_qfid)
> ! qi->qf_curlist = qf_id2nr(qi, save_qfid);
>
> // Autocommands might have cleared the list, check for it
> if (!qf_list_empty(qi, qi->qf_curlist))
> --- 4335,4365
> }
>
> /*
> + * If the current list is not "save_qfid" and we can find the list with
> that ID
> + * then make it the current list.
> + * This is used when autocommands may have changed the current list.
> + */
> + static void
> + qf_restore_list(qf_info_T *qi, int_u save_qfid)
> + {
> + int curlist;
> +
> + if (qi->qf_lists[qi->qf_curlist].qf_id != save_qfid)
> + {
> + curlist = qf_id2nr(qi, save_qfid);
> + if (curlist >= 0)
> + qi->qf_curlist = curlist;
> + // else: what if the list can't be found?
>
In all the places where qf_restore_list() is currently invoked, there
is a call to
qflist_valid() to check whether the list with the specified id is
present or not.
If the list is not present, then qf_restore_list() is not called. But
in the future,
someone might call qf_restore_list() without first checking whether the list
is still present in the stack or not.
Regards,
Yegappan
> + }
> + }
> +
> + /*
--
--
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
