[asterisk-dev] [Code Review] 4464: testsuite: Increase timeout for Asterisk shutdown
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4464/ --- Review request for Asterisk Developers. Repository: testsuite Description --- I've run into issues lately in Asterisk 13+ where 5 seconds is not long enough to complete the Asterisk shutdown, causing it to be killed. This results in many false failures with REF_DEBUG enabled. I feel the best way to handle this is to always increase the timeout to 10 seconds. I realize that REF_DEBUG is the reason it's taking longer, but it's also the reason that dozens of these timeouts have been resolved. If needed I can find a way to check for the existance of a refs log and double the timeout if found, but I don't think 10 seconds is an unreasonable. It's tough to tell but I think this issue effects around 30 tests in 13/trunk (approximate number of tests with unstable results). Once this is committed it'll be easier to identify and resolve real shutdown timeouts - where Asterisk wouldn't shutdown no matter how long you wait. Diffs - /asterisk/trunk/lib/python/asterisk/asterisk.py 6482 Diff: https://reviewboard.asterisk.org/r/4464/diff/ Testing --- No more shutdown timeouts from tests capable of graceful shutdown. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4108: Weak Proxy Objects
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4108/ --- (Updated March 4, 2015, 4:43 p.m.) Review request for Asterisk Developers, George Joseph and rmudgett. Changes --- I never much liked the API naming I originally chose, but was initially focused on getting this thread safe. I like Richard's idea that this is a weakproxy, so I've changed the code to use ao2_weakproxy as the namespace. The only exception being ao2_get_weakproxy, as this is run against normal AO2 objects. I've also made all ao2_weakproxy functions operate on weakproxy's only. These functions now reject normal AO2 objects where weakproxy is expected. This change also makes it possible to efficiently use weakproxy's in containers, as the weakproxy can contain it's own fields. For weakproxy that may be put into containers, you would put the fields normally required for sort_fn, hash_fn and cmp_fn into the weakproxy. One thing I don't like about the current API is the need to have 'struct ao2_weakproxy' as the first field of weakproxies. I know this could be avoided by moving the field into 'struct astobj2', I'm just not sure extra storage for all AO2 objects is acceptable. It could probably be done using another private structure (such as 'struct astobj2_lock'), but I'm not sure this is worth the added complexity to AO2. I plan to add REF_DEBUG variants of some functions, I want to hold off on that until the API gets approval. Summary (updated) - Weak Proxy Objects Repository: Asterisk Description (updated) --- This implements weak references. The weakproxy object is a real ao2 with normal reference counting of its own. When a weakproxy is pointed to a normal object they hold references to each other. The normal object is automatically freed when a single reference remains (the weakproxy). The weakproxy also supports subscriptions that will notify callbacks when the normal object is about to be destroyed. Diffs (updated) - /trunk/tests/test_astobj2_weaken.c PRE-CREATION /trunk/main/astobj2.c 432445 /trunk/include/asterisk/astobj2.h 432445 Diff: https://reviewboard.asterisk.org/r/4108/diff/ Testing (updated) --- Ran the included test with REF_DEBUG enabled under valgrind. No reference leaks or improper memory access. Though this does not test for races, I don't know of an automated way to do that. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4108: Weak Proxy Objects
On Dec. 9, 2014, 7:36 p.m., rmudgett wrote: /trunk/main/astobj2.c, line 432 https://reviewboard.asterisk.org/r/4108/diff/3/?file=68553#file68553line432 Use of obj-priv_data.weakptr is not protected from other threads creating a weak object from the object. This wouldn't be a problem if the object were required to be immediately weakened after creation. Corey Farrell wrote: I've transfered the actual creation of weak proxy objects from ao2_weaken to ao2_alloc (with an option). I've changed my mind. Creation of ao2_weakproxy objects requires it's own allocation function. I'm envisioning scenarios where the weakproxy can be a persistant object. So after the object a weakproxy points to is free'd, we may want to point the weakproxy to a new object. Previous patches did not allow for this although there is no technical reason against it. So to your original finding, I've documented the thread safety concerns about ao2_weakproxy_set_object. Documentation / wording is not my strongest area, I'm open to any suggestions that make it clear when it's safe to associate an object to a weakproxy. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4108/#review13933 --- On March 4, 2015, 4:43 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4108/ --- (Updated March 4, 2015, 4:43 p.m.) Review request for Asterisk Developers, George Joseph and rmudgett. Repository: Asterisk Description --- This implements weak references. The weakproxy object is a real ao2 with normal reference counting of its own. When a weakproxy is pointed to a normal object they hold references to each other. The normal object is automatically freed when a single reference remains (the weakproxy). The weakproxy also supports subscriptions that will notify callbacks when the normal object is about to be destroyed. Diffs - /trunk/tests/test_astobj2_weaken.c PRE-CREATION /trunk/main/astobj2.c 432445 /trunk/include/asterisk/astobj2.h 432445 Diff: https://reviewboard.asterisk.org/r/4108/diff/ Testing --- Ran the included test with REF_DEBUG enabled under valgrind. No reference leaks or improper memory access. Though this does not test for races, I don't know of an automated way to do that. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4370: RAII_VAR: nested functions aren't portable. Adapting RAII_VAR to use clang/llvm blocks to get the same/similar functionality. TAKE 2
On Feb. 20, 2015, 12:03 p.m., Corey Farrell wrote: /branches/11/include/asterisk/utils.h, lines 946-948 https://reviewboard.asterisk.org/r/4370/diff/2/?file=71539#file71539line946 I feel that configure should create a #define for the type of nested procedure supported. This should use results of configure (#ifdef HAVE_CLANG_BLOCKS), instead of implementing it's own direct tests. Matt Jordan wrote: Hm. I'm not sure how much better that makes this, since I find parsing out a configure.ac file to be much harder than the #defines in a header. What specifically are you envisioning? Right now, I'm having a hard time determining how this would look in the configure script and how it would be reflected here. The idea is that configure is responsible for testing features and reporting what was found, so we shouldn't attempt to duplicate the logic of those tests. The tests performed in configure are better anyways. In configure.ac where it finds working CLANG support, we want to add: AC_DEFINE([HAVE_CLANG_BLOCKS], 1, [Define to 1 if your compiler supports CLANG blocks.]) And when it finds GCC nested functions: AC_DEFINE([HAVE_GCC_NESTED_FUNCTIONS], 1, [Define to 1 if your compiler supports GCC nested functions.]) Then when you run bootstrap / configure, you will get the appropriate #define / #undef in asterisk/autoconfig.h. Then in asterisk/utils.h: #if defined(HAVE_CLANG_BLOCKS) /* CLANG support code */ #elif defined(HAVE_GCC_NESTED_FUNCTIONS) /* GCC support code */ #else #error #endif - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4370/#review14513 --- On Feb. 20, 2015, 9:35 p.m., Matt Jordan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4370/ --- (Updated Feb. 20, 2015, 9:35 p.m.) Review request for Asterisk Developers and Diederik de Groot. Bugs: ASTERISK-20850 https://issues.asterisk.org/jira/browse/ASTERISK-20850 Repository: Asterisk Description --- This is a continuation of the patch put up for review on r3488. It addresses the issues found on that review. This patch *should* make Asterisk compile under clang. Note that compiling with --enable-dev-mode will cause Asterisk to fail to compile under clang, as it detects a number of warnings that aren't fixed under this patch. Diffs - /branches/11/makeopts.in 432053 /branches/11/main/Makefile 432053 /branches/11/include/asterisk/utils.h 432053 /branches/11/include/asterisk/inline_api.h 432053 /branches/11/configure.ac 432053 /branches/11/configure UNKNOWN /branches/11/Makefile 432053 Diff: https://reviewboard.asterisk.org/r/4370/diff/ Testing --- * Compiled Asterisk with and without --enable-dev-mode using gcc. Asterisk compiles correctly. * Compiled Asterisk without --enable-dev-mode using clang. Asterisk compiles, links, and executes. Note that you will need the BlocksRuntime to run Asterisk when it is compiled with clang. Thanks, Matt Jordan -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4370: RAII_VAR: nested functions aren't portable. Adapting RAII_VAR to use clang/llvm blocks to get the same/similar functionality. TAKE 2
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4370/#review14519 --- /branches/11/configure.ac https://reviewboard.asterisk.org/r/4370/#comment25074 I missed this before / thought Josh pointed it out. This one should be AC_MSG_ERROR as well. /branches/11/configure.ac https://reviewboard.asterisk.org/r/4370/#comment25073 These lines are unneeded as we've failed. - Corey Farrell On Feb. 20, 2015, 9:35 p.m., Matt Jordan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4370/ --- (Updated Feb. 20, 2015, 9:35 p.m.) Review request for Asterisk Developers and Diederik de Groot. Bugs: ASTERISK-20850 https://issues.asterisk.org/jira/browse/ASTERISK-20850 Repository: Asterisk Description --- This is a continuation of the patch put up for review on r3488. It addresses the issues found on that review. This patch *should* make Asterisk compile under clang. Note that compiling with --enable-dev-mode will cause Asterisk to fail to compile under clang, as it detects a number of warnings that aren't fixed under this patch. Diffs - /branches/11/makeopts.in 432053 /branches/11/main/Makefile 432053 /branches/11/include/asterisk/utils.h 432053 /branches/11/include/asterisk/inline_api.h 432053 /branches/11/configure.ac 432053 /branches/11/configure UNKNOWN /branches/11/Makefile 432053 Diff: https://reviewboard.asterisk.org/r/4370/diff/ Testing --- * Compiled Asterisk with and without --enable-dev-mode using gcc. Asterisk compiles correctly. * Compiled Asterisk without --enable-dev-mode using clang. Asterisk compiles, links, and executes. Note that you will need the BlocksRuntime to run Asterisk when it is compiled with clang. Thanks, Matt Jordan -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4436: asterisk/lock.h: Fix syntax errors for non-gcc OSX with 64-bit integers
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4436/ --- (Updated Feb. 20, 2015, 8:45 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 432054 Bugs: ASTERISK-24814 https://issues.asterisk.org/jira/browse/ASTERISK-24814 Repository: Asterisk Description --- asterisk/lock.h: Add a couple of missing closing brackets / parenthesis. Diffs - /branches/11/include/asterisk/lock.h 431994 Diff: https://reviewboard.asterisk.org/r/4436/diff/ Testing --- Looked at the source. I don't know what compiler would match the #if conditions. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4435: Reverse #if statement in asterisk.c to fix code folding
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4435/ --- (Updated Feb. 20, 2015, 8:51 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 432057 Bugs: ASTERISK-24813 https://issues.asterisk.org/jira/browse/ASTERISK-24813 Repository: Asterisk Description --- listener() opens the same code block in two places (#if and #else). This confuses some folding editors causing it to think that an extra code block was opened. Folding in 'geany' causes all code after listener() to be folded as if it were part of that procedure. This patch does not change any compiled logic, it just makes it easier for some editors to parse. Diffs - /trunk/main/asterisk.c 431991 Diff: https://reviewboard.asterisk.org/r/4435/diff/ Testing --- Some testsuite runs. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4428: Allow graceful shutdown to unload modules that register bucket scheme's or codec's.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4428/ --- (Updated Feb. 20, 2015, 8:55 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 432058 Bugs: ASTERISK-24796 https://issues.asterisk.org/jira/browse/ASTERISK-24796 Repository: Asterisk Description --- * Allow modules that call ast_bucket_scheme_register or ast_codec_register to be unloaded during graceful shutdown only (13+ only). * Change __ast_module_shutdown_ref to be NULL safe (11+). Diffs - /branches/13/main/loader.c 431876 /branches/13/main/codec.c 431876 /branches/13/main/bucket.c 431876 Diff: https://reviewboard.asterisk.org/r/4428/diff/ Testing --- Build, basic startup/shutdown test. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4370: RAII_VAR: nested functions aren't portable. Adapting RAII_VAR to use clang/llvm blocks to get the same/similar functionality. TAKE 2
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4370/#review14513 --- /branches/11/include/asterisk/utils.h https://reviewboard.asterisk.org/r/4370/#comment25064 I feel that configure should create a #define for the type of nested procedure supported. This should use results of configure (#ifdef HAVE_CLANG_BLOCKS), instead of implementing it's own direct tests. /branches/11/include/asterisk/utils.h https://reviewboard.asterisk.org/r/4370/#comment25068 #error? - Corey Farrell On Feb. 19, 2015, 11:09 p.m., Matt Jordan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4370/ --- (Updated Feb. 19, 2015, 11:09 p.m.) Review request for Asterisk Developers and Diederik de Groot. Bugs: ASTERISK-20850 https://issues.asterisk.org/jira/browse/ASTERISK-20850 Repository: Asterisk Description --- This is a continuation of the patch put up for review on r3488. It addresses the issues found on that review. This patch *should* make Asterisk compile under clang. Note that compiling with --enable-dev-mode will cause Asterisk to fail to compile under clang, as it detects a number of warnings that aren't fixed under this patch. Diffs - /branches/11/makeopts.in 432011 /branches/11/main/Makefile 432011 /branches/11/include/asterisk/utils.h 432011 /branches/11/include/asterisk/inline_api.h 432011 /branches/11/configure.ac 432011 /branches/11/configure UNKNOWN /branches/11/Makefile 432011 Diff: https://reviewboard.asterisk.org/r/4370/diff/ Testing --- * Compiled Asterisk with and without --enable-dev-mode using gcc. Asterisk compiles correctly. * Compiled Asterisk without --enable-dev-mode using clang. Asterisk compiles, links, and executes. Note that you will need the BlocksRuntime to run Asterisk when it is compiled with clang. Thanks, Matt Jordan -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4435: Reverse #if statement in asterisk.c to fix code folding
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4435/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24813 https://issues.asterisk.org/jira/browse/ASTERISK-24813 Repository: Asterisk Description --- listener() opens the same code block in two places (#if and #else). This confuses some folding editors causing it to think that an extra code block was opened. Folding in 'geany' causes all code after listener() to be folded as if it were part of that procedure. This patch does not change any compiled logic, it just makes it easier for some editors to parse. Diffs - /trunk/main/asterisk.c 431991 Diff: https://reviewboard.asterisk.org/r/4435/diff/ Testing --- Some testsuite runs. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4436: asterisk/lock.h: Fix syntax errors for non-gcc OSX with 64-bit integers
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4436/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24814 https://issues.asterisk.org/jira/browse/ASTERISK-24814 Repository: Asterisk Description --- asterisk/lock.h: Add a couple of missing closing brackets / parenthesis. Diffs - /branches/11/include/asterisk/lock.h 431994 Diff: https://reviewboard.asterisk.org/r/4436/diff/ Testing --- Looked at the source. I don't know what compiler would match the #if conditions. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4435: Reverse #if statement in asterisk.c to fix code folding
On Feb. 19, 2015, 5:26 p.m., rmudgett wrote: /trunk/main/asterisk.c, line 1613 https://reviewboard.asterisk.org/r/4435/diff/1/?file=71478#file71478line1613 I prefer the #if defined(SO_PASSCRED) format as it allows more complicated expressions. Will switch this when I commit. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4435/#review14494 --- On Feb. 19, 2015, 4:43 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4435/ --- (Updated Feb. 19, 2015, 4:43 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24813 https://issues.asterisk.org/jira/browse/ASTERISK-24813 Repository: Asterisk Description --- listener() opens the same code block in two places (#if and #else). This confuses some folding editors causing it to think that an extra code block was opened. Folding in 'geany' causes all code after listener() to be folded as if it were part of that procedure. This patch does not change any compiled logic, it just makes it easier for some editors to parse. Diffs - /trunk/main/asterisk.c 431991 Diff: https://reviewboard.asterisk.org/r/4435/diff/ Testing --- Some testsuite runs. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4425: Create function to work around reference leaks caused by shutdown with pending scheduled events
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4425/ --- (Updated Feb. 18, 2015, 7:59 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 431916 Bugs: ASTERISK-24451 https://issues.asterisk.org/jira/browse/ASTERISK-24451 Repository: Asterisk Description --- When an event is scheduled, it often includes data with a reference bump for the scheduler. If the scheduler context needs to be destroyed before all events have run, references are leaked. This change adds a procedure to run all events that were scheduled for a specific callback, but haven't run yet. chan_iax2 is modified to use this new procedure to run all pending peercnt_remove_cb and replace_callno events. In the long run I think the scheduler will need to be ao2 aware, but that's not feasible for existing releases. Diffs - /branches/11/main/sched.c 431733 /branches/11/include/asterisk/sched.h 431733 /branches/11/channels/chan_iax2.c 431733 Diff: https://reviewboard.asterisk.org/r/4425/diff/ Testing --- Ran a bunch of tests that were leaking references: tests/apps/directed_pickup/pickup_chan, tests/callparking, tests/channels/iax2/acl_call, tests/channels/iax2/basic-call, tests/feature_attended_transfer, tests/feature_blonde_transfer Only tests/callparking still has 1 leaked reference (it was more). Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4428: Allow graceful shutdown to unload modules that register bucket scheme's or codec's.
On Feb. 17, 2015, 9:51 a.m., Joshua Colp wrote: /branches/13/main/loader.c, line 1487 https://reviewboard.asterisk.org/r/4428/diff/1/?file=71466#file71466line1487 Why the !mod addition? Something trigger it? codec_builtin.c runs __ast_codec_register with a NULL module. Technically this is not required for 11, but I'd rather keep the function the same. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4428/#review14474 --- On Feb. 16, 2015, 1:58 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4428/ --- (Updated Feb. 16, 2015, 1:58 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24796 https://issues.asterisk.org/jira/browse/ASTERISK-24796 Repository: Asterisk Description --- * Allow modules that call ast_bucket_scheme_register or ast_codec_register to be unloaded during graceful shutdown only (13+ only). * Change __ast_module_shutdown_ref to be NULL safe (11+). Diffs - /branches/13/main/loader.c 431876 /branches/13/main/codec.c 431876 /branches/13/main/bucket.c 431876 Diff: https://reviewboard.asterisk.org/r/4428/diff/ Testing --- Build, basic startup/shutdown test. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4428: Allow graceful shutdown to unload modules that register bucket scheme's or codec's.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4428/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24796 https://issues.asterisk.org/jira/browse/ASTERISK-24796 Repository: Asterisk Description --- * Allow modules that call ast_bucket_scheme_register or ast_codec_register to be unloaded during graceful shutdown only (13+ only). * Change __ast_module_shutdown_ref to be NULL safe (11+). Diffs - /branches/13/main/loader.c 431876 /branches/13/main/codec.c 431876 /branches/13/main/bucket.c 431876 Diff: https://reviewboard.asterisk.org/r/4428/diff/ Testing --- Build, basic startup/shutdown test. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4425: Create function to work around reference leaks caused by shutdown with pending scheduled events
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4425/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24451 https://issues.asterisk.org/jira/browse/ASTERISK-24451 Repository: Asterisk Description --- When an event is scheduled, it often includes data with a reference bump for the scheduler. If the scheduler context needs to be destroyed before all events have run, references are leaked. This change adds a procedure to run all events that were scheduled for a specific callback, but haven't run yet. chan_iax2 is modified to use this new procedure to run all pending peercnt_remove_cb and replace_callno events. In the long run I think the scheduler will need to be ao2 aware, but that's not feasible for existing releases. Diffs - /branches/11/main/sched.c 431733 /branches/11/include/asterisk/sched.h 431733 /branches/11/channels/chan_iax2.c 431733 Diff: https://reviewboard.asterisk.org/r/4425/diff/ Testing --- Ran a bunch of tests that were leaking references: tests/apps/directed_pickup/pickup_chan, tests/callparking, tests/channels/iax2/acl_call, tests/channels/iax2/basic-call, tests/feature_attended_transfer, tests/feature_blonde_transfer Only tests/callparking still has 1 leaked reference (it was more). Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4425: Create function to work around reference leaks caused by shutdown with pending scheduled events
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4425/ --- (Updated Feb. 15, 2015, 6:27 p.m.) Review request for Asterisk Developers. Changes --- Add locking, tweak while loop. Bugs: ASTERISK-24451 https://issues.asterisk.org/jira/browse/ASTERISK-24451 Repository: Asterisk Description --- When an event is scheduled, it often includes data with a reference bump for the scheduler. If the scheduler context needs to be destroyed before all events have run, references are leaked. This change adds a procedure to run all events that were scheduled for a specific callback, but haven't run yet. chan_iax2 is modified to use this new procedure to run all pending peercnt_remove_cb and replace_callno events. In the long run I think the scheduler will need to be ao2 aware, but that's not feasible for existing releases. Diffs (updated) - /branches/11/main/sched.c 431733 /branches/11/include/asterisk/sched.h 431733 /branches/11/channels/chan_iax2.c 431733 Diff: https://reviewboard.asterisk.org/r/4425/diff/ Testing --- Ran a bunch of tests that were leaking references: tests/apps/directed_pickup/pickup_chan, tests/callparking, tests/channels/iax2/acl_call, tests/channels/iax2/basic-call, tests/feature_attended_transfer, tests/feature_blonde_transfer Only tests/callparking still has 1 leaked reference (it was more). Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4411: testsuite: fix a number of tests where Asterisk does not shutdown gracefully
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4411/ --- (Updated Feb. 11, 2015, 11:09 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 6381 Repository: testsuite Description --- * Add Hangup() to priority after Dial() where needed. This prevents auto-fallthrough from playing 10 seconds of BUSY or CONGESTION tone. * Decrease Wait(10) to Wait(5) in tests/channels/SIP/info_dtmf. * Maintain list of AGI connections where needed so they can all be agi.finish(). * Replace calls to reactor.stop() with self.stop_reactor(), remove test.start_asterisk()/test.stop_asterisk() from main(). * Delay self.stop_reactor() in tests/channels/SIP/sip_tls_call by 2 seconds. This gives the calls enough time to end and avoid shutdown timeout. Diffs - /asterisk/trunk/tests/funcs/func_srv/run-test 6377 /asterisk/trunk/tests/funcs/func_presencestate/run-test 6377 /asterisk/trunk/tests/fastagi/stream-file/run-test 6377 /asterisk/trunk/tests/fastagi/database/run-test 6377 /asterisk/trunk/tests/fastagi/control-stream-file/run-test 6377 /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test 6377 /asterisk/trunk/tests/channels/SIP/sip_srtp/run-test 6377 /asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast1/extensions.conf 6377 /asterisk/trunk/tests/channels/SIP/secure_bridge_media/run-test 6377 /asterisk/trunk/tests/channels/SIP/noload_res_srtp/run-test 6377 /asterisk/trunk/tests/channels/SIP/info_dtmf/configs/ast1/extensions.conf 6377 /asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast1/extensions.conf 6377 /asterisk/trunk/tests/channels/SIP/generic_ccss/configs/ast1/extensions.conf 6377 Diff: https://reviewboard.asterisk.org/r/4411/diff/ Testing --- Ran all effected tests against Asterisk 11 with REF_DEBUG. Prior to these fixes graceful shutdown of Asterisk timed out, causing reference leaks to be reported. These tests now shutdown gracefully and have no reference leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4410: testsuite: Fix output of minversion/maxversion of tests that cannot run
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4410/ --- (Updated Feb. 11, 2015, 11:04 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers and Matt Jordan. Changes --- Committed in revision 6380 Repository: testsuite Description --- r4270 changed minversion/maxversion properties to arrays and updated the output for listing tests. This updates the output for tests that cannot run. Diffs - /asterisk/trunk/runtests.py 6377 Diff: https://reviewboard.asterisk.org/r/4410/diff/ Testing --- Verified correct output for a test that cannot run due to minversion. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4411: testsuite: fix a number of tests where Asterisk does not shutdown gracefully
On Feb. 10, 2015, 12:29 p.m., Mark Michelson wrote: The only caveat here is that you may want to watch automated runs of the SIP info_dtmf test to be sure that on awful hardware the 5 second Wait() isn't too short for the test to complete. I suspect it will be okay though. On a side note, I have another test to add to my list of tests that could be rewritten to not rely on timing, though :) If this turns out to be a problem then we can change the Wait() back to 5 seconds. If that is the case then I think it will be appropriate to increase the timeout for 'core stop gracefully' from 5 seconds to 10. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4411/#review14433 --- On Feb. 9, 2015, 12:50 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4411/ --- (Updated Feb. 9, 2015, 12:50 p.m.) Review request for Asterisk Developers. Repository: testsuite Description --- * Add Hangup() to priority after Dial() where needed. This prevents auto-fallthrough from playing 10 seconds of BUSY or CONGESTION tone. * Decrease Wait(10) to Wait(5) in tests/channels/SIP/info_dtmf. * Maintain list of AGI connections where needed so they can all be agi.finish(). * Replace calls to reactor.stop() with self.stop_reactor(), remove test.start_asterisk()/test.stop_asterisk() from main(). * Delay self.stop_reactor() in tests/channels/SIP/sip_tls_call by 2 seconds. This gives the calls enough time to end and avoid shutdown timeout. Diffs - /asterisk/trunk/tests/funcs/func_srv/run-test 6377 /asterisk/trunk/tests/funcs/func_presencestate/run-test 6377 /asterisk/trunk/tests/fastagi/stream-file/run-test 6377 /asterisk/trunk/tests/fastagi/database/run-test 6377 /asterisk/trunk/tests/fastagi/control-stream-file/run-test 6377 /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test 6377 /asterisk/trunk/tests/channels/SIP/sip_srtp/run-test 6377 /asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast1/extensions.conf 6377 /asterisk/trunk/tests/channels/SIP/secure_bridge_media/run-test 6377 /asterisk/trunk/tests/channels/SIP/noload_res_srtp/run-test 6377 /asterisk/trunk/tests/channels/SIP/info_dtmf/configs/ast1/extensions.conf 6377 /asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast1/extensions.conf 6377 /asterisk/trunk/tests/channels/SIP/generic_ccss/configs/ast1/extensions.conf 6377 Diff: https://reviewboard.asterisk.org/r/4411/diff/ Testing --- Ran all effected tests against Asterisk 11 with REF_DEBUG. Prior to these fixes graceful shutdown of Asterisk timed out, causing reference leaks to be reported. These tests now shutdown gracefully and have no reference leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4141: Enable REF_DEBUG for ast_module_ref / ast_module_unref
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4141/ --- (Updated Feb. 11, 2015, 9:38 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 431662 Bugs: ASTERISK-24479 https://issues.asterisk.org/jira/browse/ASTERISK-24479 Repository: Asterisk Description --- This change includes an ABI change with compatibility stubs for 11 and 13. The compatibility stubs will not be included in trunk. The point of this change is to have each module create an AO2 object on load, and hopefully destroy it on unload. This allows module reference count errors to be debugged through REF_DEBUG. When REF_DEBUG is enabled: * adds an empty ao2 object to 'struct ast_module' * Allocate ao2 when the module is loaded * Perform an ao2_ref in each place where mod-usecount is manipulated. * ao2_cleanup on module unload. The passthrough of file, line and func is needed for the REF_DEBUG to be of any use, so without the ABI changes this is not useful. The change to bridge_builtin_features.c ensures that the module cannot be manually unloaded, but is able to be unloaded during ast_module_shutdown. Note ast_module_shutdown only happens during clean shutdown and does not actually run dlclose so this is safe. Diffs - /branches/11/main/loader.c 429406 /branches/11/include/asterisk/module.h 429406 /branches/11/bridges/bridge_builtin_features.c 429406 Diff: https://reviewboard.asterisk.org/r/4141/diff/ Testing --- Using tests/manager/originate with REF_DEBUG enabled. When the change to bridge_builtin_features.c is omitted the test fails due to that one reference leak. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4411: testsuite: fix a number of tests where Asterisk does not shutdown gracefully
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4411/ --- Review request for Asterisk Developers. Repository: testsuite Description --- * Add Hangup() to priority after Dial() where needed. This prevents auto-fallthrough from playing 10 seconds of BUSY or CONGESTION tone. * Decrease Wait(10) to Wait(5) in tests/channels/SIP/info_dtmf. * Maintain list of AGI connections where needed so they can all be agi.finish(). * Replace calls to reactor.stop() with self.stop_reactor(), remove test.start_asterisk()/test.stop_asterisk() from main(). * Delay self.stop_reactor() in tests/channels/SIP/sip_tls_call by 2 seconds. This gives the calls enough time to end and avoid shutdown timeout. Diffs - /asterisk/trunk/tests/funcs/func_srv/run-test 6377 /asterisk/trunk/tests/funcs/func_presencestate/run-test 6377 /asterisk/trunk/tests/fastagi/stream-file/run-test 6377 /asterisk/trunk/tests/fastagi/database/run-test 6377 /asterisk/trunk/tests/fastagi/control-stream-file/run-test 6377 /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test 6377 /asterisk/trunk/tests/channels/SIP/sip_srtp/run-test 6377 /asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast1/extensions.conf 6377 /asterisk/trunk/tests/channels/SIP/secure_bridge_media/run-test 6377 /asterisk/trunk/tests/channels/SIP/noload_res_srtp/run-test 6377 /asterisk/trunk/tests/channels/SIP/info_dtmf/configs/ast1/extensions.conf 6377 /asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast1/extensions.conf 6377 /asterisk/trunk/tests/channels/SIP/generic_ccss/configs/ast1/extensions.conf 6377 Diff: https://reviewboard.asterisk.org/r/4411/diff/ Testing --- Ran all effected tests against Asterisk 11 with REF_DEBUG. Prior to these fixes graceful shutdown of Asterisk timed out, causing reference leaks to be reported. These tests now shutdown gracefully and have no reference leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4410: testsuite: Fix output of minversion/maxversion of tests that cannot run
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4410/ --- Review request for Asterisk Developers and Matt Jordan. Repository: testsuite Description --- r4270 changed minversion/maxversion properties to arrays and updated the output for listing tests. This updates the output for tests that cannot run. Diffs - /asterisk/trunk/runtests.py 6377 Diff: https://reviewboard.asterisk.org/r/4410/diff/ Testing --- Verified correct output for a test that cannot run due to minversion. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4141: Enable REF_DEBUG for ast_module_ref / ast_module_unref
On Dec. 2, 2014, 9:30 a.m., opticron wrote: /branches/11/include/asterisk/module.h, lines 272-278 https://reviewboard.asterisk.org/r/4141/diff/1/?file=68704#file68704line272 These could use a bit of documentation. I've documented the macro's instead. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4141/#review13861 --- On Nov. 2, 2014, 2:13 a.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4141/ --- (Updated Nov. 2, 2014, 2:13 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24479 https://issues.asterisk.org/jira/browse/ASTERISK-24479 Repository: Asterisk Description --- This change includes an ABI change with compatibility stubs for 11, 12 and 13. The compatibility stubs will not be included in trunk. The point of this change is to have each module create an AO2 object on load, and hopefully destroy it on unload. This allows module reference count errors to be debugged through REF_DEBUG. When REF_DEBUG is enabled: * adds an empty ao2 object to 'struct ast_module' * Allocate ao2 when the module is loaded * Perform an ao2_ref in each place where mod-usecount is manipulated. * ao2_cleanup on module unload. The passthrough of file, line and func is needed for the REF_DEBUG to be of any use, so without the ABI changes this is not useful. The change to bridge_builtin_features.c ensures that the module cannot be manually unloaded, but is able to be unloaded during ast_module_shutdown. Note ast_module_shutdown only happens during clean shutdown and does not actually run dlclose so this is safe. Diffs - /branches/11/main/loader.c 426830 /branches/11/include/asterisk/module.h 426830 /branches/11/bridges/bridge_builtin_features.c 426830 Diff: https://reviewboard.asterisk.org/r/4141/diff/ Testing --- Using tests/manager/originate with REF_DEBUG enabled. When the change to bridge_builtin_features.c is omitted the test fails due to that one reference leak. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4141: Enable REF_DEBUG for ast_module_ref / ast_module_unref
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4141/ --- (Updated Feb. 5, 2015, 3:36 a.m.) Review request for Asterisk Developers. Changes --- * Make all 3 functions always accept the REF_DEBUG parameters. They are not called enough to be worth the optimization by removing the unused parameters. * Updated patch to show actual code for 11. 13/trunk are different in bridge_builtin_features.c * Switched list iteration to normal (non _SAFE) as no elements are added/removed. * Address findings Bugs: ASTERISK-24479 https://issues.asterisk.org/jira/browse/ASTERISK-24479 Repository: Asterisk Description (updated) --- This change includes an ABI change with compatibility stubs for 11 and 13. The compatibility stubs will not be included in trunk. The point of this change is to have each module create an AO2 object on load, and hopefully destroy it on unload. This allows module reference count errors to be debugged through REF_DEBUG. When REF_DEBUG is enabled: * adds an empty ao2 object to 'struct ast_module' * Allocate ao2 when the module is loaded * Perform an ao2_ref in each place where mod-usecount is manipulated. * ao2_cleanup on module unload. The passthrough of file, line and func is needed for the REF_DEBUG to be of any use, so without the ABI changes this is not useful. The change to bridge_builtin_features.c ensures that the module cannot be manually unloaded, but is able to be unloaded during ast_module_shutdown. Note ast_module_shutdown only happens during clean shutdown and does not actually run dlclose so this is safe. Diffs (updated) - /branches/11/main/loader.c 429406 /branches/11/include/asterisk/module.h 429406 /branches/11/bridges/bridge_builtin_features.c 429406 Diff: https://reviewboard.asterisk.org/r/4141/diff/ Testing --- Using tests/manager/originate with REF_DEBUG enabled. When the change to bridge_builtin_features.c is omitted the test fails due to that one reference leak. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4108: Weak References
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4108/ --- (Updated Feb. 5, 2015, 2:30 a.m.) Review request for Asterisk Developers, George Joseph and rmudgett. Changes --- Address Richard's findings. Repository: Asterisk Description --- This implements weak references. The weak object is a real ao2 with normal reference counting of its own. The original object is destroyed when the only reference remaining is from the weak object. Diffs (updated) - /trunk/tests/test_astobj2_weaken.c PRE-CREATION /trunk/main/astobj2.c 431571 /trunk/include/asterisk/astobj2.h 431571 Diff: https://reviewboard.asterisk.org/r/4108/diff/ Testing --- Very little, I'm unsure how to actually test that this cannot race, since any potential for a race would be due to very exact timing. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4108: Weak References
On Dec. 9, 2014, 7:36 p.m., rmudgett wrote: Summary of this weak ref implementation: weak_proxy_obj - obj * The weak_proxy_obj and the obj get refs to each other on the initial call to ao2_weaken(). * The weak_proxy_obj will then stick around for as long as obj lives so it can be returned by subsequent ao2_weaken() calls. * Once obj dies because its last external ref is released, the weak_proxy_obj will die when the last external ref to it is released. This weak object strategy is workable for random objects. It seems rather expensive for use by keyed containers because the real obj needs to be referenced by container operations through the sort_fn, hash_fn, and cmp_fn callbacks. What about exposing a field 'void *data;' to the weak objects? This would allow containers of weak proxy objects to make a copy of the data needed for some or all callbacks. Object types could sacrifice some memory in exchange for speed of container operations. Honestly I'm not sure if this would be a good thing or not. On Dec. 9, 2014, 7:36 p.m., rmudgett wrote: /trunk/main/astobj2.c, lines 798-804 https://reviewboard.asterisk.org/r/4108/diff/3/?file=68553#file68553line798 Use of ao2_bump here is unnecessary since you already checked if the pointer was NULL. Thanks for pointing out. I tend to forget the overhead of the NULL check in ao2_bump and use it for the return value. Fixed. On Dec. 9, 2014, 7:36 p.m., rmudgett wrote: /trunk/tests/test_astobj2_weaken.c, line 118 https://reviewboard.asterisk.org/r/4108/diff/3/?file=68554#file68554line118 This should be fail_cleanup1 so obj gets cleaned up. This is a paranoia check - the test only passes if all conditions of the if statement are false. If obj were somehow freed early all conditions would be true. In that case obj points to an object that was already destroyed. I feel the best thing is for this test to risk leaking in this situation, since the other option would be to risk a crash. On Dec. 9, 2014, 7:36 p.m., rmudgett wrote: /trunk/tests/test_astobj2_weaken.c, lines 91-96 https://reviewboard.asterisk.org/r/4108/diff/3/?file=68554#file68554line91 Use of weakref1 after ref released. This is intentional. I'm checking the pointer without dereferencing it, so this is ok. On Dec. 9, 2014, 7:36 p.m., rmudgett wrote: /trunk/main/astobj2.c, line 432 https://reviewboard.asterisk.org/r/4108/diff/3/?file=68553#file68553line432 Use of obj-priv_data.weakptr is not protected from other threads creating a weak object from the object. This wouldn't be a problem if the object were required to be immediately weakened after creation. I've transfered the actual creation of weak proxy objects from ao2_weaken to ao2_alloc (with an option). - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4108/#review13933 --- On Oct. 26, 2014, 7:10 a.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4108/ --- (Updated Oct. 26, 2014, 7:10 a.m.) Review request for Asterisk Developers, George Joseph and rmudgett. Repository: Asterisk Description --- This implements weak references. The weak object is a real ao2 with normal reference counting of its own. The original object is destroyed when the only reference remaining is from the weak object. Diffs - /trunk/tests/test_astobj2_weaken.c PRE-CREATION /trunk/main/astobj2.c 426139 /trunk/include/asterisk/astobj2.h 426139 Diff: https://reviewboard.asterisk.org/r/4108/diff/ Testing --- Very little, I'm unsure how to actually test that this cannot race, since any potential for a race would be due to very exact timing. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4374: Asterisk: For httpd server, need option to define server name for security purposes
On Jan. 28, 2015, 7:21 a.m., Corey Farrell wrote: If we assume that there are always unknown security vulnerabilities, I think it is worth completely removing Server: Asterisk/version. Another option would be trimming to major version only - Server: Asterisk/13. Otherwise any system with default config that does not receive a security update will always inform hackers of that fact. I'm not sure others will agree with this but feel that it needs to be considered. Matt Jordan wrote: I thought about this as well. As a rebuttal to changing this mid-stream, I'd note the following: * Although it is somewhat unlikely, there is a chance that someone has built a system relying on this information. For example, if I had a pool of private Asterisk servers, I may be using cURL to check Asterisk's HTTP server to get the version that is deployed on each server. While this isn't highly likely, I've seen systems that do weirder things. I'd prefer to not break existing systems unless we feel there is no other option. * We do the same thing in other areas. For example, the UserAgent header and the SDP session name in chan_sip include the version. Arguably, this exposes Asterisk more than the HTTP server - we are far more likely to have someone inspecting the SIP traffic than the HTTP server (which sits on a non-standard port). As it is, I'd be fine if we changed this in trunk, but I'd prefer the 11/13 implementations to keep the existing behaviour. If we uncomment servername=asterisk in the sample config for trunk only I'd be happy. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4374/#review14339 --- On Jan. 28, 2015, 9:13 p.m., Ashley Sanders wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4374/ --- (Updated Jan. 28, 2015, 9:13 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24316 https://issues.asterisk.org/jira/browse/ASTERISK-24316 Repository: Asterisk Description --- Currently, all responses from the Asterisk HTTP server contain a [Server] header that identifies Asterisk and its version (e.g. Server:Asterisk/version, where version is the currently running version of Asterisk). The preferred behavior is to allow the user to configure an alternate name to use for the value returned in the [Server] header for HTTP responses (e.g. Server:SomeSuperAwesomeServerName). This patch provides a new configuration property, [servername], in http.conf, that gives users the ability to modify the value that Asterisk uses when identifying itself. By default, the new property is unused, which means that out-of-the-box, the HTTP server behaves just like it did prior to the patch. Requests to the HTTP server will generate responses with the old-style [Server] header (e.g. Server:Asterisk/version, where version is the currently running version of Asterisk). To see the new behavior, you must add the configuration property, [servername] with some value (an empty value will work, also) to http.conf. Whatever value the HTTP server is holding for the server name can now be seen through the httpstatus web page (http://bindaddr:bindport/prefix/httpstatus) (where [bindaddr], [bindport], and [prefix] are all values configured in http.conf) and the CLI command: http show status. ***Note*** This is just the patch to the Asterisk source. You can find the review for the Testsuite at: https://reviewboard.asterisk.org/r/4377/ Diffs - ./branches/13/main/http.c 431112 ./branches/13/include/asterisk/http.h 431112 ./branches/13/configs/samples/http.conf.sample 431112 Diff: https://reviewboard.asterisk.org/r/4374/diff/ Testing --- Thanks, Ashley Sanders -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4374: Asterisk: For httpd server, need option to define server name for security purposes
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4374/#review14339 --- If we assume that there are always unknown security vulnerabilities, I think it is worth completely removing Server: Asterisk/version. Another option would be trimming to major version only - Server: Asterisk/13. Otherwise any system with default config that does not receive a security update will always inform hackers of that fact. I'm not sure others will agree with this but feel that it needs to be considered. - Corey Farrell On Jan. 27, 2015, 7:16 p.m., Ashley Sanders wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4374/ --- (Updated Jan. 27, 2015, 7:16 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24316 https://issues.asterisk.org/jira/browse/ASTERISK-24316 Repository: Asterisk Description --- Currently, all responses from the Asterisk HTTP server contain a [Server] header that identifies Asterisk and its version (e.g. Server:Asterisk/version, where version is the currently running version of Asterisk). The preferred behavior is to allow the user to configure an alternate name to use for the value returned in the [Server] header for HTTP responses (e.g. Server:SomeSuperAwesomeServerName). This patch provides a new configuration property, [servername], in http.conf, that gives users the ability to modify the value that Asterisk uses when identifying itself. By default, the new property is unused, which means that out-of-the-box, the HTTP server behaves just like it did prior to the patch. Requests to the HTTP server will generate responses with the old-style [Server] header (e.g. Server:Asterisk/version, where version is the currently running version of Asterisk). To see the new behavior, you must add the configuration property, [servername] with some value (an empty value will work, also) to http.conf. Whatever value the HTTP server is holding for the server name can now be seen through the httpstatus web page (http://bindaddr:bindport/prefix/httpstatus) (where [bindaddr], [bindport], and [prefix] are all values configured in http.conf) and the CLI command: http show status. ***Note*** This is just the patch to the Asterisk source. You can find the review for the Testsuite at: https://reviewboard.asterisk.org/r/4377/ Diffs - ./branches/13/main/http.c 431112 ./branches/13/include/asterisk/http.h 431112 ./branches/13/configs/samples/http.conf.sample 431112 Diff: https://reviewboard.asterisk.org/r/4374/diff/ Testing --- Thanks, Ashley Sanders -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4186: stringfields: Fix regression from fix for unintentional memory retention caused by ast_string_fields_copy
On Nov. 18, 2014, 1:45 p.m., rmudgett wrote: Ship It! wdoekes wrote: Corey: is this an issue in 12+ only because the code is different? Or did the test simply not fail in 11-? While I hope the former, from your comment [1] on the ticket I cannot deduce which it is. [1] It appears that the issue I thought applied to 1.8/11 does not exist. So this issue applies to 12+ only. Originally I thought that __ast_string_field_ptr_grow could change the value of __p__, but Richard corrected me on this. This is the only issue I thought I found in ast_string_field_ptr_set_by_fields. The issue that does exist and is being fixed is with ast_string_fields_copy, which is 12+ only. Maybe I should have said So this ticket applies to 12+ only. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4186/#review13806 --- On Nov. 18, 2014, 1:36 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4186/ --- (Updated Nov. 18, 2014, 1:36 p.m.) Review request for Asterisk Developers and rmudgett. Bugs: ASTERISK-24535 https://issues.asterisk.org/jira/browse/ASTERISK-24535 Repository: Asterisk Description --- ast_string_fields_copy relies on the fact that __ast_string_field_release_active never previously zeroed pool-used, so keeping the existing pointer was ok. Setting each field to __ast_string_field_empty after releasing the memory seems to resolve the issue. Diffs - /branches/12/include/asterisk/stringfields.h 428167 Diff: https://reviewboard.asterisk.org/r/4186/diff/ Testing --- Full testsuite against 12. I had 17 failures, but that is normal on my system. I re-ran the 17 tests without this patch, they still failed. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4186: stringfields: Fix regression from fix for unintentional memory retention caused by ast_string_fields_copy
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4186/ --- (Updated Nov. 19, 2014, 1:30 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers and rmudgett. Changes --- Committed in revision 428272 Bugs: ASTERISK-24535 https://issues.asterisk.org/jira/browse/ASTERISK-24535 Repository: Asterisk Description --- ast_string_fields_copy relies on the fact that __ast_string_field_release_active never previously zeroed pool-used, so keeping the existing pointer was ok. Setting each field to __ast_string_field_empty after releasing the memory seems to resolve the issue. Diffs - /branches/12/include/asterisk/stringfields.h 428167 Diff: https://reviewboard.asterisk.org/r/4186/diff/ Testing --- Full testsuite against 12. I had 17 failures, but that is normal on my system. I re-ran the 17 tests without this patch, they still failed. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4090: testsuite: add basic valgrind support
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4090/#review13791 --- Ship it! Please discard my findings, they are minor. I'd like to see this committed ASAP so further improvements can be made against it. - Corey Farrell On Oct. 16, 2014, 5:23 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4090/ --- (Updated Oct. 16, 2014, 5:23 p.m.) Review request for Asterisk Developers. Repository: testsuite Description --- This adds very basic valgrind support, which is convenient for manual test runs but does not (yet) include support for more automated valgrind usage. 1) CLI flag '-V' enables valgrind (./runtests -V -t tests/test) 2) Minimal changes to testsuite, other improvements can be added later if desired 3) Valgrind output is picked up by error logging and shown after test run. 4) Unlike previous valgrind attempt, this one works fine on tests with multiple asterisk instances Diffs - /asterisk/trunk/runtests.py 5733 /asterisk/trunk/lib/python/asterisk/test_case.py 5733 /asterisk/trunk/lib/python/asterisk/asterisk.py 5733 Diff: https://reviewboard.asterisk.org/r/4090/diff/ Testing --- Thanks, Scott Griepentrog -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4186: stringfields: Fix regression from fix for unintentional memory retention and another issue exposed by the fix
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4186/ --- (Updated Nov. 18, 2014, 9:25 a.m.) Review request for Asterisk Developers and rmudgett. Changes --- Add JIRA ticket. Bugs: ASTERISK-24535 https://issues.asterisk.org/jira/browse/ASTERISK-24535 Repository: Asterisk Description --- This addresses a regression in the previous fix that applies to all versions. When __ast_string_field_ptr_grow is called by ast_string_field_ptr_set_by_fields, it needs to be passed target, not __p__. In the current code if __ast_string_field_ptr_grow actually does anything, it will cause *__p__ != target. Unfortunately in this case *__p__ points to the new address, target to the old. This may cause too much data to be written to the old space for the string, and this could cause memory corruption. This was the first thing I noticed when attempting to troubleshoot res/parking/dynamic_parking_variables unit test failure, but did not fix the test. Also needed to get the unit test working again is a fix for ast_string_fields_copy in 12+. The existing code relies on the fact that __ast_string_field_release_active never previously zeroed pool-used, so keeping the existing pointer was ok. Setting each field to __ast_string_field_empty after releasing the memory seems to resolve the issue. Diffs - /branches/12/include/asterisk/stringfields.h 427735 Diff: https://reviewboard.asterisk.org/r/4186/diff/ Testing --- Full testsuite against 12. I had 17 failures, but that is normal on my system. I re-ran the 17 tests without this patch, they still failed. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4090: testsuite: add basic valgrind support
On Nov. 18, 2014, 9:18 a.m., Corey Farrell wrote: Please discard my findings, they are minor. I'd like to see this committed ASAP so further improvements can be made against it. Scott Griepentrog wrote: I like your idea of adding an option to pass valgrind options, but it needs to first be set to write logs to the appropriate run_x/asty directory so there isn't a conflict between multiple asterisk instances. Then a simple parser added to check the log for critical issues that trigger a test failure, which then get included in the test results, even if in an abbreviated form. Not sure how best to pass leak info other than just leaving it in the log though. Upon further thought, I'm not sure it's desirable to be able to add options through the command-line. Extra options read from ~/.valgrindrc, $VALGRIND_OPTS, ./.valgrindrc. So I think in most cases it would be best for people to create ./.valgrindrc. Maybe we can add that to svn:ignore for '.'. But as you said, getting the logs to run_x/asty dirs is more important. Once this change is submitted I will look at putting the logs in the appropriate dir. As for parsing the test results, I think this would require using --xml-file=, then using libxml and/or libxslt (through python, sorry I don't know the modules for this). Anything we do to parse the text-based valgrind output will be a fragile hack. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4090/#review13791 --- On Nov. 18, 2014, 10:49 a.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4090/ --- (Updated Nov. 18, 2014, 10:49 a.m.) Review request for Asterisk Developers. Repository: testsuite Description --- This adds very basic valgrind support, which is convenient for manual test runs but does not (yet) include support for more automated valgrind usage. 1) CLI flag '-V' enables valgrind (./runtests -V -t tests/test) 2) Minimal changes to testsuite, other improvements can be added later if desired 3) Valgrind output is picked up by error logging and shown after test run. 4) Unlike previous valgrind attempt, this one works fine on tests with multiple asterisk instances Diffs - /asterisk/trunk/runtests.py 5733 /asterisk/trunk/lib/python/asterisk/test_case.py 5733 /asterisk/trunk/lib/python/asterisk/asterisk.py 5733 Diff: https://reviewboard.asterisk.org/r/4090/diff/ Testing --- Thanks, Scott Griepentrog -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4186: stringfields: Fix regression from fix for unintentional memory retention and another issue exposed by the fix
On Nov. 18, 2014, 1:19 p.m., rmudgett wrote: What you have found shows that the problem is in v12+ and not v1.8+ I had thought that the call to __ast_string_field_ptr_grow was a problem in 1.8/11 based on thinking that 'AST_STRING_FIELD_ALLOCATION(*ptr) += grow;' would modify the __p__, but if that's not the case then the only issue I could find is with ast_string_fields_copy, which is only in v12+. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4186/#review13803 --- On Nov. 18, 2014, 9:25 a.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4186/ --- (Updated Nov. 18, 2014, 9:25 a.m.) Review request for Asterisk Developers and rmudgett. Bugs: ASTERISK-24535 https://issues.asterisk.org/jira/browse/ASTERISK-24535 Repository: Asterisk Description --- This addresses a regression in the previous fix that applies to all versions. When __ast_string_field_ptr_grow is called by ast_string_field_ptr_set_by_fields, it needs to be passed target, not __p__. In the current code if __ast_string_field_ptr_grow actually does anything, it will cause *__p__ != target. Unfortunately in this case *__p__ points to the new address, target to the old. This may cause too much data to be written to the old space for the string, and this could cause memory corruption. This was the first thing I noticed when attempting to troubleshoot res/parking/dynamic_parking_variables unit test failure, but did not fix the test. Also needed to get the unit test working again is a fix for ast_string_fields_copy in 12+. The existing code relies on the fact that __ast_string_field_release_active never previously zeroed pool-used, so keeping the existing pointer was ok. Setting each field to __ast_string_field_empty after releasing the memory seems to resolve the issue. Diffs - /branches/12/include/asterisk/stringfields.h 427735 Diff: https://reviewboard.asterisk.org/r/4186/diff/ Testing --- Full testsuite against 12. I had 17 failures, but that is normal on my system. I re-ran the 17 tests without this patch, they still failed. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4186: stringfields: Fix regression from fix for unintentional memory retention caused by ast_string_fields_copy
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4186/ --- (Updated Nov. 18, 2014, 1:33 p.m.) Review request for Asterisk Developers and rmudgett. Changes --- Update patch to remove change that had no effect. Update title/branches. Summary (updated) - stringfields: Fix regression from fix for unintentional memory retention caused by ast_string_fields_copy Bugs: ASTERISK-24535 https://issues.asterisk.org/jira/browse/ASTERISK-24535 Repository: Asterisk Description --- This addresses a regression in the previous fix that applies to all versions. When __ast_string_field_ptr_grow is called by ast_string_field_ptr_set_by_fields, it needs to be passed target, not __p__. In the current code if __ast_string_field_ptr_grow actually does anything, it will cause *__p__ != target. Unfortunately in this case *__p__ points to the new address, target to the old. This may cause too much data to be written to the old space for the string, and this could cause memory corruption. This was the first thing I noticed when attempting to troubleshoot res/parking/dynamic_parking_variables unit test failure, but did not fix the test. Also needed to get the unit test working again is a fix for ast_string_fields_copy in 12+. The existing code relies on the fact that __ast_string_field_release_active never previously zeroed pool-used, so keeping the existing pointer was ok. Setting each field to __ast_string_field_empty after releasing the memory seems to resolve the issue. Diffs (updated) - /branches/12/include/asterisk/stringfields.h 428167 Diff: https://reviewboard.asterisk.org/r/4186/diff/ Testing --- Full testsuite against 12. I had 17 failures, but that is normal on my system. I re-ran the 17 tests without this patch, they still failed. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4186: stringfields: Fix regression from fix for unintentional memory retention caused by ast_string_fields_copy
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4186/ --- (Updated Nov. 18, 2014, 1:36 p.m.) Review request for Asterisk Developers and rmudgett. Changes --- Fix description Bugs: ASTERISK-24535 https://issues.asterisk.org/jira/browse/ASTERISK-24535 Repository: Asterisk Description (updated) --- ast_string_fields_copy relies on the fact that __ast_string_field_release_active never previously zeroed pool-used, so keeping the existing pointer was ok. Setting each field to __ast_string_field_empty after releasing the memory seems to resolve the issue. Diffs - /branches/12/include/asterisk/stringfields.h 428167 Diff: https://reviewboard.asterisk.org/r/4186/diff/ Testing --- Full testsuite against 12. I had 17 failures, but that is normal on my system. I re-ran the 17 tests without this patch, they still failed. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4194: ast_str: Fix improper member access to struct ast_str members.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4194/#review13809 --- Ship it! Ship It! - Corey Farrell On Nov. 18, 2014, 3:03 p.m., rmudgett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4194/ --- (Updated Nov. 18, 2014, 3:03 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Accessing members of struct ast_str outside of the string manipulation API routines is invalid since struct ast_str is supposed to be opaque. Diffs - /branches/11/res/res_calendar.c 428170 /branches/11/channels/sip/security_events.c 428170 /branches/11/channels/chan_sip.c 428170 Diff: https://reviewboard.asterisk.org/r/4194/diff/ Testing --- Use of the DEBUG_OPAQUE define no longer produces compile errors with the patch. Thanks, rmudgett -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4186: stringfields: Fix regression from fix for unintentional memory retention and another issue exposed by the fix
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4186/ --- (Updated Nov. 17, 2014, 10:48 a.m.) Review request for Asterisk Developers and rmudgett. Repository: Asterisk Description --- This addresses a regression in the previous fix that applies to all versions. When __ast_string_field_ptr_grow is called by ast_string_field_ptr_set_by_fields, it needs to be passed target, not __p__. In the current code if __ast_string_field_ptr_grow actually does anything, it will cause *__p__ != target. Unfortunately in this case *__p__ points to the new address, target to the old. This may cause too much data to be written to the old space for the string, and this could cause memory corruption. This was the first thing I noticed when attempting to troubleshoot res/parking/dynamic_parking_variables unit test failure, but did not fix the test. Also needed to get the unit test working again is a fix for ast_string_fields_copy in 12+. The existing code relies on the fact that __ast_string_field_release_active never previously zeroed pool-used, so keeping the existing pointer was ok. Setting each field to __ast_string_field_empty after releasing the memory seems to resolve the issue. Diffs - /branches/12/include/asterisk/stringfields.h 427735 Diff: https://reviewboard.asterisk.org/r/4186/diff/ Testing (updated) --- Full testsuite against 12. I had 17 failures, but that is normal on my system. I re-ran the 17 tests without this patch, they still failed. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4160: chan_sip: Fix theoretical leak of p-refer
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4160/ --- (Updated Nov. 17, 2014, 9:56 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 428117 Bugs: ASTERISK-15242 https://issues.asterisk.org/jira/browse/ASTERISK-15242 Repository: Asterisk Description --- If transmit_refer is called when p-refer is already allocated, it will leak the previous allocation. I checked for all occurrences of sip_refer_alloc, found that transmit_refer was the only caller that didn't check p-refer first. This change moves the check for !p-refer to sip_refer_alloc. I made transmit_refer destroy any previous p-refer so it will have a clean structure after reallocation like it does currently. Unsure if it's needed, but the little bit of extra processing is worth keeping this fix low risk. The change is slightly different in 12+, as p-refer-refer_call only exists in 11. Diffs - /branches/11/channels/chan_sip.c 427685 Diff: https://reviewboard.asterisk.org/r/4160/diff/ Testing --- tests/channels/SIP against 11 Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/#review13789 --- Ship it! Ship It! - Corey Farrell On Nov. 14, 2014, 6:03 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Nov. 14, 2014, 6:03 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs - /branches/13/main/asterisk.c 427948 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4186: stringfields: Fix regression from fix for unintentional memory retention and another issue exposed by the fix
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4186/ --- Review request for Asterisk Developers and rmudgett. Repository: Asterisk Description --- This addresses a regression in the previous fix that applies to all versions. When __ast_string_field_ptr_grow is called by ast_string_field_ptr_set_by_fields, it needs to be passed target, not __p__. In the current code if __ast_string_field_ptr_grow actually does anything, it will cause *__p__ != target. Unfortunately in this case *__p__ points to the new address, target to the old. This may cause too much data to be written to the old space for the string, and this could cause memory corruption. This was the first thing I noticed when attempting to troubleshoot res/parking/dynamic_parking_variables unit test failure, but did not fix the test. Also needed to get the unit test working again is a fix for ast_string_fields_copy in 12+. The existing code relies on the fact that __ast_string_field_release_active never previously zeroed pool-used, so keeping the existing pointer was ok. Setting each field to __ast_string_field_empty after releasing the memory seems to resolve the issue. Diffs - /branches/12/include/asterisk/stringfields.h 427735 Diff: https://reviewboard.asterisk.org/r/4186/diff/ Testing --- Not enough, so far I've only verified this change with the unit test brought to my attention by Richard. I'll be running the full testsuite ASAP, but wanted to allow this code to be seen now. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4166: testsuite: tests/bridge/bridge_action leaves a channel open
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4166/ --- (Updated Nov. 15, 2014, 2:37 p.m.) Status -- This change has been discarded. Review request for Asterisk Developers. Repository: testsuite Description --- self.channels[3] is not hung up, causing the Asterisk graceful shutdown to timeout. This causes the test to fail under REF_DEBUG mode and prevents coverage from seeing the code executed by this test. Diffs - /asterisk/trunk/tests/bridge/bridge_action/bridge_action.py 5920 Diff: https://reviewboard.asterisk.org/r/4166/diff/ Testing --- Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4166: testsuite: tests/bridge/bridge_action leaves a channel open
On Nov. 13, 2014, 11:30 a.m., Mark Michelson wrote: With the fix being made to the leaked bridge in Asterisk, is this change still required? Does hanging up self.channels[1] not result in self.channels[3] and the bridge being destroyed as expected? Corey Farrell wrote: Still required, I'm guessing that when the first channel hangs up it leaves the second in a single user bridge. I'm not sure if this is a bug in Asterisk or just the way it works. The XMLDOC doesn't specify. Corey Farrell wrote: Actually on further inspection it appears the bridge is destroyed, and the caller is sent back to the previous Dialplan location. core show channels without hangup of self.channels[3]: Channel Location State Application(Data) Local/waiting_area@d (None) Up Echo() Local/waiting_area@d (None) Up Echo() 2 active channels 2 active calls 14 calls processed If I then run 'channel request hangup Local/waiting_area@default-0003;1' (or hangup ;2), the test finishes and passes leak free. This is strange to me, why all the other channel pairs were hung up but not this one. Mark Michelson wrote: There are a few of unique properties to the final bridge that, imo, shouldn't affect operations but in practice might: 1) The final bridge between 1 and 3 is the only one where the remaining channel in the bridge came from another bridge rather than directly from the dialplan. In fact, the remaining channel had been in two previous bridges, so its movement through the bridges may have something to do with it. 2) The final bridge between 1 and 3 is the only one where a channel is hung up. In the other cases, a channel is stolen from the bridge, resulting in bridge dissolution. After looking at the code again in action_bridge in features.c, the channels that are bridged together have ast_bridge_set_after_go_on() set on them. Looking at the docs for ast_bridge_set_after_go_on(), it says If parseable_goto, then use the given context/exten/priority as the relative position for the parseable goto. Else goto the given context/exten/priorit+1. We don't provide a parseable goto, so after becoming unbridged, the channels should move to the next priority in the dialplan after the one they're currently in. Since the channels being bridged are currently at the Echo() priority, they should be returned to the dialplan at the next priority, which is Hangup(). This happens with all but channel 3. Somehow, I'm guessing that some code path is causing the priority number to goto after the bridge to get screwed up, so it ends up back in Echo() instead of moving on to Hangup(). I think this is a problem in Asterisk, not the test. Joshua just pointed out ASTERISK-24020, looks like this is already a reported issue against Asterisk. I'm discarding this review since we've determined the testsuite it not at fault. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4166/#review13745 --- On Nov. 11, 2014, 3:37 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4166/ --- (Updated Nov. 11, 2014, 3:37 p.m.) Review request for Asterisk Developers. Repository: testsuite Description --- self.channels[3] is not hung up, causing the Asterisk graceful shutdown to timeout. This causes the test to fail under REF_DEBUG mode and prevents coverage from seeing the code executed by this test. Diffs - /asterisk/trunk/tests/bridge/bridge_action/bridge_action.py 5920 Diff: https://reviewboard.asterisk.org/r/4166/diff/ Testing --- Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/#review13777 --- /branches/13/main/asterisk.c https://reviewboard.asterisk.org/r/4182/#comment24262 Does this actually initialize 256 bytes of '\0', or just initialize the first byte? /branches/13/main/asterisk.c https://reviewboard.asterisk.org/r/4182/#comment24261 Space around '-'. Also why was the return removed? - Corey Farrell On Nov. 14, 2014, 10:12 a.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Nov. 14, 2014, 10:12 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs - /branches/13/main/asterisk.c 427813 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4170: testsuite: Delete bridges on completion for a bunch of rest_api tests
On Nov. 13, 2014, 11:06 a.m., Joshua Colp wrote: Ship It! I have to look into this again now that kmoore has committed testsuite revision 5921, many of these changes use the StasisEnd event to do cleanup. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4170/#review13743 --- On Nov. 11, 2014, 5:41 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4170/ --- (Updated Nov. 11, 2014, 5:41 p.m.) Review request for Asterisk Developers. Repository: testsuite Description --- ARI users are responsible for deleting bridges when they are no longer needed. This change deletes bridges at the appropriate time, allowing these tests to pass with REF_DEBUG enabled. Diffs - /asterisk/trunk/tests/rest_api/channels/snoop_spy/test-config.yaml 5920 /asterisk/trunk/tests/rest_api/channels/snoop_spy/channel_spy.py 5920 /asterisk/trunk/tests/rest_api/bridges/unhappy/bridge_unhappy.py 5920 /asterisk/trunk/tests/rest_api/bridges/move/bridge_move.py 5920 /asterisk/trunk/tests/rest_api/bridges/happy/test-config.yaml 5920 /asterisk/trunk/tests/rest_api/bridges/happy/bridge_happy.py 5920 /asterisk/trunk/tests/rest_api/bridges/hangup/test-config.yaml 5920 /asterisk/trunk/tests/rest_api/bridges/hangup/bridge_hangup.py 5920 /asterisk/trunk/tests/rest_api/bridges/add_recording_channel/add_recording_channel.py 5920 /asterisk/trunk/tests/rest_api/applications/subscribe-bridge/subscribe_bridge.py 5920 Diff: https://reviewboard.asterisk.org/r/4170/diff/ Testing --- Against 13 Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/#review13751 --- Could this be solved by: read(ast_consock, buf, sizeof(buf) - 1) ? read() operates on buffers not strings, so it has no concept of null termination. Also I think we need to either memset(buf, 0, sizeof(buf)) before the read, or (better) use the result of read to set the NULL terminator. It looks like we're currently relying on buf to be zero filled before read() when it's actually uninitialized data. I don't have a specific problem with increasing the buffer from 80 to 256, but I think we need to fix the parser so it doesn't crash if given 256 or more bytes. I think this issue applies to 11+. - Corey Farrell On Nov. 13, 2014, 3:31 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Nov. 13, 2014, 3:31 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs - /branches/13/main/asterisk.c 427813 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4166: testsuite: tests/bridge/bridge_action leaves a channel open
On Nov. 13, 2014, 11:30 a.m., Mark Michelson wrote: With the fix being made to the leaked bridge in Asterisk, is this change still required? Does hanging up self.channels[1] not result in self.channels[3] and the bridge being destroyed as expected? Still required, I'm guessing that when the first channel hangs up it leaves the second in a single user bridge. I'm not sure if this is a bug in Asterisk or just the way it works. The XMLDOC doesn't specify. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4166/#review13745 --- On Nov. 11, 2014, 3:37 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4166/ --- (Updated Nov. 11, 2014, 3:37 p.m.) Review request for Asterisk Developers. Repository: testsuite Description --- self.channels[3] is not hung up, causing the Asterisk graceful shutdown to timeout. This causes the test to fail under REF_DEBUG mode and prevents coverage from seeing the code executed by this test. Diffs - /asterisk/trunk/tests/bridge/bridge_action/bridge_action.py 5920 Diff: https://reviewboard.asterisk.org/r/4166/diff/ Testing --- Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4166: testsuite: tests/bridge/bridge_action leaves a channel open
On Nov. 13, 2014, 11:30 a.m., Mark Michelson wrote: With the fix being made to the leaked bridge in Asterisk, is this change still required? Does hanging up self.channels[1] not result in self.channels[3] and the bridge being destroyed as expected? Corey Farrell wrote: Still required, I'm guessing that when the first channel hangs up it leaves the second in a single user bridge. I'm not sure if this is a bug in Asterisk or just the way it works. The XMLDOC doesn't specify. Actually on further inspection it appears the bridge is destroyed, and the caller is sent back to the previous Dialplan location. core show channels without hangup of self.channels[3]: Channel Location State Application(Data) Local/waiting_area@d (None) Up Echo() Local/waiting_area@d (None) Up Echo() 2 active channels 2 active calls 14 calls processed If I then run 'channel request hangup Local/waiting_area@default-0003;1' (or hangup ;2), the test finishes and passes leak free. This is strange to me, why all the other channel pairs were hung up but not this one. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4166/#review13745 --- On Nov. 11, 2014, 3:37 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4166/ --- (Updated Nov. 11, 2014, 3:37 p.m.) Review request for Asterisk Developers. Repository: testsuite Description --- self.channels[3] is not hung up, causing the Asterisk graceful shutdown to timeout. This causes the test to fail under REF_DEBUG mode and prevents coverage from seeing the code executed by this test. Diffs - /asterisk/trunk/tests/bridge/bridge_action/bridge_action.py 5920 Diff: https://reviewboard.asterisk.org/r/4166/diff/ Testing --- Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4151: Fix compiler error when using ./configure --enable-dev-mode --enable-coverage
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4151/ --- (Updated Nov. 12, 2014, 7:44 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 427682 Bugs: ASTERISK-24502 https://issues.asterisk.org/jira/browse/ASTERISK-24502 Repository: Asterisk Description --- When coverage and dev-mode are enabled with DONT_OPTIMIZE, it causes build failure. The double compilation does a 'shadow' build of each file with output to /dev/null. Unfortunately when coverage is enabled, GCC tries writing to /dev/null.gcno (at least some versions do). This prevents the build from proceeding. This change prevents coverage from being enabled for the shadow build of all files. This involves using a separate variable to hold the CFLAGS for coverage, and adding it to the commands for all real builds. Diffs - /branches/11/Makefile.rules 427380 Diff: https://reviewboard.asterisk.org/r/4151/diff/ Testing --- Build with these options now works in Linux Mint 17 (gcc 4.8.2-19ubuntu1). Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4160: chan_sip: Fix theoretical leak of p-refer
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4160/ --- (Updated Nov. 12, 2014, 3:28 p.m.) Review request for Asterisk Developers. Changes --- Make sip_refer_alloc destroy any previous p-refer. Bugs: ASTERISK-15242 https://issues.asterisk.org/jira/browse/ASTERISK-15242 Repository: Asterisk Description --- If transmit_refer is called when p-refer is already allocated, it will leak the previous allocation. I checked for all occurrences of sip_refer_alloc, found that transmit_refer was the only caller that didn't check p-refer first. This change moves the check for !p-refer to sip_refer_alloc. I made transmit_refer destroy any previous p-refer so it will have a clean structure after reallocation like it does currently. Unsure if it's needed, but the little bit of extra processing is worth keeping this fix low risk. The change is slightly different in 12+, as p-refer-refer_call only exists in 11. Diffs (updated) - /branches/11/channels/chan_sip.c 427685 Diff: https://reviewboard.asterisk.org/r/4160/diff/ Testing (updated) --- tests/channels/SIP against 11 Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4159: chan_sip: Add missing braces on if, else, while and for
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4159/ --- (Updated Nov. 11, 2014, 2:17 p.m.) Status -- This change has been discarded. Review request for Asterisk Developers. Bugs: ASTERISK-24510 https://issues.asterisk.org/jira/browse/ASTERISK-24510 Repository: Asterisk Description --- Thank you in advance to anyone who takes the time to review this tedious patch. It is large but benign. Scanned for / fixed instances of the regex search pattern (else|(if|while|for)\s*\(.*\))(\s*/\*.*\*/|)\s*$ in channels/chan_sip.c and channels/sip/*.c. I've proposed this change for 13 and trunk. The set of braces for one 'if' statement was excluded from this change to allow the entire patch to apply cleanly to 13 and trunk with patch fuzz level 0. If you approve this change please note if you are approving it for 13+, or just trunk. I was late to AstriDevCon so I'm not sure if this is acceptable to 13, figured it doesn't hurt to ask. Asterisk 13 will be supported for a long time and chan_sip is in extended support status. I feel that adding the braces will make it easier/lower risk for me to fix bugs that are found in the next 4 years. I have no idea what reviewboard is doing with the display for lines 6078 - 6254, no code is actually moved. Check the diff file for the actual changes in that section. Diffs - /branches/13/channels/sip/reqresp_parser.c 427641 /branches/13/channels/sip/dialplan_functions.c 427641 /branches/13/channels/sip/config_parser.c 427641 /branches/13/channels/chan_sip.c 427641 Diff: https://reviewboard.asterisk.org/r/4159/diff/ Testing --- Compiled, visually inspected. Ran all of tests/channel/SIP in testsuite. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4160: chan_sip: Fix theoretical leak of p-refer
On Nov. 11, 2014, 1:43 p.m., Mark Michelson wrote: If I understand the purpose of p-refer correctly, it's supposed to be details relating to a specific REFER (or REFER-esque in some cases) transaction. I think that any in-dialog places where p-refer may be allocated, the previous p-refer should be freed. In addition to the transmit_refer() change you have, this would mean that handle_request_refer() and get_also_info() should free p-refer and then allocate a new one. Honestly, the best way to do this is perhaps to just have sip_refer_alloc() destroy the old p-refer and then allocate a new one. This seams reasonable to me. Now that this will change existing behaviour, I want to run it through testsuite tests/channels/SIP. Once that passes I'll update the review. Unless I'm misunderstanding, the call to sip_refer_alloc from handle_request_invite should destroy any existing p-refer as well? - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4160/#review13722 --- On Nov. 10, 2014, 1:25 a.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4160/ --- (Updated Nov. 10, 2014, 1:25 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-15242 https://issues.asterisk.org/jira/browse/ASTERISK-15242 Repository: Asterisk Description --- If transmit_refer is called when p-refer is already allocated, it will leak the previous allocation. I checked for all occurrences of sip_refer_alloc, found that transmit_refer was the only caller that didn't check p-refer first. This change moves the check for !p-refer to sip_refer_alloc. I made transmit_refer destroy any previous p-refer so it will have a clean structure after reallocation like it does currently. Unsure if it's needed, but the little bit of extra processing is worth keeping this fix low risk. The change is slightly different in 12+, as p-refer-refer_call only exists in 11. Diffs - /branches/11/channels/chan_sip.c 427666 Diff: https://reviewboard.asterisk.org/r/4160/diff/ Testing --- Compiled, visual inspection. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4166: testsuite: tests/bridge/bridge_action leaves a channel open
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4166/ --- Review request for Asterisk Developers. Repository: testsuite Description --- self.channels[3] is not hung up, causing the Asterisk graceful shutdown to timeout. This causes the test to fail under REF_DEBUG mode and prevents coverage from seeing the code executed by this test. Diffs - /asterisk/trunk/tests/bridge/bridge_action/bridge_action.py 5920 Diff: https://reviewboard.asterisk.org/r/4166/diff/ Testing --- Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4170: testsuite: Delete bridges on completion for a bunch of rest_api tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4170/ --- Review request for Asterisk Developers. Repository: testsuite Description --- ARI users are responsible for deleting bridges when they are no longer needed. This change deletes bridges at the appropriate time, allowing these tests to pass with REF_DEBUG enabled. Diffs - /asterisk/trunk/tests/rest_api/channels/snoop_spy/test-config.yaml 5920 /asterisk/trunk/tests/rest_api/channels/snoop_spy/channel_spy.py 5920 /asterisk/trunk/tests/rest_api/bridges/unhappy/bridge_unhappy.py 5920 /asterisk/trunk/tests/rest_api/bridges/move/bridge_move.py 5920 /asterisk/trunk/tests/rest_api/bridges/happy/test-config.yaml 5920 /asterisk/trunk/tests/rest_api/bridges/happy/bridge_happy.py 5920 /asterisk/trunk/tests/rest_api/bridges/hangup/test-config.yaml 5920 /asterisk/trunk/tests/rest_api/bridges/hangup/bridge_hangup.py 5920 /asterisk/trunk/tests/rest_api/bridges/add_recording_channel/add_recording_channel.py 5920 /asterisk/trunk/tests/rest_api/applications/subscribe-bridge/subscribe_bridge.py 5920 Diff: https://reviewboard.asterisk.org/r/4170/diff/ Testing --- Against 13 Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4151: Fix compiler error when using ./configure --enable-dev-mode --enable-coverage
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4151/#review13714 --- As part of this change I am adding *.gcno and *.gcna to svn:ignore for the following folders: apps/confbridge/ codecs/ilbc/ codecs/speex/ tests/ - Corey Farrell On Nov. 8, 2014, 1:29 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4151/ --- (Updated Nov. 8, 2014, 1:29 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24502 https://issues.asterisk.org/jira/browse/ASTERISK-24502 Repository: Asterisk Description --- When coverage and dev-mode are enabled with DONT_OPTIMIZE, it causes build failure. The double compilation does a 'shadow' build of each file with output to /dev/null. Unfortunately when coverage is enabled, GCC tries writing to /dev/null.gcno (at least some versions do). This prevents the build from proceeding. This change prevents coverage from being enabled for the shadow build of all files. This involves using a separate variable to hold the CFLAGS for coverage, and adding it to the commands for all real builds. Diffs - /branches/11/Makefile.rules 427380 Diff: https://reviewboard.asterisk.org/r/4151/diff/ Testing --- Build with these options now works in Linux Mint 17 (gcc 4.8.2-19ubuntu1). Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4159: chan_sip: Add missing braces on if, else, while and for
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4159/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24510 https://issues.asterisk.org/jira/browse/ASTERISK-24510 Repository: Asterisk Description --- Thank you in advance to anyone who takes the time to review this tedious patch. It is large but benign. Scanned for / fixed instances of the regex search pattern (else|(if|while|for)\s*\(.*\))(\s*/\*.*\*/|)\s*$ in channels/chan_sip.c and channels/sip/*.c. I've proposed this change for 13 and trunk. The set of braces for one 'if' statement was excluded from this change to allow the entire patch to apply cleanly to 13 and trunk with patch fuzz level 0. If you approve this change please note if you are approving it for 13+, or just trunk. I was late to AstriDevCon so I'm not sure if this is acceptable to 13, figured it doesn't hurt to ask. Asterisk 13 will be supported for a long time and chan_sip is in extended support status. I feel that adding the braces will make it easier/lower risk for me to fix bugs that are found in the next 4 years. I have no idea what reviewboard is doing with the display for lines 6078 - 6254, no code is actually moved. Check the diff file for the actual changes in that section. Diffs - /branches/13/channels/sip/reqresp_parser.c 427641 /branches/13/channels/sip/dialplan_functions.c 427641 /branches/13/channels/sip/config_parser.c 427641 /branches/13/channels/chan_sip.c 427641 Diff: https://reviewboard.asterisk.org/r/4159/diff/ Testing --- Compiled, visually inspected. Ran all of tests/channel/SIP in testsuite. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4160: chan_sip: Fix theoretical leak of p-refer
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4160/ --- Review request for Asterisk Developers. Bugs: ASTERISK-15242 https://issues.asterisk.org/jira/browse/ASTERISK-15242 Repository: Asterisk Description --- If transmit_refer is called when p-refer is already allocated, it will leak the previous allocation. I checked for all occurrences of sip_refer_alloc, found that transmit_refer was the only caller that didn't check p-refer first. This change moves the check for !p-refer to sip_refer_alloc. I made transmit_refer destroy any previous p-refer so it will have a clean structure after reallocation like it does currently. Unsure if it's needed, but the little bit of extra processing is worth keeping this fix low risk. The change is slightly different in 12+, as p-refer-refer_call only exists in 11. Diffs - /branches/11/channels/chan_sip.c 427666 Diff: https://reviewboard.asterisk.org/r/4160/diff/ Testing --- Compiled, visual inspection. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4151: Fix compiler error when using ./configure --enable-dev-mode --enable-coverage
On Nov. 6, 2014, 6:04 a.m., wdoekes wrote: Isn't the better fix to disable coverage for the shadow compilation? Corey Farrell wrote: Probably. I have updated code but I won't have time to test until this weekend. I'll update the review once I've had a chance to do some builds with it. Matt Jordan wrote: I'm kind of surprised by this, since the build agents compile with --enable-dev-mode and --enable-coverage: https://bamboo.asterisk.org/bamboo/browse/AST-ATSF-410/artifact/C632TE/Coverage/coverage.tar.gz I agree, it must be some difference between versions of GCC. Or if the build agents run make as root, maybe you'll find that you have a /dev/null.gcno file after the build :) - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4151/#review13701 --- On Nov. 6, 2014, 5:06 a.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4151/ --- (Updated Nov. 6, 2014, 5:06 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24502 https://issues.asterisk.org/jira/browse/ASTERISK-24502 Repository: Asterisk Description --- When coverage and dev-mode are enabled with DONT_OPTIMIZE, it causes build failure. The double compilation does a 'shadow' build of each file with output to /dev/null. Unfortunately when coverage is enabled, GCC tries writing to /dev/null.gcno (at least some versions do). This prevents the build from proceeding. The simple fix is to simply prevent COMPILE_DOUBLE when coverage is enabled, that is what I'm proposing here. The other option would be to use a real output location instead of /dev/null, delete the file immediately after building. I'm not sure that is needed, so I've proposed the simpler fix. Diffs - /branches/11/Makefile.rules 427380 Diff: https://reviewboard.asterisk.org/r/4151/diff/ Testing --- Build with these options now works in Linux Mint 17 (gcc 4.8.2-19ubuntu1). Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4152: chan_console: Fix reference leaks to pvt
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4152/ --- (Updated Nov. 8, 2014, 11:28 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 427554 Bugs: ASTERISK-24504 https://issues.asterisk.org/jira/browse/ASTERISK-24504 Repository: Asterisk Description --- Fix a bunch of calls to get_active_pvt where the reference is never released. Leaks found by Bamboo https://bamboo.asterisk.org/bamboo/artifact/AST-ATSRC1/C632TE/build-2/Test-Suite-Logs/apps/queues/queue_baseline/run_1/ast1/var/log/asterisk/refs.txt Diffs - /branches/11/channels/chan_console.c 427380 Diff: https://reviewboard.asterisk.org/r/4152/diff/ Testing --- No. Visually inspected changes, they are straight forward. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4151: Fix compiler error when using ./configure --enable-dev-mode --enable-coverage
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4151/ --- (Updated Nov. 8, 2014, 1:29 p.m.) Review request for Asterisk Developers. Changes --- Disable coverage for shadow compilation. This stops GCC from attempting to create /dev/null.gcno. Bugs: ASTERISK-24502 https://issues.asterisk.org/jira/browse/ASTERISK-24502 Repository: Asterisk Description (updated) --- When coverage and dev-mode are enabled with DONT_OPTIMIZE, it causes build failure. The double compilation does a 'shadow' build of each file with output to /dev/null. Unfortunately when coverage is enabled, GCC tries writing to /dev/null.gcno (at least some versions do). This prevents the build from proceeding. This change prevents coverage from being enabled for the shadow build of all files. This involves using a separate variable to hold the CFLAGS for coverage, and adding it to the commands for all real builds. Diffs (updated) - /branches/11/Makefile.rules 427380 Diff: https://reviewboard.asterisk.org/r/4151/diff/ Testing --- Build with these options now works in Linux Mint 17 (gcc 4.8.2-19ubuntu1). Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4153: manager: HTTP connections leak references
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4153/ --- (Updated Nov. 9, 2014, 1:56 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 427641 Bugs: ASTERISK-24505 https://issues.asterisk.org/jira/browse/ASTERISK-24505 Repository: Asterisk Description --- Fix reference leak that happens if (session !blastaway). Diffs - /branches/11/main/manager.c 427380 Diff: https://reviewboard.asterisk.org/r/4153/diff/ Testing --- tests/manager/config/basic against 13. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4114: Prevent stringfields from accumulating unused memory
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4114/ --- (Updated Nov. 6, 2014, 3:05 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 427380 Bugs: ASTERISK-24307 https://issues.asterisk.org/jira/browse/ASTERISK-24307 Repository: Asterisk Description --- Any time a stringfield is blanked it currently prevents any currently allocated memory from being freed. If a stringfield is repeatedly set to blank then set to a non-blank value, it causes new pools to be continuously allocated and never freed. I'm unsure if the loop can be optimized, maybe the break can be re-added to the original location on the condition that ptr == __ast_string_field_empty? Diffs - /branches/11/main/utils.c 427111 /branches/11/include/asterisk/stringfields.h 427111 Diff: https://reviewboard.asterisk.org/r/4114/diff/ Testing --- Manual test using https://github.com/elessard1/asterisk-lab/blob/master/examples/lab_stringfields_leak.c to verify that old pools are now freed. Full testsuite against Asterisk 13. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4150: res_hep: fix major leak that occurs when config is missing or enabled=no
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4150/ --- (Updated Nov. 6, 2014, 3:22 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 427400 Bugs: ASTERISK-24491 https://issues.asterisk.org/jira/browse/ASTERISK-24491 Repository: Asterisk Description --- Add missing unref to hepv3_send_packet. Diffs - /branches/13/res/res_hep.c 427298 Diff: https://reviewboard.asterisk.org/r/4150/diff/ Testing --- Tested by Zane Conkle. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4151: Fix compiler error when using ./configure --enable-dev-mode --enable-coverage
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4151/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24502 https://issues.asterisk.org/jira/browse/ASTERISK-24502 Repository: Asterisk Description --- When coverage and dev-mode are enabled with DONT_OPTIMIZE, it causes build failure. The double compilation does a 'shadow' build of each file with output to /dev/null. Unfortunately when coverage is enabled, GCC tries writing to /dev/null.gcno (at least some versions do). This prevents the build from proceeding. The simple fix is to simply prevent COMPILE_DOUBLE when coverage is enabled, that is what I'm proposing here. The other option would be to use a real output location instead of /dev/null, delete the file immediately after building. I'm not sure that is needed, so I've proposed the simpler fix. Diffs - /branches/11/Makefile.rules 427380 Diff: https://reviewboard.asterisk.org/r/4151/diff/ Testing --- Build with these options now works in Linux Mint 17 (gcc 4.8.2-19ubuntu1). Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4152: chan_console: Fix reference leaks to pvt
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4152/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24504 https://issues.asterisk.org/jira/browse/ASTERISK-24504 Repository: Asterisk Description --- Fix a bunch of calls to get_active_pvt where the reference is never released. Leaks found by Bamboo https://bamboo.asterisk.org/bamboo/artifact/AST-ATSRC1/C632TE/build-2/Test-Suite-Logs/apps/queues/queue_baseline/run_1/ast1/var/log/asterisk/refs.txt Diffs - /branches/11/channels/chan_console.c 427380 Diff: https://reviewboard.asterisk.org/r/4152/diff/ Testing --- No. Visually inspected changes, they are straight forward. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4152: chan_console: Fix reference leaks to pvt
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4152/ --- (Updated Nov. 6, 2014, 6:06 a.m.) Review request for Asterisk Developers. Changes --- Add missing braces. Bugs: ASTERISK-24504 https://issues.asterisk.org/jira/browse/ASTERISK-24504 Repository: Asterisk Description --- Fix a bunch of calls to get_active_pvt where the reference is never released. Leaks found by Bamboo https://bamboo.asterisk.org/bamboo/artifact/AST-ATSRC1/C632TE/build-2/Test-Suite-Logs/apps/queues/queue_baseline/run_1/ast1/var/log/asterisk/refs.txt Diffs (updated) - /branches/11/channels/chan_console.c 427380 Diff: https://reviewboard.asterisk.org/r/4152/diff/ Testing --- No. Visually inspected changes, they are straight forward. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4151: Fix compiler error when using ./configure --enable-dev-mode --enable-coverage
On Nov. 6, 2014, 6:04 a.m., wdoekes wrote: Isn't the better fix to disable coverage for the shadow compilation? Probably. I have updated code but I won't have time to test until this weekend. I'll update the review once I've had a chance to do some builds with it. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4151/#review13701 --- On Nov. 6, 2014, 5:06 a.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4151/ --- (Updated Nov. 6, 2014, 5:06 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24502 https://issues.asterisk.org/jira/browse/ASTERISK-24502 Repository: Asterisk Description --- When coverage and dev-mode are enabled with DONT_OPTIMIZE, it causes build failure. The double compilation does a 'shadow' build of each file with output to /dev/null. Unfortunately when coverage is enabled, GCC tries writing to /dev/null.gcno (at least some versions do). This prevents the build from proceeding. The simple fix is to simply prevent COMPILE_DOUBLE when coverage is enabled, that is what I'm proposing here. The other option would be to use a real output location instead of /dev/null, delete the file immediately after building. I'm not sure that is needed, so I've proposed the simpler fix. Diffs - /branches/11/Makefile.rules 427380 Diff: https://reviewboard.asterisk.org/r/4151/diff/ Testing --- Build with these options now works in Linux Mint 17 (gcc 4.8.2-19ubuntu1). Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4153: manager: HTTP connections leak references
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4153/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24505 https://issues.asterisk.org/jira/browse/ASTERISK-24505 Repository: Asterisk Description --- Fix reference leak that happens if (session !blastaway). Diffs - /branches/11/main/manager.c 427380 Diff: https://reviewboard.asterisk.org/r/4153/diff/ Testing --- tests/manager/config/basic against 13. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4149: main/file.c: fix possible extra ast_module_unref to format modules
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4149/ --- (Updated Nov. 6, 2014, 6:10 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 427464 Bugs: ASTERISK-24492 https://issues.asterisk.org/jira/browse/ASTERISK-24492 Repository: Asterisk Description --- fn_wrapper only adds a reference to the format's module if the file was able to be opened. If not this causes an unmatched ast_module_unref in filestream_destructor. Diffs - /branches/11/main/file.c 427255 Diff: https://reviewboard.asterisk.org/r/4149/diff/ Testing --- Verified the issue and fix with tests/apps/voicemail/play_message + r4141. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3603: func_jitterbuffer: fix audio failure caused by certain masquerade's
On Nov. 5, 2014, 7:55 a.m., Joshua Colp wrote: Was the behavior for this ever brought up on the -dev list? I don't remember it so I don't think so. Before accepting the behavior you've done I'd really like us to talk about it there amongst everyone. I really think it's the reverse of what a user would expect to happen when a masquerade happens. http://lists.digium.com/pipermail/asterisk-dev/2014-October/071154.html The only response I received was on IRC from Richard, who said it was a well written email. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3603/#review13685 --- On Oct. 30, 2014, 8:06 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3603/ --- (Updated Oct. 30, 2014, 8:06 p.m.) Review request for Asterisk Developers, Joshua Colp and Matt Jordan. Bugs: ASTERISK-22409 https://issues.asterisk.org/jira/browse/ASTERISK-22409 Repository: Asterisk Description --- During masquerade it is possible for the AST_JITTERBUFFER_FD to be cleared (set to -1). This change adds a check when copying channel fd's to prevent clearing an FD with -1. This seems to resolve the bad audio quality experienced after the masquerade. When AST_JITTERBUFFER_FD was set to -1, this prevented the channel from polling that timer. This caused RTP packets to be received late, and discarded. Diffs - /branches/11/main/channel.c 426593 /branches/11/funcs/func_jitterbuffer.c 426593 Diff: https://reviewboard.asterisk.org/r/3603/diff/ Testing --- Verified the scenario outlined in ASTERISK-22409 no longer experiences audio quality loss. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4140: res_http_websockets: Module reference decrease below zero
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4140/ --- (Updated Nov. 4, 2014, 1:30 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 427200 Bugs: ASTERISK-24480 https://issues.asterisk.org/jira/browse/ASTERISK-24480 Repository: Asterisk Description --- In Asterisk 12+ websocket_add_protocol_internal is used to add the echo protocol, but ast_websocket_remove_protocol is used to remove it. This causes an extra call to ast_module_unref. Diffs - /branches/12/res/res_http_websocket.c 426831 Diff: https://reviewboard.asterisk.org/r/4140/diff/ Testing --- Found/tested with r4141. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4143: testsuite: Close HTTP connections when test is complete to prevent reported leaks in tests/phoneprov/res_phoneprov and tests/manager/config
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4143/ --- (Updated Nov. 4, 2014, 2:57 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 5887 Repository: testsuite Description --- HTTP connections left open when a test completes causes leaks to be reported by REF_DEBUG. This ensures the connections are closed in two tests to ensure they pass with REF_DEBUG. Remove import of syncami from tests/channels/SIP/pcap_demo since it's not actually used. Diffs - /asterisk/trunk/tests/phoneprov/res_phoneprov/run-test 5878 /asterisk/trunk/tests/manager/config/ManagerConfigTest.py 5878 /asterisk/trunk/tests/channels/SIP/pcap_demo/run-test 5878 /asterisk/trunk/lib/python/asterisk/syncami.py 5878 Diff: https://reviewboard.asterisk.org/r/4143/diff/ Testing --- Against 13, tests passed with no leaks reported. Verified no other active test uses syncami. Only tests/channels/SIP/nat_supertest used it but that test is disabled per ASTERISK-19565. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4149: main/file.c: fix possible extra ast_module_unref to format modules
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4149/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24492 https://issues.asterisk.org/jira/browse/ASTERISK-24492 Repository: Asterisk Description --- fn_wrapper only adds a reference to the format's module if the file was able to be opened. If not this causes an unmatched ast_module_unref in filestream_destructor. Diffs - /branches/11/main/file.c 427255 Diff: https://reviewboard.asterisk.org/r/4149/diff/ Testing --- Verified the issue and fix with tests/apps/voicemail/play_message + r4141. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4150: res_hep: fix major leak that occurs when config is missing or enabled=no
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4150/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24491 https://issues.asterisk.org/jira/browse/ASTERISK-24491 Repository: Asterisk Description --- Add missing unref to hepv3_send_packet. Diffs - /branches/13/res/res_hep.c 427298 Diff: https://reviewboard.asterisk.org/r/4150/diff/ Testing --- Tested by Zane Conkle. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4114: Prevent stringfields from accumulating unused memory
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4114/ --- (Updated Nov. 3, 2014, 8:29 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24307 https://issues.asterisk.org/jira/browse/ASTERISK-24307 Repository: Asterisk Description --- Any time a stringfield is blanked it currently prevents any currently allocated memory from being freed. If a stringfield is repeatedly set to blank then set to a non-blank value, it causes new pools to be continuously allocated and never freed. I'm unsure if the loop can be optimized, maybe the break can be re-added to the original location on the condition that ptr == __ast_string_field_empty? Diffs (updated) - /branches/11/main/utils.c 427111 /branches/11/include/asterisk/stringfields.h 427111 Diff: https://reviewboard.asterisk.org/r/4114/diff/ Testing --- Manual test using https://github.com/elessard1/asterisk-lab/blob/master/examples/lab_stringfields_leak.c to verify that old pools are now freed. Full testsuite against Asterisk 13. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4114: Prevent stringfields from accumulating unused memory
On Oct. 29, 2014, 7:55 p.m., rmudgett wrote: 1) In stringfields.h:ast_string_field_ptr_set_by_fields(), the __p__ and ptr pointers are the same by initialization so the test for *__p__ != *ptr is always false and will not release the old string value when __ast_string_field_alloc_space() allocates space for the new string value. I think this is the primary leak. 2) In utils.c:__ast_string_field_ptr_grow(), the increase of pool-used doesn't seem right. It should be increased to keep alignment similar to utils.c:__ast_string_field_alloc_space(). 3) I think a check needs to be added to utils.c:__ast_string_field_ptr_build_va() for the case when the string created by vsnprintf() is empty so the pool string can be set to the constant __ast_string_field_empty pointer. (Like is done in stringfields.h:ast_string_field_ptr_set_by_fields()) 4) All of these fixes would apply to v1.8 as well. 1) I think I've fixed this. 2) To be honest I'm not sure how to fix this. 3) Fixed. 4) If this review is complete before the final 1.8 bug fix release. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4114/#review13628 --- On Nov. 3, 2014, 8:29 a.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4114/ --- (Updated Nov. 3, 2014, 8:29 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24307 https://issues.asterisk.org/jira/browse/ASTERISK-24307 Repository: Asterisk Description --- Any time a stringfield is blanked it currently prevents any currently allocated memory from being freed. If a stringfield is repeatedly set to blank then set to a non-blank value, it causes new pools to be continuously allocated and never freed. I'm unsure if the loop can be optimized, maybe the break can be re-added to the original location on the condition that ptr == __ast_string_field_empty? Diffs - /branches/11/main/utils.c 427111 /branches/11/include/asterisk/stringfields.h 427111 Diff: https://reviewboard.asterisk.org/r/4114/diff/ Testing --- Manual test using https://github.com/elessard1/asterisk-lab/blob/master/examples/lab_stringfields_leak.c to verify that old pools are now freed. Full testsuite against Asterisk 13. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4114: Prevent stringfields from accumulating unused memory
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4114/ --- (Updated Nov. 3, 2014, 12:55 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24307 https://issues.asterisk.org/jira/browse/ASTERISK-24307 Repository: Asterisk Description --- Any time a stringfield is blanked it currently prevents any currently allocated memory from being freed. If a stringfield is repeatedly set to blank then set to a non-blank value, it causes new pools to be continuously allocated and never freed. I'm unsure if the loop can be optimized, maybe the break can be re-added to the original location on the condition that ptr == __ast_string_field_empty? Diffs (updated) - /branches/11/main/utils.c 427111 /branches/11/include/asterisk/stringfields.h 427111 Diff: https://reviewboard.asterisk.org/r/4114/diff/ Testing --- Manual test using https://github.com/elessard1/asterisk-lab/blob/master/examples/lab_stringfields_leak.c to verify that old pools are now freed. Full testsuite against Asterisk 13. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4114: Prevent stringfields from accumulating unused memory
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4114/ --- (Updated Nov. 3, 2014, 1:53 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24307 https://issues.asterisk.org/jira/browse/ASTERISK-24307 Repository: Asterisk Description --- Any time a stringfield is blanked it currently prevents any currently allocated memory from being freed. If a stringfield is repeatedly set to blank then set to a non-blank value, it causes new pools to be continuously allocated and never freed. I'm unsure if the loop can be optimized, maybe the break can be re-added to the original location on the condition that ptr == __ast_string_field_empty? Diffs (updated) - /branches/11/main/utils.c 427111 /branches/11/include/asterisk/stringfields.h 427111 Diff: https://reviewboard.asterisk.org/r/4114/diff/ Testing --- Manual test using https://github.com/elessard1/asterisk-lab/blob/master/examples/lab_stringfields_leak.c to verify that old pools are now freed. Full testsuite against Asterisk 13. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4114: Prevent stringfields from accumulating unused memory
On Nov. 3, 2014, 1:12 p.m., rmudgett wrote: Minor nit. Does the patch still fix the reporter's memory leak? Mostly, yes (as much as the original patch). I'm not sure it can be fixed completely. I tested with the reporters sip.conf. Each peer originally allocated 2000 bytes for the stringfield's. After 37 iterations of 'sip reload' the stringfield of all 500 peers added a new pool (allocating 8144 bytes each). The original allocations are never freed, however the memory never grows beyond the 2nd allocation (tested to 2000 reload iterations). At least one field in the original pool is not being blanked during reload, causing the original memory pool to never free. The existing code causes the memory usage to increase indefinitely, so this is a major improvement. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4114/#review13658 --- On Nov. 3, 2014, 12:55 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4114/ --- (Updated Nov. 3, 2014, 12:55 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24307 https://issues.asterisk.org/jira/browse/ASTERISK-24307 Repository: Asterisk Description --- Any time a stringfield is blanked it currently prevents any currently allocated memory from being freed. If a stringfield is repeatedly set to blank then set to a non-blank value, it causes new pools to be continuously allocated and never freed. I'm unsure if the loop can be optimized, maybe the break can be re-added to the original location on the condition that ptr == __ast_string_field_empty? Diffs - /branches/11/main/utils.c 427111 /branches/11/include/asterisk/stringfields.h 427111 Diff: https://reviewboard.asterisk.org/r/4114/diff/ Testing --- Manual test using https://github.com/elessard1/asterisk-lab/blob/master/examples/lab_stringfields_leak.c to verify that old pools are now freed. Full testsuite against Asterisk 13. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4141: Enable REF_DEBUG for ast_module_ref / ast_module_unref
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4141/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24479 https://issues.asterisk.org/jira/browse/ASTERISK-24479 Repository: Asterisk Description --- This change includes an ABI change with compatibility stubs for 11, 12 and 13. The compatibility stubs will not be included in trunk. The point of this change is to have each module create an AO2 object on load, and hopefully destroy it on unload. This allows module reference count errors to be debugged through REF_DEBUG. When REF_DEBUG is enabled: * adds an empty ao2 object to 'struct ast_module' * Allocate ao2 when the module is loaded * Perform an ao2_ref in each place where mod-usecount is manipulated. * ao2_cleanup on module unload. The passthrough of file, line and func is needed for the REF_DEBUG to be of any use, so without the ABI changes this is not useful. The change to bridge_builtin_features.c ensures that the module cannot be manually unloaded, but is able to be unloaded during ast_module_shutdown. Note ast_module_shutdown only happens during clean shutdown and does not actually run dlclose so this is safe. Diffs - /branches/11/main/loader.c 426830 /branches/11/include/asterisk/module.h 426830 /branches/11/bridges/bridge_builtin_features.c 426830 Diff: https://reviewboard.asterisk.org/r/4141/diff/ Testing --- Using tests/manager/originate with REF_DEBUG enabled. When the change to bridge_builtin_features.c is omitted the test fails due to that one reference leak. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4140: res_http_websockets: Module reference decrease below zero
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4140/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24480 https://issues.asterisk.org/jira/browse/ASTERISK-24480 Repository: Asterisk Description --- In Asterisk 12+ websocket_add_protocol_internal is used to add the echo protocol, but ast_websocket_remove_protocol is used to remove it. This causes an extra call to ast_module_unref. Diffs - /branches/12/res/res_http_websocket.c 426831 Diff: https://reviewboard.asterisk.org/r/4140/diff/ Testing --- Found/tested with r4141. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] Fwd: [asterisk-commits] mjordan: branch 12 r426995 - /branches/12/res/res_stasis.c
If it's possible for apps_registry to be NULL then it's possible for the
condition to race. Maybe we need to use AO2_GLOBAL_OBJ_STATIC? Could this
issue apply to the other global containers in this module (app_controls,
app_bridges, app_bridges_moh, app_bridges_playback)?
-- Forwarded message --
From: SVN commits to the Asterisk project
asterisk-comm...@lists.digium.com
Date: Nov 1, 2014 9:01 PM
Subject: [asterisk-commits] mjordan: branch 12 r426995 -
/branches/12/res/res_stasis.c
To: asterisk-comm...@lists.digium.com, svn-comm...@lists.digium.com
Cc:
Author: mjordan
Date: Sat Nov 1 20:01:15 2014
New Revision: 426995
URL: http://svnview.digium.com/svn/asterisk?view=revrev=426995
Log:
res/res_stasis: Fix crash on module unload while performing operation
When the res_stasis module is unloaded, it will dispose of the apps_registry
container. This is a problem if an ARI operation is in flight that attempts
to use the registry, as the shutdown occurs in a separate thread. This patch
adds some sanity checks to the various routines that access the registry
which
cause the operations to fail if the apps_registry does not exist.
Crash caught by the Asterisk Test Suite.
Modified:
branches/12/res/res_stasis.c
Modified: branches/12/res/res_stasis.c
URL:
http://svnview.digium.com/svn/asterisk/branches/12/res/res_stasis.c?view=diffrev=426995r1=426994r2=426995
==
--- branches/12/res/res_stasis.c (original)
+++ branches/12/res/res_stasis.c Sat Nov 1 20:01:15 2014
@@ -1204,6 +1204,10 @@
*/
remove_stasis_end_published(chan);
+ if (!apps_registry) {
+ return -1;
+ }
+
app = ao2_find(apps_registry, app_name, OBJ_SEARCH_KEY);
if (!app) {
ast_log(LOG_ERROR,
@@ -1364,6 +1368,10 @@
{
RAII_VAR(struct stasis_app *, app, NULL, ao2_cleanup);
+ if (!apps_registry) {
+ return -1;
+ }
+
app = ao2_find(apps_registry, app_name, OBJ_SEARCH_KEY);
if (!app) {
/* XXX We can do a better job handling late binding,
queueing up
@@ -1381,6 +1389,10 @@
{
struct stasis_app *res = NULL;
+ if (!apps_registry) {
+ return NULL;
+ }
+
if (!ast_strlen_zero(app_name)) {
res = ao2_find(apps_registry, app_name, OBJ_SEARCH_KEY);
}
@@ -1405,6 +1417,10 @@
{
RAII_VAR(struct ao2_container *, apps, NULL, ao2_cleanup);
+ if (!apps_registry) {
+ return NULL;
+ }
+
apps = ast_str_container_alloc(1);
if (!apps) {
return NULL;
@@ -1419,8 +1435,11 @@
{
RAII_VAR(struct stasis_app *, app, NULL, ao2_cleanup);
- SCOPED_LOCK(apps_lock, apps_registry, ao2_lock, ao2_unlock);
-
+ if (!apps_registry) {
+ return -1;
+ }
+
+ ao2_lock(apps_registry);
app = ao2_find(apps_registry, app_name, OBJ_SEARCH_KEY |
OBJ_NOLOCK);
if (app) {
app_update(app, handler, data);
@@ -1429,6 +1448,7 @@
if (app) {
ao2_link_flags(apps_registry, app, OBJ_NOLOCK);
} else {
+ ao2_unlock(apps_registry);
return -1;
}
}
@@ -1437,6 +1457,7 @@
* prevent memory leaks, and we're lazy.
*/
cleanup();
+ ao2_unlock(apps_registry);
return 0;
}
@@ -1445,6 +1466,10 @@
RAII_VAR(struct stasis_app *, app, NULL, ao2_cleanup);
if (!app_name) {
+ return;
+ }
+
+ if (!apps_registry) {
return;
}
@@ -1837,6 +1862,7 @@
messaging_cleanup();
+ cleanup();
ao2_cleanup(apps_registry);
apps_registry = NULL;
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-commits mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-commits
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4128: func_jitterbuffer: fix frame leaks
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4128/ --- (Updated Nov. 2, 2014, 1:35 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 427019 Bugs: ASTERISK-22409 https://issues.asterisk.org/jira/browse/ASTERISK-22409 Repository: Asterisk Description --- These changes are not controversial and fix a memory leak so they are now split from r3603. In 12+ these changes apply to main/abstract_js.c instead of funcs/func_jitterbuffer.c. Diffs - /branches/11/main/abstract_jb.c 426593 /branches/11/funcs/func_jitterbuffer.c 426593 Diff: https://reviewboard.asterisk.org/r/4128/diff/ Testing --- tests/funcs/func_jitterbuffer against 13 no longer leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4138: main/app.c, app_voicemail: Fix ast_writestream leaks
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4138/ --- (Updated Nov. 2, 2014, 2:01 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 427023 Bugs: ASTERISK-24476 https://issues.asterisk.org/jira/browse/ASTERISK-24476 Repository: Asterisk Description --- Fix leak of ast_writestream recording_fs in app_voicemail:leave_voicemail. Fix cleanup in __ast_play_and_record where others[x] may be leaked. This was caught where prepend != NULL outmsg != NULL, once realfile[x] == NULL any further others[x] would be leaked. I also added a cleanup block for prepend != NULL outmsg == NULL. Note: this is a serious leak, a single ast_writestream is slightly over 64k. Diffs - /branches/11/main/app.c 426569 /branches/11/apps/app_voicemail.c 426569 Diff: https://reviewboard.asterisk.org/r/4138/diff/ Testing --- tests/apps/voicemail/check_voicemail_forward_with_prepend/ no longer leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4142: func_talkdetect: Fix stasis message leak in audiohook callback
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4142/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24482 https://issues.asterisk.org/jira/browse/ASTERISK-24482 Repository: Asterisk Description --- Add missing cleanup for stasis_message. Diffs - /branches/12/funcs/func_talkdetect.c 427023 Diff: https://reviewboard.asterisk.org/r/4142/diff/ Testing --- tests/funcs/func_talkdetect no longer leaks - tested against 13. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4143: testsuite: Close HTTP connections when test is complete to prevent reported leaks in tests/phoneprov/res_phoneprov and tests/manager/config
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4143/ --- Review request for Asterisk Developers. Repository: testsuite Description --- HTTP connections left open when a test completes causes leaks to be reported by REF_DEBUG. This ensures the connections are closed in two tests to ensure they pass with REF_DEBUG. Remove import of syncami from tests/channels/SIP/pcap_demo since it's not actually used. Diffs - /asterisk/trunk/tests/phoneprov/res_phoneprov/run-test 5878 /asterisk/trunk/tests/manager/config/ManagerConfigTest.py 5878 /asterisk/trunk/tests/channels/SIP/pcap_demo/run-test 5878 /asterisk/trunk/lib/python/asterisk/syncami.py 5878 Diff: https://reviewboard.asterisk.org/r/4143/diff/ Testing --- Against 13, tests passed with no leaks reported. Verified no other active test uses syncami. Only tests/channels/SIP/nat_supertest used it but that test is disabled per ASTERISK-19565. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4144: testsuite: Sort tests by name after loading
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4144/ --- Review request for Asterisk Developers. Repository: testsuite Description --- Minor change to runtests.py which causes the list of tests to be sorted alphabetically by name. This makes it much easier to look at the output of runtests.py and know how far along the run is. Diffs - /asterisk/trunk/runtests.py 5878 Diff: https://reviewboard.asterisk.org/r/4144/diff/ Testing --- Yes Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4138: main/app.c, app_voicemail: Fix ast_writestream leaks
On Oct. 31, 2014, 12:58 p.m., Mark Michelson wrote: /branches/1.8/apps/app_voicemail.c, line 5800 https://reviewboard.asterisk.org/r/4138/diff/1/?file=68670#file68670line5800 My assumption here is that you initially created this patch for 11, 12, or 13, and then backported to 1.8. This line doesn't fit in 1.8 because recording_fs does not exist there. The problem is, I don't know where in the 11+ code this ast_closestream() call is supposed to be. Well that's embarrassing. In 11+ this is needed in msg_create_from_file(), line 6023 in 11. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4138/#review13647 --- On Oct. 31, 2014, 1:26 a.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4138/ --- (Updated Oct. 31, 2014, 1:26 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24476 https://issues.asterisk.org/jira/browse/ASTERISK-24476 Repository: Asterisk Description --- Fix leak of ast_writestream recording_fs in app_voicemail:leave_voicemail. Fix cleanup in __ast_play_and_record where others[x] may be leaked. This was caught where prepend != NULL outmsg != NULL, once realfile[x] == NULL any further others[x] would be leaked. I also added a cleanup block for prepend != NULL outmsg == NULL. Note: this is a serious leak, a single ast_writestream is slightly over 64k. Diffs - /branches/1.8/main/app.c 426829 /branches/1.8/apps/app_voicemail.c 426829 Diff: https://reviewboard.asterisk.org/r/4138/diff/ Testing --- tests/apps/voicemail/check_voicemail_forward_with_prepend/ no longer leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4138: main/app.c, app_voicemail: Fix ast_writestream leaks
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4138/ --- (Updated Oct. 31, 2014, 1:15 p.m.) Review request for Asterisk Developers. Changes --- Use v11 patch to show where change in app_voicemail will go (11+ only). Bugs: ASTERISK-24476 https://issues.asterisk.org/jira/browse/ASTERISK-24476 Repository: Asterisk Description --- Fix leak of ast_writestream recording_fs in app_voicemail:leave_voicemail. Fix cleanup in __ast_play_and_record where others[x] may be leaked. This was caught where prepend != NULL outmsg != NULL, once realfile[x] == NULL any further others[x] would be leaked. I also added a cleanup block for prepend != NULL outmsg == NULL. Note: this is a serious leak, a single ast_writestream is slightly over 64k. Diffs (updated) - /branches/11/main/app.c 426569 /branches/11/apps/app_voicemail.c 426569 Diff: https://reviewboard.asterisk.org/r/4138/diff/ Testing --- tests/apps/voicemail/check_voicemail_forward_with_prepend/ no longer leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4128: func_jitterbuffer: fix frame leaks
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4128/ --- (Updated Oct. 31, 2014, 1:20 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-22409 https://issues.asterisk.org/jira/browse/ASTERISK-22409 Repository: Asterisk Description --- These changes are not controversial and fix a memory leak so they are now split from r3603. In 12+ these changes apply to main/abstract_js.c instead of funcs/func_jitterbuffer.c. Diffs (updated) - /branches/11/main/abstract_jb.c 426593 /branches/11/funcs/func_jitterbuffer.c 426593 Diff: https://reviewboard.asterisk.org/r/4128/diff/ Testing --- tests/funcs/func_jitterbuffer against 13 no longer leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4122: testsuite: Fix freeze on tests/pbx/dialplan_reload
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4122/ --- (Updated Oct. 31, 2014, 8:02 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers and Scott Griepentrog. Changes --- Committed in revision 5832 Repository: testsuite Description --- In some configurations 3 seconds is not enough of a delay before Asterisk is fully booted, preventing core restart gracefully from succeeding. This causes many iterations to be skipped, and in some cases the test never ends. Make use of core waitfullybooted to delay restarts. Remove unused global variables workingdir and testdir, add global variable restart_iterations to specify 50 runs. Decrease reactor_timeout from 300 to 30, use reset_timeout instead. Diffs - /asterisk/trunk/tests/pbx/dialplan_reload/run-test 5803 Diff: https://reviewboard.asterisk.org/r/4122/diff/ Testing --- Yes Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] Fwd: [asterisk-commits] mjordan: testsuite/asterisk/trunk r5811 - in /asterisk/trunk/tests/fax/pjsip:...
I'm confused about how this can be? Unless I'm missing something a module dependency doesn't effect how the test runs, just if it runs. If chan_sip is not required then shouldn't we remove sip.conf from each of these tests config dirs? -- Forwarded message -- From: SVN commits to the Asterisk project asterisk-comm...@lists.digium.com Date: Thu, Oct 30, 2014 at 1:28 PM Subject: [asterisk-commits] mjordan: testsuite/asterisk/trunk r5811 - in /asterisk/trunk/tests/fax/pjsip:... To: asterisk-comm...@lists.digium.com, svn-comm...@lists.digium.com Author: mjordan Date: Thu Oct 30 12:28:43 2014 New Revision: 5811 URL: http://svnview.digium.com/svn/testsuite?view=revrev=5811 Log: tests/fax/pjsip: Fix failing PJSIP fax tests The tests were failing due to having an erroneous dependency on chan_sip. That has now been corrected in each test's test-config.yaml. Modified: asterisk/trunk/tests/fax/pjsip/gateway_native_t38/test-config.yaml asterisk/trunk/tests/fax/pjsip/gateway_t38_g711/test-config.yaml asterisk/trunk/tests/fax/pjsip/t38/test-config.yaml Modified: asterisk/trunk/tests/fax/pjsip/gateway_native_t38/test-config.yaml URL: http://svnview.digium.com/svn/testsuite/asterisk/trunk/tests/fax/pjsip/gateway_native_t38/test-config.yaml?view=diffrev=5811r1=5810r2=5811 == --- asterisk/trunk/tests/fax/pjsip/gateway_native_t38/test-config.yaml (original) +++ asterisk/trunk/tests/fax/pjsip/gateway_native_t38/test-config.yaml Thu Oct 30 12:28:43 2014 @@ -21,7 +21,6 @@ - python : 'twisted' - python : 'starpy' - custom : 'fax' -- asterisk : 'chan_sip' - asterisk : 'chan_pjsip' - asterisk : 'res_pjsip_t38' tags: Modified: asterisk/trunk/tests/fax/pjsip/gateway_t38_g711/test-config.yaml URL: http://svnview.digium.com/svn/testsuite/asterisk/trunk/tests/fax/pjsip/gateway_t38_g711/test-config.yaml?view=diffrev=5811r1=5810r2=5811 == --- asterisk/trunk/tests/fax/pjsip/gateway_t38_g711/test-config.yaml (original) +++ asterisk/trunk/tests/fax/pjsip/gateway_t38_g711/test-config.yaml Thu Oct 30 12:28:43 2014 @@ -23,7 +23,6 @@ - python : 'twisted' - python : 'starpy' - custom : 'fax' -- asterisk : 'chan_sip' - asterisk : 'chan_pjsip' - asterisk : 'res_pjsip_t38' tags: Modified: asterisk/trunk/tests/fax/pjsip/t38/test-config.yaml URL: http://svnview.digium.com/svn/testsuite/asterisk/trunk/tests/fax/pjsip/t38/test-config.yaml?view=diffrev=5811r1=5810r2=5811 == --- asterisk/trunk/tests/fax/pjsip/t38/test-config.yaml (original) +++ asterisk/trunk/tests/fax/pjsip/t38/test-config.yaml Thu Oct 30 12:28:43 2014 @@ -10,7 +10,6 @@ - python : 'twisted' - python : 'starpy' - custom : 'fax' -- asterisk : 'chan_sip' - asterisk : 'chan_pjsip' - asterisk : 'res_pjsip_t38' tags: -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-commits mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-commits -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4124: audiohooks: Clean references to formats
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4124/ --- (Updated Oct. 30, 2014, 6:44 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 426803 Bugs: ASTERISK-24465 https://issues.asterisk.org/jira/browse/ASTERISK-24465 Repository: Asterisk Description --- Cleanup references to in_translate[x].format / out_translate[x].format in ast_audiohook_detach_list. Diffs - /branches/13/main/audiohook.c 426528 Diff: https://reviewboard.asterisk.org/r/4124/diff/ Testing --- Yes Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4121: testsuite: Close ARI websocket connections before stopping reactor
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4121/ --- (Updated Oct. 30, 2014, 6:49 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 5818 Repository: testsuite Description --- All (or most) tests in tests/rest_api leak numerous referenced objects by not closing the ARI websocket connection. Diffs - /asterisk/trunk/lib/python/asterisk/ari.py 5796 Diff: https://reviewboard.asterisk.org/r/4121/diff/ Testing --- Using r4038 Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4125: app_queue: fix a couple leaks to struct call_queue in set_member_value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4125/ --- (Updated Oct. 30, 2014, 6:53 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 426805 Bugs: ASTERISK-24466 https://issues.asterisk.org/jira/browse/ASTERISK-24466 Repository: Asterisk Description --- set_member_value has a couple leaks to references in the variable q found through testsuite tests/queues/set_penalty. This change also removes the REF_DEBUG_ONLY_QUEUES compiler declaration, this is no longer possible with the updated REF_DEBUG code. Diffs - /branches/11/apps/app_queue.c 426569 Diff: https://reviewboard.asterisk.org/r/4125/diff/ Testing --- All tests/queues/set_penalty no longer leaks any references (verifies first added queue_unref). I'm unsure if the second added queue_unref has been tested, but seems like it is needed. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4128: func_jitterbuffer: fix frame leaks
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4128/ --- Review request for Asterisk Developers. Bugs: ASTERISK-22409 https://issues.asterisk.org/jira/browse/ASTERISK-22409 Repository: Asterisk Description --- These changes are not controversial and fix a memory leak so they are now split from r3603. In 12+ these changes apply to main/abstract_js.c instead of funcs/func_jitterbuffer.c. Diffs - /branches/11/funcs/func_jitterbuffer.c 426593 Diff: https://reviewboard.asterisk.org/r/4128/diff/ Testing --- tests/funcs/func_jitterbuffer against 13 no longer leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3603: func_jitterbuffer: fix audio failure caused by certain masquerade's
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3603/ --- (Updated Oct. 30, 2014, 8:06 p.m.) Review request for Asterisk Developers, Joshua Colp and Matt Jordan. Changes --- Remove fixes for leaks and move them to new review 4128, this way they can be addressed sooner. Summary (updated) - func_jitterbuffer: fix audio failure caused by certain masquerade's Bugs: ASTERISK-22409 https://issues.asterisk.org/jira/browse/ASTERISK-22409 Repository: Asterisk Description (updated) --- During masquerade it is possible for the AST_JITTERBUFFER_FD to be cleared (set to -1). This change adds a check when copying channel fd's to prevent clearing an FD with -1. This seems to resolve the bad audio quality experienced after the masquerade. When AST_JITTERBUFFER_FD was set to -1, this prevented the channel from polling that timer. This caused RTP packets to be received late, and discarded. Diffs (updated) - /branches/11/main/channel.c 426593 /branches/11/funcs/func_jitterbuffer.c 426593 Diff: https://reviewboard.asterisk.org/r/3603/diff/ Testing (updated) --- Verified the scenario outlined in ASTERISK-22409 no longer experiences audio quality loss. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] func_jitterbuffer handling of masquerades
Hello everyone, Review 3063 [1] is posted to address audio quality failure experienced when certain masquerades occur on a channel with func_jitterbuffer active. The goal of this change is to address situations where a masquerade occurs due to Local channel optimization, and one or both of the channels have active JB. This email refers to the channels of the masquerade using variable names clonechan and original. Since these are not very good names I will explain that clonechan is the source and original is the destination of the masquerade. So by default all the datastores / framehooks / etc should be copied from clonechan to original. After the masquerade clonechan will soon die. There are 3 possible scenarios involving masquerades with func_jitterbuffer active: 1) JB is active on original but not clonechan 2) JB is active on clonechan but not original 3) JB is active on both With the current code scenario 1 causes complete failure of audio quality. This is caused by AST_JITTERBUFFER_FD being cleared on original while the framehook is still active. This causes the framehook to run without a functioning ast_timer, and nearly all packets are rejected by the jitterbuffer. The change I am proposing is that we always have an active JB after masquerade if either side had one before the masquerade. So in scenario 1 and 2 listed above this would cause the only active jitterbuffer to remain active after a masquerade. For situations where both channels have active jitterbuffer, we would always prefer the jitterbuffer settings from clonechan. [1] https://reviewboard.asterisk.org/r/3603/ -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4128: func_jitterbuffer: fix frame leaks
On Oct. 30, 2014, 8:32 p.m., rmudgett wrote: /branches/11/funcs/func_jitterbuffer.c, lines 309-310 https://reviewboard.asterisk.org/r/4128/diff/1/?file=68612#file68612line309 ast_frfree(frame) before dup replaces frame. This caused a segmentation fault in tests/funcs/func_jitterbuffer. On Oct. 30, 2014, 8:32 p.m., rmudgett wrote: /branches/11/funcs/func_jitterbuffer.c, line 315 https://reviewboard.asterisk.org/r/4128/diff/1/?file=68612#file68612line315 ast_frfree(frame) before replacing with null frame. This caused a segmentation fault in tests/funcs/func_jitterbuffer. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4128/#review13639 --- On Oct. 30, 2014, 8:06 p.m., Corey Farrell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4128/ --- (Updated Oct. 30, 2014, 8:06 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-22409 https://issues.asterisk.org/jira/browse/ASTERISK-22409 Repository: Asterisk Description --- These changes are not controversial and fix a memory leak so they are now split from r3603. In 12+ these changes apply to main/abstract_js.c instead of funcs/func_jitterbuffer.c. Diffs - /branches/11/funcs/func_jitterbuffer.c 426593 Diff: https://reviewboard.asterisk.org/r/4128/diff/ Testing --- tests/funcs/func_jitterbuffer against 13 no longer leaks. Thanks, Corey Farrell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
