[PATCH] Fix bad free in connections

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

[PATCH] Fix bad free in connections

Steve Grubb
Hello,

There are times when b points to buf which is a stack variable. This
leads to a bad free. The current test actually guarantees the stack
will try to get freed. Simplest to just drop the variable and directly
test if b should get freed.


Signed-off-by: Steve Grubb <[hidden email]>


Index: src/main/connections.c
===================================================================
--- src/main/connections.c (revision 72935)
+++ src/main/connections.c (working copy)
@@ -421,7 +421,6 @@
     char buf[BUFSIZE], *b = buf;
     int res;
     const void *vmax = NULL; /* -Wall*/
-    int usedVasprintf = FALSE;
     va_list aq;
 
     va_copy(aq, ap);
@@ -434,7 +433,7 @@
     b = buf;
     buf[BUFSIZE-1] = '\0';
     warning(_("printing of extremely long output is truncated"));
- } else usedVasprintf = TRUE;
+ }
     }
 #else
     if(res >= BUFSIZE) { /* res is the desired output length */
@@ -481,7 +480,7 @@
     } else
  con->write(b, 1, res, con);
     if(vmax) vmaxset(vmax);
-    if(usedVasprintf) free(b);
+    if(b != buf) free(b);
     return res;
 }

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix bad free in connections

Martin Morgan-3
On 07/20/2017 05:04 PM, Steve Grubb wrote:

> Hello,
>
> There are times when b points to buf which is a stack variable. This
> leads to a bad free. The current test actually guarantees the stack
> will try to get freed. Simplest to just drop the variable and directly
> test if b should get freed.
>
>
> Signed-off-by: Steve Grubb <[hidden email]>
>
>
> Index: src/main/connections.c
> ===================================================================
> --- src/main/connections.c (revision 72935)
> +++ src/main/connections.c (working copy)
> @@ -421,7 +421,6 @@
>       char buf[BUFSIZE], *b = buf;
>       int res;
>       const void *vmax = NULL; /* -Wall*/
> -    int usedVasprintf = FALSE;
>       va_list aq;
>  
>       va_copy(aq, ap);
> @@ -434,7 +433,7 @@
>      b = buf;
>      buf[BUFSIZE-1] = '\0';
>      warning(_("printing of extremely long output is truncated"));
> - } else usedVasprintf = TRUE;
> + }
>       }
>   #else
>       if(res >= BUFSIZE) { /* res is the desired output length */
> @@ -481,7 +480,7 @@
>       } else
>   con->write(b, 1, res, con);
>       if(vmax) vmaxset(vmax);
> -    if(usedVasprintf) free(b);
> +    if(b != buf) free(b);

The code can be exercised with

   z = paste(rep("a", 11000), collapse="")
   f = fifo("foo", "w+")
   writeLines(z, f)

If the macro HAVE_VASPRINTF is not defined, then b is the result of
R_alloc(), and it is not appropriate to free(b).

If the macro is defined we go through

        res = vasprintf(&b, format, ap);
        if (res < 0) {
            b = buf;
            buf[BUFSIZE-1] = '\0';
            warning(_("printing of extremely long output is truncated"));
        } else usedVasprintf = TRUE;

b gets reallocated when

     res = vasprintf(&b, format, ap);

is successful and res >= 0. usedVasprintf is then set to TRUE, and
free(b) called.

It seems like the code is correct as written?

Martin Morgan (the real other Martin M*)

>       return res;
>   }
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>


This email message may contain legally privileged and/or...{{dropped:2}}

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix bad free in connections

Steve Grubb
On Friday, July 21, 2017 5:03:09 AM EDT Martin Morgan wrote:
> b gets reallocated when
>
>      res = vasprintf(&b, format, ap);
>
> is successful and res >= 0. usedVasprintf is then set to TRUE, and
> free(b) called.
>
> It seems like the code is correct as written?

Yes, I think I see the issue. vasprintf seems to not be tracked for memory
allocation. So, yes I agree its correct as written. Sorry for the noise.

-Steve

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel