[Home]

Summary:ASTERISK-06555: [patch] Bug and bugfix for ast_frisolate
Reporter:stefankroon (stefankroon)Labels:
Date Opened:2006-03-15 10:22:16.000-0600Date Closed:2006-05-05 21:33:12
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) ast_frisolate_bugfix.patch
Description:I found a bug in the function ast_frisolate. The bug applies when the frame-header is malloced and the data-pointer isn't (so in the case mallocd == 1). In that case the data in the frame is not properly copied (by memcpy).

In the case the following branch is executed:
 } else {
   out = fr;
 }

And this branch is executed:

if (!(fr->mallocd & AST_MALLOCD_DATA))  {
if (!(out->data = ast_malloc(fr->datalen + ...))
...
memcpy(out->data, fr->data, fr->datalen);
}

The point is that by executing `out = fr`. The two pointers are equal. Then the pointer `out->data` is overwritten with new malloced data. Afterwards there is a copy from `fr->data` to `out->data`, but the fr->data pointer has been replaced by the earlier statement. So the the pointers given to the memcpy-function are actually the same.

Find attached my patch for this bug.

****** ADDITIONAL INFORMATION ******

I found out that the frisolate function isn't used very much, so I report this bug as a minor.
Comments:By: Russell Bryant (russell) 2006-03-16 11:55:12.000-0600

Thank you for reporting this bug!  However, to look at your patch, we will need you to file a disclaimer.  However, I can probably produce a patch on my own just from your description if you do not wish to do so ...
By: Olle Johansson (oej) 2006-03-17 04:18:14.000-0600

Reminder of a disclaimer :-)
By: Roy Sigurd Karlsbakk (rkarlsba) 2006-03-17 06:32:39.000-0600

this fixed the leak with the sip jitterbuffer :D
thanks a lot
By: stefankroon (stefankroon) 2006-03-17 07:21:46.000-0600

If you can wait for the disclaimer till next week, then wait. I have no fax arround here right now. Else apply your own bugfix.
By: stefankroon (stefankroon) 2006-03-17 07:24:18.000-0600

Dear rkarlsba,

Grep-ping through the source code only delivered the following occurences of ast_frisolate:
channels/chan_vpb.c:      *p->bridge->fo = ast_frisolate(&f);
formats/format_jpeg.c:    return ast_frisolate(&fr);
frame.c:                  struct ast_frame *ast_frisolate(struct ast_frame *fr)
frame.c:                        return ast_frisolate(f);
include/asterisk/frame.h: struct ast_frame *ast_frisolate(struct ast_frame *fr);

The line with 'return ast_frisolate(f);' in the file frame.c is commented out. As far as I can see the function is only used in format_jpeg and chan_vbp. I can't see any SIP jitterbuffer that is using the function. Can you explain where it is used?
By: puzzled (puzzled) 2006-03-17 08:57:08.000-0600

Check the jitterbuffer-1.2 branch and do another grep. Results below. svn checkout http://svn.digium.com/svn/asterisk/team/oej/jitterbuffer-1.2 jitterbuffer-1.2

$ grep -r ast_frisolate ./*
./abstract_jb.c:                frr = ast_frisolate(f);
./channels/chan_vpb.c:                                  *p->bridge->fo = ast_frisolate(&f);
./formats/format_jpeg.c:        return ast_frisolate(&fr);
./frame.c:struct ast_frame *ast_frisolate(struct ast_frame *fr)
./frame.c:      return ast_frisolate(f);
./include/asterisk/frame.h:struct ast_frame *ast_frisolate(struct ast_frame *fr);
By: stefankroon (stefankroon) 2006-03-17 13:07:35.000-0600

I just send a disclaimer by fax. Did they receive it at digium. At least now they can use my patch (if they still need it).

Stefan
By: Olle Johansson (oej) 2006-03-17 16:21:40.000-0600

Stefan,
Thanks for this patch. It really seem like a very important fix for the new SIP jitterbuffer in the jitterbuffer branch!

Trevlig helg!

/Olle
By: Roy Sigurd Karlsbakk (rkarlsba) 2006-03-18 04:26:56.000-0600

the jb leak was _not_ fixed :P
the mrtg process hung.....
By: Serge Vecher (serge-v) 2006-05-04 11:02:47

rkarlsba: can you provide any feedback/debug in order to improve the patch here?
By: Russell Bryant (russell) 2006-05-05 21:32:44

this has been fixed in the 1.2 branch and the trunk in revisions 25160 and 25164, thanks!