Review Board 1.7.16

Fix a variety of memory leaks

Review Request #1922 - Created May 15, 2012 and submitted

Matt Jordan
opticron, otherwiseguy, rmudgett
This fixes a number of memory leaks in core modules (and a few modules that are extended support, but were easy to fix) that were found by a static analysis tool.  A brief summary of the changes:

* app_minivm: free ast_str objects on off nominal paths
* app_page: free the ast_dial object if the requested channel technology cannot be appended to the dialing structure
* app_queue: if a penalty rule failed to match any existing rule list names, the created rule would not be inserted and its memory would be leaked
* app_read: dispose of the created silence detector in the presence of off nominal circumstances
* app_voicemail: dispose of an allocated unique ID field for MWI event un-subscribe requests in off nominal paths; dispose of configuration objects when using the secret.conf option
* chan_dahdi: dispose of the allocated frame produced by ast_dsp_process
* chan_iax2: properly unref peer in CLI command "iax2 unregister"
* chan_sip: dispose of the allocated frame produced by sip_rtp_read's call of ast_dsp_process; free memory in parse unit tests
* func_dialgroup: properly deref ao2 object grhead in nominal path of dialgroup_read
* func_odbc: free resultset in off nominal paths of odbc_read
* cli: free match_list in off nominal paths of CLI match completion
* config: free comment_buffer/list_buffer when configuration file load is unchanged; free the same buffers any time they were created and config files were processed
* data: free XML nodes in various places
* enum: free context buffer in off nominal paths
* features: free ast_call_feature in off nominal paths of applicationmap config processing
* netsock2: users of ast_sockaddr_resolve pass in an ast_sockaddr struct that is allocated by the method.  Failures in ast_sockaddr_resolve could result in the users of the method not knowing whether or not the buffer was allocated.  The method will now not allocate the ast_sockaddr struct if it will return failure.
* pbx: cleanup hash table traversals in off nominal paths; free ignore pattern buffer if it already exists for the specified context
* xmldoc: cleanup various nodes when we no longer need them
* main/editline: various cleanup of pointers not being freed before being assigned to other memory, cleanup along off nominal paths
* menuselect/mxml: cleanup of value buffer for an attribute when that attribute did not specify a value
* res_calendar*: responses are allocated via the various *_request method returns and should not be allocated in the various write_event methods; ensure attendee buffer is freed if no data exists in the parsed node; ensure that calendar objects are de-ref'd appropriately
* res_jabber: free buffer in off nominal path
* res_musiconhold: close the DIR* object in off nominal paths
* res_rtp_asterisk: if we run out of ports, close the rtp socket object and free the rtp object
* res_srtp: if we fail to create the session in libsrtp, destroy the temporary ast_srtp object

Review request changed
Updated (May 18, 2012, 3:41 a.m.)
Addressed Mark's findings.
Ship it!
Posted (May 18, 2012, 3:48 a.m.)
There are some leaks you're fixing where under current conditions, they cannot occur. However, the fixes you're putting in place are forward-looking and of course shut the static analyzer up. So go for it!
Ship it!
Posted (May 18, 2012, 4 a.m.)


/branches/1.8/channels/chan_dahdi.c (Diff revision 2)
There is one more of these to watch out for when forward porting to trunk (and probably 10 as well).
  1. Found it in trunk, will fix it in a subsequent patch.
/branches/1.8/channels/chan_sip.c (Diff revision 2)
Since we're in this area of code and fixing this type of problem, this probably needs a ast_frfree.
  1. Hm.  It might, although that wasn't caught by the tool.  The static analysis tool caught the fact that ast_dsp_process can create a new frame and that we weren't disposing of it properly.
    Since calling ast_frfree shouldn't cause any harm if we are indeed done with the frame, I'll go ahead and add that to sip_rtp_read in those cases where we return an explicit null frame and ignore the frame pointed to by f. 
/branches/1.8/channels/chan_sip.c (Diff revision 2)
  1. Fixed
/branches/1.8/channels/chan_sip.c (Diff revision 2)
Random whitespace change?
  1. I wouldn't say random.
/branches/1.8/funcs/func_odbc.c (Diff revision 2)
This needs to be freed at the end of the function.
  1. I don't think so.  If row is allocated, it gets added to the end of the resultset's list, so we don't want to free it - resultset gets put into the channel's datastore and is used later.
    However, this does raise a problem: we can't just free resultset directly.  Once rows are assigned to a resultset we have to use the destructor function odbc_datastore_free, otherwise we will leave the memory allocated for the rows orphaned.
    Note that there is a code path where we may want to dispose of a resultset and it may be NULL.  If OPT_MULTIROW is not set on the query, and the rowlimit is equal to or less then 1, a resultset object will not be created.  In that case, if a query fails, we may try to dispose of a resultset that is still NULL - so rather then add a bunch of checks throughout acf_odbc_read, the destructor function now simply returns if the pointer passed to it is NULL.
/branches/1.8/main/cli.c (Diff revision 2)
retstr needs to be freed here.
  1. Good catch, done. runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to