[Home]

Summary:ASTERISK-22467: [patch] memory leaks 1.8+
Reporter:Corey Farrell (coreyfarrell)Labels:
Date Opened:2013-09-05 02:14:56Date Closed:2013-10-24 15:34:03
Priority:MinorRegression?
Status:Closed/CompleteComponents:Applications/app_voicemail Channels/chan_dahdi Channels/chan_sip/General Codecs/codec_ilbc Core/General Core/Jitterbuffer Functions/func_math Tests/General
Versions:SVN 1.8.24.0 11.6.0 12.0.0-alpha1 Frequency of
Occurrence
Related
Issues:
is duplicated byASTERISK-23091 possible memory leak in abstract jitterbuffer
Environment:Attachments:( 0) app_voicemail-1.8.patch
( 1) app_voicemail-11up.patch
( 2) ast_app_parse_timelen-fail-zero-length.patch
( 3) ast_app_parse_timelen-initialize-vars.patch
( 4) astobj2-clean-debug-cli-1.8-11.patch
( 5) astobj2-clean-debug-cli-12up.patch
( 6) chan_dahdi-cleanup_push.patch
( 7) chan_dahdi-free_pfds.patch
( 8) chan_sip-parse_contact_header_test-free-contacts.patch
( 9) clicompat.patch
(10) clicompat-r2.patch
(11) cli-filename-completion-leak.patch
(12) codecs-ilbc-doCPLC.patch
(13) data-cleanup-test-registration.patch
(14) func_math.patch
(15) jitterbuf-jb_reset-leak-1.8.patch
(16) jitterbuf-jb_reset-leak-11up.patch
(17) main-asterisk-kill-listener.patch
(18) main-test-cleanup.patch
(19) main-utils-1.8.patch
(20) main-utils-11.patch
(21) main-utils-12up.patch
(22) test_dlinklists.patch
(23) test_linkedlists-1.8.patch
(24) test_linkedlists-11up.patch
Description:Patches for memory leaks found with 'test execute all'.  Compile/run tests done for 1.8/11.  Compile only test for 12, for trunk I only verified that the patches applied.

