mg(1) segfault

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

mg(1) segfault

Gleydson Soares-3

mg(1) segfault.
it is triggered as follows:

1- echo "(start-kbd-macro)" >> $HOME/.mg
2- open mg and type twice C-x (

find below the backtrace and a patch to fix.
OK?

Program received signal SIGBUS, Bus error.
definemacro (f=Variable "f" is not available.
) at macro.c:43
43                              lp2 = lp1->l_fp;
(gdb) backtrace
#0  definemacro (f=Variable "f" is not available.
) at macro.c:43
#1  0x0000038cecf15606 in doin () at kbd.c:158
#2  0x0000038cecf16d4b in main (argc=Variable "argc" is not available.
) at main.c:188
(gdb)


? mg
? mg_segfault.diff
Index: macro.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/macro.c,v
retrieving revision 1.16
diff -u -p -r1.16 macro.c
--- macro.c 19 Mar 2015 21:22:15 -0000 1.16
+++ macro.c 4 Apr 2015 13:45:15 -0000
@@ -38,7 +38,7 @@ definemacro(int f, int n)
  }
 
  /* free lines allocated for string arguments */
- if (maclhead != NULL) {
+ if (macrodef && maclhead != NULL) {
  for (lp1 = maclhead->l_fp; lp1 != maclhead; lp1 = lp2) {
  lp2 = lp1->l_fp;
  free(lp1);
Reply | Threaded
Open this post in threaded view
|

Re: mg(1) segfault

Florian Obser-2
On Sat, Apr 04, 2015 at 10:48:15AM -0300, Gleydson Soares wrote:

>
> mg(1) segfault.
> it is triggered as follows:
>
> 1- echo "(start-kbd-macro)" >> $HOME/.mg
> 2- open mg and type twice C-x (
>
> find below the backtrace and a patch to fix.
> OK?
>
> Program received signal SIGBUS, Bus error.
> definemacro (f=Variable "f" is not available.
> ) at macro.c:43
> 43                              lp2 = lp1->l_fp;
> (gdb) backtrace
> #0  definemacro (f=Variable "f" is not available.
> ) at macro.c:43
> #1  0x0000038cecf15606 in doin () at kbd.c:158
> #2  0x0000038cecf16d4b in main (argc=Variable "argc" is not available.
> ) at main.c:188
> (gdb)
>

> ? mg
> ? mg_segfault.diff
> Index: macro.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mg/macro.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 macro.c
> --- macro.c 19 Mar 2015 21:22:15 -0000 1.16
> +++ macro.c 4 Apr 2015 13:45:15 -0000
> @@ -38,7 +38,7 @@ definemacro(int f, int n)
>   }
>  
>   /* free lines allocated for string arguments */
> - if (maclhead != NULL) {
> + if (macrodef && maclhead != NULL) {
>   for (lp1 = maclhead->l_fp; lp1 != maclhead; lp1 = lp2) {
>   lp2 = lp1->l_fp;
>   free(lp1);

No, this is obviously not correct. macrodef can't be true here, ever.
Some lines above this we have:
        if (macrodef) {
                ewprintf("already defining macro");
                return (macrodef = FALSE);
        }

--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: mg(1) segfault

Gleydson Soares-3
>                 return (macrodef = FALSE);

but we shouldn't change macrodef here.


? mg
? mg_segfault.diff
? v2_mg_segfault.diff
Index: macro.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/macro.c,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 macro.c
--- macro.c 19 Mar 2015 21:22:15 -0000 1.16
+++ macro.c 4 Apr 2015 16:09:38 -0000
@@ -34,11 +34,11 @@ definemacro(int f, int n)
 
  if (macrodef) {
  ewprintf("already defining macro");
- return (macrodef = FALSE);
+ return (FALSE);
  }
 
  /* free lines allocated for string arguments */
- if (maclhead != NULL) {
+ if (macrodef && maclhead != NULL) {
  for (lp1 = maclhead->l_fp; lp1 != maclhead; lp1 = lp2) {
  lp2 = lp1->l_fp;
  free(lp1);
Reply | Threaded
Open this post in threaded view
|

Re: mg(1) segfault

Steven McDonald
On Sat, 4 Apr 2015 15:23:45 -0300
Gleydson Soares <[hidden email]> wrote:

> but we shouldn't change macrodef here.

Regardless of whether macrodef is being changed, you're still returning
if macrodef is true, so there is no way for it to be true at the point
where you're adding a test for it.

Reply | Threaded
Open this post in threaded view
|

Re: mg(1) segfault

Florian Obser-2
In reply to this post by Gleydson Soares-3
On Sat, Apr 04, 2015 at 03:23:45PM -0300, Gleydson Soares wrote:
> >                 return (macrodef = FALSE);
>
> but we shouldn't change macrodef here.
>

I hate the startup file.
Look, this is a use after free, but I can't find it...

#0  0x00001b9de0b1b77f in definemacro (f=0, n=1)
    at /usr/src/usr.bin/mg/macro.c:43
43                              lp2 = lp1->l_fp;
(gdb) p *maclhead
$1 = {l_fp = 0xdfdfdfdfdfdfdfdf, l_bp = 0xdfdfdfdfdfdfdfdf,
  l_size = -538976289, l_used = -538976289,
  l_text = 0xdfdfdfdfdfdfdfdf <Address 0xdfdfdfdfdfdfdfdf out of bounds>}

also: what Steven McDonald says

> ? mg
> ? mg_segfault.diff
> ? v2_mg_segfault.diff
> Index: macro.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mg/macro.c,v
> retrieving revision 1.16
> diff -u -p -u -p -r1.16 macro.c
> --- macro.c 19 Mar 2015 21:22:15 -0000 1.16
> +++ macro.c 4 Apr 2015 16:09:38 -0000
> @@ -34,11 +34,11 @@ definemacro(int f, int n)
>  
>   if (macrodef) {
>   ewprintf("already defining macro");
> - return (macrodef = FALSE);
> + return (FALSE);
>   }
>  
>   /* free lines allocated for string arguments */
> - if (maclhead != NULL) {
> + if (macrodef && maclhead != NULL) {
>   for (lp1 = maclhead->l_fp; lp1 != maclhead; lp1 = lp2) {
>   lp2 = lp1->l_fp;
>   free(lp1);


--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: mg(1) segfault

Gleydson Soares-3
> I hate the startup file.
> Look, this is a use after free, but I can't find it...
>
> #0  0x00001b9de0b1b77f in definemacro (f=0, n=1)
>     at /usr/src/usr.bin/mg/macro.c:43
> 43                              lp2 = lp1->l_fp;
> (gdb) p *maclhead
> $1 = {l_fp = 0xdfdfdfdfdfdfdfdf, l_bp = 0xdfdfdfdfdfdfdfdf,
>   l_size = -538976289, l_used = -538976289,
>   l_text = 0xdfdfdfdfdfdfdfdf <Address 0xdfdfdfdfdfdfdfdf out of bounds>}

seems that it is in excline(), look:

src/usr.bin/mg/extend.c:907
        lp = maclcur->l_fp;
        while (lp != maclcur) {
                np = lp->l_fp;
                free(lp);
                lp = np;
        }
        free(lp);
        return (status);

excline() loads .mg file and free(lp) lines afterwards.

following diff add a cleanline check to make sure that the cleanup was already done or not.
avoid user after free in definemacro()/macro.c:45 in cases where excline() take care of the free lines cleanup.


? mg
Index: extend.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/extend.c,v
retrieving revision 1.61
diff -u -p -u -p -r1.61 extend.c
--- extend.c 24 Mar 2015 22:28:10 -0000 1.61
+++ extend.c 11 Apr 2015 04:41:38 -0000
@@ -910,6 +910,7 @@ cleanup:
  free(lp);
  lp = np;
  }
+ cleanline = 1;
  free(lp);
  return (status);
 }
Index: macro.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/macro.c,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 macro.c
--- macro.c 19 Mar 2015 21:22:15 -0000 1.16
+++ macro.c 11 Apr 2015 04:41:38 -0000
@@ -15,6 +15,7 @@
 #include "key.h"
 #include "macro.h"
 
+int cleanline = 0;
 int inmacro = FALSE; /* Macro playback in progess */
 int macrodef = FALSE; /* Macro recording in progress */
 int macrocount = 0;
@@ -38,7 +39,7 @@ definemacro(int f, int n)
  }
 
  /* free lines allocated for string arguments */
- if (maclhead != NULL) {
+ if (!cleanline && maclhead != NULL) {
  for (lp1 = maclhead->l_fp; lp1 != maclhead; lp1 = lp2) {
  lp2 = lp1->l_fp;
  free(lp1);
Index: macro.h
===================================================================
RCS file: /cvs/src/usr.bin/mg/macro.h,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 macro.h
--- macro.h 18 Nov 2005 20:56:53 -0000 1.7
+++ macro.h 11 Apr 2015 04:41:38 -0000
@@ -6,6 +6,7 @@
 
 #define MAXMACRO 256 /* maximum functs in a macro */
 
+extern int cleanline;
 extern int inmacro;
 extern int macrodef;
 extern int macrocount;