[asterisk-dev] Is this a bug in frame.c?

[Russell Bryant]

----- Russell Bryant <russell at digium.com> wrote:
> ----- Slav Klenov <slav at securax.org> wrote:
> > I think theres still a problem when the frame header isn't allocated
> > and
> > the data allocation fails. Shouldn't we have:
> >
> >     if (!(fr->mallocd & AST_MALLOCD_DATA))  {
> >         if (!(newdata = ast_malloc(fr->datalen +
> > AST_FRIENDLY_OFFSET))) {
> >             if(out != fr)
> >                 free(out);
> >             return NULL;
> >         }
> >         newdata += AST_FRIENDLY_OFFSET;
> >         ...
> >
> > instead?
> 
> At this point in the code, the frame header will always be allocatted.
>  However, I think it would still be appropriate to change free(out);
> to be ast_frfree(out); because the current code is a memory leak.  As
> it stands the frame header gets free'd, but none of its other members
> do.

Nevermind, I see what you are saying now.  The code should not be freeing anything from the original frame, only the new parts being allocated in this function.  Yes, that makes sense.  See this patch for 1.2:

Index: frame.c
===================================================================
--- frame.c     (revision 32817)
+++ frame.c     (working copy)
@@ -324,15 +324,24 @@
                out = fr;

        if (!(fr->mallocd & AST_MALLOCD_SRC)) {
-               if (fr->src)
-                       out->src = strdup(fr->src);
+               if (fr->src) {
+                       if (!(out->src = strdup(fr->src))) {
+                               if (fr != out)
+                                       free(out);
+                               ast_log(LOG_ERROR, "Memory allocation error\n");
+                               return NULL;
+                       }
+               }
        } else
                out->src = fr->src;

        if (!(fr->mallocd & AST_MALLOCD_DATA))  {
                newdata = malloc(fr->datalen + AST_FRIENDLY_OFFSET);
                if (!newdata) {
-                       free(out);
+                       if (fr->src != out->src)
+                               free((char *) out->src);
+                       if (fr != out)
+                               free(out);
                        ast_log(LOG_WARNING, "Out of memory\n");
                        return NULL;
                }



and for trunk ...

Index: frame.c
===================================================================
--- frame.c     (revision 32919)
+++ frame.c     (working copy)
@@ -328,14 +328,21 @@
                out = fr;

        if (!(fr->mallocd & AST_MALLOCD_SRC)) {
-               if (fr->src)
-                       out->src = strdup(fr->src);
+               if (fr->src) {
+                       if (!(out->src = ast_strdup(fr->src))) {
+                               if (out != fr)
+                                       free(out);
+                       }
+               }
        } else
                out->src = fr->src;

        if (!(fr->mallocd & AST_MALLOCD_DATA))  {
                if (!(newdata = ast_malloc(fr->datalen + AST_FRIENDLY_OFFSET))) {
-                       free(out);
+                       if (fr->src != out->src)
+                               free((char *) out->src);
+                       if (fr != out)
+                               free(out);
                        return NULL;
                }
                newdata += AST_FRIENDLY_OFFSET;



-- 
Russell Bryant
Software Developer
Digium, Inc.