channels/sip/reqresp_parser.c: parse_contact_header_test leaks contacts
channels/chan_dahdi.c: pfds from do_monitor() leaks
codecs/ilbc/doCPLC.c: doThePLC() possible uninitialized use of max_per (reported as error by gcc with dev-mode and codec's embedded)
funcs/func_math.c: test_MATH_function leaks expr + result
main/editline/readline.c: filename completion leaks filename/dirname
main/app.c: ast_app_parse_timelen() can use 'amount' uninitialized (reported by valgrind)
main/asterisk.c: listener thread not joined, sig_alert_pipe not closed
main/astobj2.c: AO2_DEBUG/TEST_FRAMEWORK cli commands not unregistered
main/data.c: test not unregistered
main/test.c: cli commands not unregistered
tests/test_dlinklist.c: dll_tests leaks test container and record "b"

apps/app_voicemail.c:  result of find_user is leaked in some places
-- test_voicemail_msgcount (all versions)
-- acf_vm_info in (11+).
main/jitterbuffer.c:
-- jb_reset() causes frame cache (jb->free) to leak.
-- switch jb_new to ast_calloc to prevent uninitialized field access during first jb_reset.
main/utils.c:
-- DEBUG_THREADS cli commands not unregistered (all versions)
-- dev_urandom_fd is not closed (11+)
-- handle_show_locks leaks ast_str_create (1.8/11)
tests/test_linkedlist.c:
-- single_ll_tests leaks buf (all versions)
-- double_ll_tests leaks buf (11+)
Comments:By: Corey Farrell (coreyfarrell) 2013-09-05 02:31:57.887-0500

I believe app_voicemail.c foward_message() can leak results of find_user in all current versions.  I haven't had time to follow the breaks and returns of that procedure or patch it.
By: Corey Farrell (coreyfarrell) 2013-09-05 22:35:39.779-0500

utils/clicompat.c did not have ast_cli_unregister_multiple, prevented linking of utils that use astobj2.  [^clicompat.patch] is for all active branches.
By: Jonathan Rose (jrose) 2013-10-18 16:39:43.264-0500

{{checklist}}
{{---------}}
{color:green}-chan_sip-parse_contact_header_test-free-contacts.patch - committed-{color}
{color:green}-cli-filename-completion-leak.patch - committed-{color}
{color:green}-func_math.patch - committed-{color}
{color:green}-main-test-cleanup.patch - committed-{color}
{color:green}-test_dlinklists.patch - committed-{color}
{color:green}-ast_app_parse_timelen-fail-zero-length.patch - committed replacement-{color}
{color:green}-chan_dahdi-cleanup_push.patch - committed-{color}
{color:green}-clicompat-r2.patch - committed-{color}
{color:green}-codecs-ilbc-doCPLC.patch - committed-{color}
{color:green}-data-cleanup-test-registration.patch - committed-{color}
{color:green}-main-asterisk-kill-listener.patch - committed-{color}
{color:green}-app_voicemail-1.8.patch - committed-{color}
{color:green}-app_voicemail-11up.patch - committed-{color}
{color:green}-astobj2-clean-debug-cli-1.8-11.patch - committed-{color}
{color:green}-astobj2-clean-debug-cli-12up.patch - committed-{color}
{color:green}-jitterbuf-jb_reset-leak-1.8.patch - committed-{color}
{color:green}-jitterbuf-jb_reset-leak-11up.patch - committed-{color}
{color:green}-test_linkedlists-1.8.patch - committed-{color}
{color:green}-test_linkedlists-11up.patch - committed-{color}
{color:green}-main-utils-1.8.patch - committed-{color}
{color:green}-main-utils-11.patch - committed-{color}
{color:green}-main-utils-12up.patch - committed-{color}

By: Corey Farrell (coreyfarrell) 2013-10-18 17:56:04.385-0500

amount is used uninitialized in ast_app_parse_timelen if timestr is not NULL but is zero length.  This is because sscanf returns EOF (-1 on my system) without setting &amount.  I think returning an error in this case would cause problems.
By: Jonathan Rose (jrose) 2013-10-18 18:06:00.259-0500

Ah, I see what you mean. At the same time though, I think the amount parameter should really be included in the time string.  If it isn't, we should fail.  Why do you think returning an error in this case would cause problems?  If there is a consumer that is relying on it not returning an error, then we should probably fix the consumer.
By: Corey Farrell (coreyfarrell) 2013-10-18 18:21:46.941-0500

Now that I look at it again it looks like returning an error shouldn't cause an issue in 1.8 or 11.  I don't fully remember but I think I found this issue by using Wait() or WaitExten() with blank parameter.  It appears that neither app will return error if ast_app_parse_timelen fails, I was worried about this causing a hangup.
By: Corey Farrell (coreyfarrell) 2013-10-22 23:34:06.575-0500

I've attached an updated patch for ast_app_parse_timelen that returns error on zero length input.
By: Jonathan Rose (jrose) 2013-10-23 10:59:48.693-0500

So as far as I can tell that will do the job just fine, but I just find myself wondering...
why do that instead of just checking the return value of sscanf for EOF as well as 0? It seems like
that would probably be computationally faster and deal with any other problems that we might be
overlooking.

I could go ahead and fix the patch to do just that since I'm sure you don't really care to write
another small and insignificant patch like that... I'm just curious if you have a particular reason
for favoring this approach.
By: Corey Farrell (coreyfarrell) 2013-10-23 11:55:25.264-0500

I was approaching ast_app_parse_timelen from the view of the error (blank string), so I wrote the code to catch the error as early as possible.  If you want to change the patch to catch EOF from sscanf instead I'm indifferent.
By: Corey Farrell (coreyfarrell) 2013-10-23 12:12:11.390-0500

I now have another concern about chan_dahdi-free_pfds.patch.  chan_dahdi regularly calls restart_monitor.  Without my patch a new pfds is allocated each time do_monitor restarts, with my patch it persists between restarts.
By: Corey Farrell (coreyfarrell) 2013-10-23 14:32:07.140-0500

main-asterisk-kill-listener.patch - pthread_kill(lthread, SIGURG) doesn't kill the thread, it just interrupts ast_poll (which has no timeout).  Without this the listener thread never returns from ast_poll.

Odd, I'm now noticing that poll() is listed as a pthreads cancellation point, so I'm not sure why (if) this kill is needed.  I think I sent SIGURG since many other thread shutdown calls do this before joining.
By: Richard Mudgett (rmudgett) 2013-10-23 15:17:57.047-0500

Hmm.  I think poll() may be a recent addition to the cancellation points list.
By: Jonathan Rose (jrose) 2013-10-23 15:22:07.872-0500

Yeah, I've been reading through the man pages on pthread_join/kill/cancel/etc (I don't work with thread termination stuff like this often) and you are right about that. According to Richard using pthread_kill to issue SIGURG is appropriate because it will ensure the active poll() gets broken.
By: Corey Farrell (coreyfarrell) 2013-10-24 00:07:04.976-0500

clicompat.c also needs a stub for ast_register_atexit.  clicompat-r2.patch includes this.
By: Corey Farrell (coreyfarrell) 2013-10-24 09:49:06.950-0500

chan_dahdi-cleanup_push.patch - this is the first time I've ever used pthread_cleanup_push but this should be the proper way to handle cleanup of the variable.  Verified with valgrind this resolves the leak.
By: Jonathan Rose (jrose) 2013-10-24 11:04:05.928-0500

That looks good.

EDIT: Just one left
By: Jonathan Rose (jrose) 2013-10-24 15:35:55.830-0500

Yay, all done! Thanks for all these patches Corey.
By: Jonathan Rose (jrose) 2013-10-25 11:10:47.553-0500

Ugh, minor setback.  Turns out clicompat-r2.patch breaks the build...
Basically what happens is ast_register_atexit gets defined twice, once by utils/clicompat.c and once by utils/recounter.c
https://bamboo.asterisk.org/bamboo/browse/AST-ATRUNKBUILD4-244

Since it's not particularly memory leak related, we should probably address it in a separate issue from this point.

EDIT: Oh frustrating.  Looks like some of it was needed on account of other changes. Well, I'll keep poking at it for now.
By: Corey Farrell (coreyfarrell) 2013-10-25 12:03:40.691-0500

Looks like ast_register_atexit was defined in utils/refcounter.c on 12/trunk only?  I think remove it from there since the one I added to clicompat.c will cover the needs of utils/*.c.  Sorry I'm out the rest of the day if this is still unresolved tomorrow I can take a look.
By: Matt Jordan (mjordan) 2013-10-25 12:07:43.913-0500

Agree with Corey - I'd simply remove it from refcounter on 12/trunk and put it where it belongs.
By: Jonathan Rose (jrose) 2013-10-25 12:32:34.577-0500

Alrighty, it's taken care of.