patch for vi seg-fault on CTRL-]

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

patch for vi seg-fault on CTRL-]

patrick keshishian
Greetings,

I happened upon this crash when trying to jump to definition
of CIRCQ_INSERT while navigating my way through kernel sources.

Note that len is decremented in the first if-statement and its
value not checked before it gets decremented again at the
start of the for-loop. Since len is of type size_t, the check
'len > 0' fails, since now len = 4294967295 (0xffffffff).  As
the loop continues on we start to access out-of-bounds memory
with '*t++ = *p++' copy.


I suppose another solution would be to check for *p != NULL for
the loop's test condition.

I hope I have explained the issue well enough for someone to
have followed it.

Best,
--patrick


Index: usr.bin/vi/ex/ex_subst.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
retrieving revision 1.15
diff -u -p -r1.15 ex_subst.c
--- usr.bin/vi/ex/ex_subst.c    22 Apr 2006 03:09:15 -0000      1.15
+++ usr.bin/vi/ex/ex_subst.c    26 Mar 2007 09:45:56 -0000
@@ -1221,13 +1221,16 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
         * the backslashes ctags inserts when escaping the search delimiter
         * characters.
         */
-       for (; len > 0; --len) {
+       for (; len != 0;) {
                if (p[0] == '\\' && (p[1] == '/' || p[1] == '?')) {
                        ++p;
                        --len;
                } else if (strchr("^.[]$*", p[0]))
                        *t++ = '\\';
                *t++ = *p++;
+
+               if (len-- == 0)
+                       break;
        }
        if (lastdollar)
                *t++ = '$';

Reply | Threaded
Open this post in threaded view
|

Re: patch for vi seg-fault on CTRL-]

Otto Moerbeek
On Mon, 26 Mar 2007, patrick keshishian wrote:

> Greetings,
>
> I happened upon this crash when trying to jump to definition
> of CIRCQ_INSERT while navigating my way through kernel sources.
>
> Note that len is decremented in the first if-statement and its
> value not checked before it gets decremented again at the
> start of the for-loop. Since len is of type size_t, the check
> 'len > 0' fails, since now len = 4294967295 (0xffffffff).  As
> the loop continues on we start to access out-of-bounds memory
> with '*t++ = *p++' copy.
>
>
> I suppose another solution would be to check for *p != NULL for
> the loop's test condition.
>
> I hope I have explained the issue well enough for someone to
> have followed it.

I see the problem in the code, but I cannot reproduce the crash. Do
you have an exact sequence of actions to trigger the bug?

        -Otto
       

>
> Best,
> --patrick
>
>
> Index: usr.bin/vi/ex/ex_subst.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 ex_subst.c
> --- usr.bin/vi/ex/ex_subst.c    22 Apr 2006 03:09:15 -0000      1.15
> +++ usr.bin/vi/ex/ex_subst.c    26 Mar 2007 09:45:56 -0000
> @@ -1221,13 +1221,16 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
>         * the backslashes ctags inserts when escaping the search delimiter
>         * characters.
>         */
> -       for (; len > 0; --len) {
> +       for (; len != 0;) {
>                if (p[0] == '\\' && (p[1] == '/' || p[1] == '?')) {
>                        ++p;
>                        --len;
>                } else if (strchr("^.[]$*", p[0]))
>                        *t++ = '\\';
>                *t++ = *p++;
> +
> +               if (len-- == 0)
> +                       break;
>        }
>        if (lastdollar)
>                *t++ = '$';

Reply | Threaded
Open this post in threaded view
|

Re: patch for vi seg-fault on CTRL-]

patrick keshishian
Hi Otto,

Sorry for the late reply. I went to sleep after sending out the
email.


On 3/26/07, Otto Moerbeek <[hidden email]> wrote:

>
> On Mon, 26 Mar 2007, patrick keshishian wrote:
>
> > Greetings,
> >
> > I happened upon this crash when trying to jump to definition
> > of CIRCQ_INSERT while navigating my way through kernel sources.
> >
> > Note that len is decremented in the first if-statement and its
> > value not checked before it gets decremented again at the
> > start of the for-loop. Since len is of type size_t, the check
> > 'len > 0' fails, since now len = 4294967295 (0xffffffff).  As
> > the loop continues on we start to access out-of-bounds memory
> > with '*t++ = *p++' copy.
> >
> >
> > I suppose another solution would be to check for *p != NULL for
> > the loop's test condition.
> >
> > I hope I have explained the issue well enough for someone to
> > have followed it.
>
> I see the problem in the code, but I cannot reproduce the crash. Do
> you have an exact sequence of actions to trigger the bug?

It is only reproducible with a "tag" with the last character being a
escape character.  In my case the macro CIRCQ_INSERT.  My tag file
has the following line for its definition location (or whatever the
proper term is for this entry in the "tags" file):


CIRCQ_INSERT    /usr/src/sys/kern/kern_timeout.c        /^#define
CIRCQ_INSERT(elem, list) do {           \\/


It is 100% reproducible with any macro pretty much that is multi-
lined (i.e., the #define has a '\\' at the EOL).

Sequence to reproduce it, assuming you already have tags file
containing macro definitions:
1. Start up vi
2. Load "tags" file (if not implicitly loaded, e.g., tags file named
   differently).
3. Search for maco (e.g., :tag CIRCQ_INSERT)


HTH,
--patrick


>         -Otto
>
> >
> > Best,
> > --patrick
> >
> >
> > Index: usr.bin/vi/ex/ex_subst.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> > retrieving revision 1.15
> > diff -u -p -r1.15 ex_subst.c
> > --- usr.bin/vi/ex/ex_subst.c    22 Apr 2006 03:09:15 -0000      1.15
> > +++ usr.bin/vi/ex/ex_subst.c    26 Mar 2007 09:45:56 -0000
> > @@ -1221,13 +1221,16 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
> >         * the backslashes ctags inserts when escaping the search delimiter
> >         * characters.
> >         */
> > -       for (; len > 0; --len) {
> > +       for (; len != 0;) {
> >                if (p[0] == '\\' && (p[1] == '/' || p[1] == '?')) {
> >                        ++p;
> >                        --len;
> >                } else if (strchr("^.[]$*", p[0]))
> >                        *t++ = '\\';
> >                *t++ = *p++;
> > +
> > +               if (len-- == 0)
> > +                       break;
> >        }
> >        if (lastdollar)
> >                *t++ = '$';

Reply | Threaded
Open this post in threaded view
|

Re: patch for vi seg-fault on CTRL-]

Thordur I. Bjornsson
patrick keshishian <[hidden email]> wrote on Mon 26.Mar'07 at 11:39:09 -0700

Yub. I can reproduce this and the diff fixes it.
looks alright to me.

> It is 100% reproducible with any macro pretty much that is multi-
> lined (i.e., the #define has a '\\' at the EOL).
>
> Sequence to reproduce it, assuming you already have tags file
> containing macro definitions:
> 1. Start up vi
> 2. Load "tags" file (if not implicitly loaded, e.g., tags file named
>   differently).
> 3. Search for maco (e.g., :tag CIRCQ_INSERT)
>
>
> HTH,
> --patrick
>
>
> >        -Otto
> >
> >>
> >> Best,
> >> --patrick
> >>
> >>
> >> Index: usr.bin/vi/ex/ex_subst.c
> >> ===================================================================
> >> RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> >> retrieving revision 1.15
> >> diff -u -p -r1.15 ex_subst.c
> >> --- usr.bin/vi/ex/ex_subst.c    22 Apr 2006 03:09:15 -0000      1.15
> >> +++ usr.bin/vi/ex/ex_subst.c    26 Mar 2007 09:45:56 -0000
> >> @@ -1221,13 +1221,16 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
> >>         * the backslashes ctags inserts when escaping the search
> >delimiter
> >>         * characters.
> >>         */
> >> -       for (; len > 0; --len) {
> >> +       for (; len != 0;) {
> >>                if (p[0] == '\\' && (p[1] == '/' || p[1] == '?')) {
> >>                        ++p;
> >>                        --len;
> >>                } else if (strchr("^.[]$*", p[0]))
> >>                        *t++ = '\\';
> >>                *t++ = *p++;
> >> +
> >> +               if (len-- == 0)
> >> +                       break;
> >>        }
> >>        if (lastdollar)
> >>                *t++ = '$';

Reply | Threaded
Open this post in threaded view
|

Re: patch for vi seg-fault on CTRL-]

Otto Moerbeek
On Mon, 26 Mar 2007, Thordur I. Bjornsson wrote:

> patrick keshishian <[hidden email]> wrote on Mon 26.Mar'07 at 11:39:09 -0700
>
> Yub. I can reproduce this and the diff fixes it.
> looks alright to me.

To me the diff looks a bit awkward. My guess is the diff below is
equivalent.  I still cannot reproduce the problem for some unknown
reason, so please test this.

        -Otto

Index: ex_subst.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
retrieving revision 1.15
diff -u -p -r1.15 ex_subst.c
--- ex_subst.c 22 Apr 2006 03:09:15 -0000 1.15
+++ ex_subst.c 26 Mar 2007 20:31:15 -0000
@@ -1228,6 +1228,8 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
  } else if (strchr("^.[]$*", p[0]))
  *t++ = '\\';
  *t++ = *p++;
+ if (len == 0)
+ break;
  }
  if (lastdollar)
  *t++ = '$';


>
> > It is 100% reproducible with any macro pretty much that is multi-
> > lined (i.e., the #define has a '\\' at the EOL).
> >
> > Sequence to reproduce it, assuming you already have tags file
> > containing macro definitions:
> > 1. Start up vi
> > 2. Load "tags" file (if not implicitly loaded, e.g., tags file named
> >   differently).
> > 3. Search for maco (e.g., :tag CIRCQ_INSERT)
> >
> >
> > HTH,
> > --patrick
> >
> >
> > >        -Otto
> > >
> > >>
> > >> Best,
> > >> --patrick
> > >>
> > >>
> > >> Index: usr.bin/vi/ex/ex_subst.c
> > >> ===================================================================
> > >> RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> > >> retrieving revision 1.15
> > >> diff -u -p -r1.15 ex_subst.c
> > >> --- usr.bin/vi/ex/ex_subst.c    22 Apr 2006 03:09:15 -0000      1.15
> > >> +++ usr.bin/vi/ex/ex_subst.c    26 Mar 2007 09:45:56 -0000
> > >> @@ -1221,13 +1221,16 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
> > >>         * the backslashes ctags inserts when escaping the search
> > >delimiter
> > >>         * characters.
> > >>         */
> > >> -       for (; len > 0; --len) {
> > >> +       for (; len != 0;) {
> > >>                if (p[0] == '\\' && (p[1] == '/' || p[1] == '?')) {
> > >>                        ++p;
> > >>                        --len;
> > >>                } else if (strchr("^.[]$*", p[0]))
> > >>                        *t++ = '\\';
> > >>                *t++ = *p++;
> > >> +
> > >> +               if (len-- == 0)
> > >> +                       break;
> > >>        }
> > >>        if (lastdollar)
> > >>                *t++ = '$';

Reply | Threaded
Open this post in threaded view
|

Re: patch for vi seg-fault on CTRL-]

patrick keshishian
On 3/26/07, Otto Moerbeek <[hidden email]> wrote:

>
> On Mon, 26 Mar 2007, Thordur I. Bjornsson wrote:
>
> > patrick keshishian <[hidden email]> wrote on Mon 26.Mar'07 at 11:39:09 -0700
> >
> > Yub. I can reproduce this and the diff fixes it.
> > looks alright to me.
>
> To me the diff looks a bit awkward. My guess is the diff below is
> equivalent.  I still cannot reproduce the problem for some unknown
> reason, so please test this.


Yes, this patch is equivalent to the other. Can't reproduce the
problem with this patch.

Though, I think it is a bit misleading/confusing to have the check
for len > 0, when what we are really checking for is len != 0.  At
first glance, it implies that len could be negative at some point,
which is false.


Otto, what arch are you using to test this?  I'm using i386.

--patrick



>         -Otto
>
> Index: ex_subst.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 ex_subst.c
> --- ex_subst.c  22 Apr 2006 03:09:15 -0000      1.15
> +++ ex_subst.c  26 Mar 2007 20:31:15 -0000
> @@ -1228,6 +1228,8 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
>                 } else if (strchr("^.[]$*", p[0]))
>                         *t++ = '\\';
>                 *t++ = *p++;
> +               if (len == 0)
> +                       break;
>         }
>         if (lastdollar)
>                 *t++ = '$';
>
>
> >
> > > It is 100% reproducible with any macro pretty much that is multi-
> > > lined (i.e., the #define has a '\\' at the EOL).
> > >
> > > Sequence to reproduce it, assuming you already have tags file
> > > containing macro definitions:
> > > 1. Start up vi
> > > 2. Load "tags" file (if not implicitly loaded, e.g., tags file named
> > >   differently).
> > > 3. Search for maco (e.g., :tag CIRCQ_INSERT)
> > >
> > >
> > > HTH,
> > > --patrick
> > >
> > >
> > > >        -Otto
> > > >
> > > >>
> > > >> Best,
> > > >> --patrick
> > > >>
> > > >>
> > > >> Index: usr.bin/vi/ex/ex_subst.c
> > > >> ===================================================================
> > > >> RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> > > >> retrieving revision 1.15
> > > >> diff -u -p -r1.15 ex_subst.c
> > > >> --- usr.bin/vi/ex/ex_subst.c    22 Apr 2006 03:09:15 -0000      1.15
> > > >> +++ usr.bin/vi/ex/ex_subst.c    26 Mar 2007 09:45:56 -0000
> > > >> @@ -1221,13 +1221,16 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
> > > >>         * the backslashes ctags inserts when escaping the search
> > > >delimiter
> > > >>         * characters.
> > > >>         */
> > > >> -       for (; len > 0; --len) {
> > > >> +       for (; len != 0;) {
> > > >>                if (p[0] == '\\' && (p[1] == '/' || p[1] == '?')) {
> > > >>                        ++p;
> > > >>                        --len;
> > > >>                } else if (strchr("^.[]$*", p[0]))
> > > >>                        *t++ = '\\';
> > > >>                *t++ = *p++;
> > > >> +
> > > >> +               if (len-- == 0)
> > > >> +                       break;
> > > >>        }
> > > >>        if (lastdollar)
> > > >>                *t++ = '$';

Reply | Threaded
Open this post in threaded view
|

Re: patch for vi seg-fault on CTRL-]

Otto Moerbeek
On Mon, 26 Mar 2007, patrick keshishian wrote:

> On 3/26/07, Otto Moerbeek <[hidden email]> wrote:
> >
> > On Mon, 26 Mar 2007, Thordur I. Bjornsson wrote:
> >
> > > patrick keshishian <[hidden email]> wrote on Mon 26.Mar'07 at 11:39:09
> > -0700
> > >
> > > Yub. I can reproduce this and the diff fixes it.
> > > looks alright to me.
> >
> > To me the diff looks a bit awkward. My guess is the diff below is
> > equivalent.  I still cannot reproduce the problem for some unknown
> > reason, so please test this.
>
>
> Yes, this patch is equivalent to the other. Can't reproduce the
> problem with this patch.
>
> Though, I think it is a bit misleading/confusing to have the check
> for len > 0, when what we are really checking for is len != 0.  At
> first glance, it implies that len could be negative at some point,
> which is false.

len is unsigned, so it's equivalent. Given that, I prefer diffs that
introduce minimal changes, to make merging of new versions more easy.

> Otto, what arch are you using to test this?  I'm using i386.

With thib's instructions I am now able to reproduce the problem.
Dunno why my previous efforts failed.

        -Otto
       

>
> --patrick
>
>
>
> >         -Otto
> >
> > Index: ex_subst.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> > retrieving revision 1.15
> > diff -u -p -r1.15 ex_subst.c
> > --- ex_subst.c  22 Apr 2006 03:09:15 -0000      1.15
> > +++ ex_subst.c  26 Mar 2007 20:31:15 -0000
> > @@ -1228,6 +1228,8 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
> >                 } else if (strchr("^.[]$*", p[0]))
> >                         *t++ = '\\';
> >                 *t++ = *p++;
> > +               if (len == 0)
> > +                       break;
> >         }
> >         if (lastdollar)
> >                 *t++ = '$';
> >
> >
> > >
> > > > It is 100% reproducible with any macro pretty much that is multi-
> > > > lined (i.e., the #define has a '\\' at the EOL).
> > > >
> > > > Sequence to reproduce it, assuming you already have tags file
> > > > containing macro definitions:
> > > > 1. Start up vi
> > > > 2. Load "tags" file (if not implicitly loaded, e.g., tags file named
> > > >   differently).
> > > > 3. Search for maco (e.g., :tag CIRCQ_INSERT)
> > > >
> > > >
> > > > HTH,
> > > > --patrick
> > > >
> > > >
> > > > >        -Otto
> > > > >
> > > > >>
> > > > >> Best,
> > > > >> --patrick
> > > > >>
> > > > >>
> > > > >> Index: usr.bin/vi/ex/ex_subst.c
> > > > >> ===================================================================
> > > > >> RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> > > > >> retrieving revision 1.15
> > > > >> diff -u -p -r1.15 ex_subst.c
> > > > >> --- usr.bin/vi/ex/ex_subst.c    22 Apr 2006 03:09:15 -0000      1.15
> > > > >> +++ usr.bin/vi/ex/ex_subst.c    26 Mar 2007 09:45:56 -0000
> > > > >> @@ -1221,13 +1221,16 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
> > > > >>         * the backslashes ctags inserts when escaping the search
> > > > >delimiter
> > > > >>         * characters.
> > > > >>         */
> > > > >> -       for (; len > 0; --len) {
> > > > >> +       for (; len != 0;) {
> > > > >>                if (p[0] == '\\' && (p[1] == '/' || p[1] == '?')) {
> > > > >>                        ++p;
> > > > >>                        --len;
> > > > >>                } else if (strchr("^.[]$*", p[0]))
> > > > >>                        *t++ = '\\';
> > > > >>                *t++ = *p++;
> > > > >> +
> > > > >> +               if (len-- == 0)
> > > > >> +                       break;
> > > > >>        }
> > > > >>        if (lastdollar)
> > > > >>                *t++ = '$';

Reply | Threaded
Open this post in threaded view
|

Re: patch for vi seg-fault on CTRL-]

Karl Sjodahl - dunceor-2
On 3/27/07, Otto Moerbeek <[hidden email]> wrote:

> On Mon, 26 Mar 2007, patrick keshishian wrote:
>
> > On 3/26/07, Otto Moerbeek <[hidden email]> wrote:
> > >
> > > On Mon, 26 Mar 2007, Thordur I. Bjornsson wrote:
> > >
> > > > patrick keshishian <[hidden email]> wrote on Mon 26.Mar'07 at 11:39:09
> > > -0700
> > > >
> > > > Yub. I can reproduce this and the diff fixes it.
> > > > looks alright to me.
> > >
> > > To me the diff looks a bit awkward. My guess is the diff below is
> > > equivalent.  I still cannot reproduce the problem for some unknown
> > > reason, so please test this.
> >
> >
> > Yes, this patch is equivalent to the other. Can't reproduce the
> > problem with this patch.
> >
> > Though, I think it is a bit misleading/confusing to have the check
> > for len > 0, when what we are really checking for is len != 0.  At
> > first glance, it implies that len could be negative at some point,
> > which is false.
>
> len is unsigned, so it's equivalent. Given that, I prefer diffs that
> introduce minimal changes, to make merging of new versions more easy.
>
> > Otto, what arch are you using to test this?  I'm using i386.
>
> With thib's instructions I am now able to reproduce the problem.
> Dunno why my previous efforts failed.
>
>         -Otto
>
> >
> > --patrick
> >
> >
> >
> > >         -Otto
> > >
> > > Index: ex_subst.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> > > retrieving revision 1.15
> > > diff -u -p -r1.15 ex_subst.c
> > > --- ex_subst.c  22 Apr 2006 03:09:15 -0000      1.15
> > > +++ ex_subst.c  26 Mar 2007 20:31:15 -0000
> > > @@ -1228,6 +1228,8 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
> > >                 } else if (strchr("^.[]$*", p[0]))
> > >                         *t++ = '\\';
> > >                 *t++ = *p++;
> > > +               if (len == 0)
> > > +                       break;
> > >         }
> > >         if (lastdollar)
> > >                 *t++ = '$';
> > >
> > >
> > > >
> > > > > It is 100% reproducible with any macro pretty much that is multi-
> > > > > lined (i.e., the #define has a '\\' at the EOL).
> > > > >
> > > > > Sequence to reproduce it, assuming you already have tags file
> > > > > containing macro definitions:
> > > > > 1. Start up vi
> > > > > 2. Load "tags" file (if not implicitly loaded, e.g., tags file named
> > > > >   differently).
> > > > > 3. Search for maco (e.g., :tag CIRCQ_INSERT)
> > > > >
> > > > >
> > > > > HTH,
> > > > > --patrick
> > > > >
> > > > >
> > > > > >        -Otto
> > > > > >
> > > > > >>
> > > > > >> Best,
> > > > > >> --patrick
> > > > > >>
> > > > > >>
> > > > > >> Index: usr.bin/vi/ex/ex_subst.c
> > > > > >> ===================================================================
> > > > > >> RCS file: /cvs/src/usr.bin/vi/ex/ex_subst.c,v
> > > > > >> retrieving revision 1.15
> > > > > >> diff -u -p -r1.15 ex_subst.c
> > > > > >> --- usr.bin/vi/ex/ex_subst.c    22 Apr 2006 03:09:15 -0000      1.15
> > > > > >> +++ usr.bin/vi/ex/ex_subst.c    26 Mar 2007 09:45:56 -0000
> > > > > >> @@ -1221,13 +1221,16 @@ re_tag_conv(sp, ptrnp, plenp, replacedp)
> > > > > >>         * the backslashes ctags inserts when escaping the search
> > > > > >delimiter
> > > > > >>         * characters.
> > > > > >>         */
> > > > > >> -       for (; len > 0; --len) {
> > > > > >> +       for (; len != 0;) {
> > > > > >>                if (p[0] == '\\' && (p[1] == '/' || p[1] == '?')) {
> > > > > >>                        ++p;
> > > > > >>                        --len;
> > > > > >>                } else if (strchr("^.[]$*", p[0]))
> > > > > >>                        *t++ = '\\';
> > > > > >>                *t++ = *p++;
> > > > > >> +
> > > > > >> +               if (len-- == 0)
> > > > > >> +                       break;
> > > > > >>        }
> > > > > >>        if (lastdollar)
> > > > > >>                *t++ = '$';
>
>

I could reproduce it as well and both patch fixes the problem (or at
least can't reproduce the problem).

Dunceor