Re: [asterisk-dev] [Code Review] 3125: http: support chunked Transfer-Encoding
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3125/#review10621 --- Ship it! Ship It! - Mark Michelson On Jan. 13, 2014, 11:26 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3125/ --- (Updated Jan. 13, 2014, 11:26 p.m.) Review request for Asterisk Developers and Matt Jordan. Bugs: ASTERISK-23068 https://issues.asterisk.org/jira/browse/ASTERISK-23068 Repository: Asterisk Description --- Implements support for HTTP chunked mode Transfer-Encoding in JSON and Form (post vars) body content. Relocated code from ast_http_get_json() and ast_http_get_post_vars() that reads content from stream into new function ast_http_get_contents(), and added support for reading and decoding chunked mode transfers. Diffs - /branches/12/main/http.c 405282 Diff: https://reviewboard.asterisk.org/r/3125/diff/ Testing --- Testsuite test rest_api/chunked_transfer added (see https://reviewboard.asterisk.org/r/3126/) to insure correct operation of chunked and regular mode. 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] 3133: manager: Clarify eventfilter documentation
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3133/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- Hi, I wasn't too happy with the eventfilter documentation, so I tried to clarify it a bit. Suggestions are welcome. I'm still not fond of the DAHDI.* example, since it implies that there is anchoring going, which there isn't. Diffs - /branches/1.8/main/manager.c 405159 /branches/1.8/configs/manager.conf.sample 405159 Diff: https://reviewboard.asterisk.org/r/3133/diff/ Testing --- No testing needed. Changes are textual only. Thanks, wdoekes -- _ -- 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] 3129: chan_pjsip: Provide a means for updating device state when going on/off hold
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3129/#review10622 --- Ship it! Ship It! - Mark Michelson On Jan. 16, 2014, 8:56 p.m., Jonathan Rose wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3129/ --- (Updated Jan. 16, 2014, 8:56 p.m.) Review request for Asterisk Developers and Mark Michelson. Repository: Asterisk Description --- Maintains a list of on hold channel uniqueids. When a snapshot is used with chan_pjsip's device state evaluating function, this list is queried to see if the channel in the snapshot is on hold. As the channel receives indications of HOLD/UNHOLD, this list is updated and the device state is updated. When the session ends, the session channel is removed from the list if it is still in it. Diffs - /branches/12/channels/chan_pjsip.c 405641 Diff: https://reviewboard.asterisk.org/r/3129/diff/ Testing --- Manual testing where I watched the device state with a websocket, polled for device state with Manager command GetVar and DEVICE_STATE function, and also modified an automated test (channels/pjsip/hold) to have a hint which checks the device state of a PJSIP channel and ExtensionStatus event is monitored to evaluate device state changes as they had. The test modifications will be in another review. Thanks, Jonathan Rose -- _ -- 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] 3125: http: support chunked Transfer-Encoding
On Jan. 16, 2014, 5 p.m., Mark Michelson wrote: /branches/12/main/http.c, line 813 https://reviewboard.asterisk.org/r/3125/diff/2/?file=52892#file52892line813 chunked_atoh shouldn't be necessary. You should be able to just use: sscanf(chunk_header, %x, chunk_length); You would need to change chunk_length to be an unsigned int instead of an int, but that's fine since the length will never be negative. Scott Griepentrog wrote: I had considered that, but I wanted to be overly cautious and throw extra checking into the algorithm, such that any invalid character or unexpected sequence would result in an error, rather than partial input and confused results. Fair enough. I'll add my ship it since that's the only thing I noticed. - Mark --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3125/#review10603 --- On Jan. 13, 2014, 11:26 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3125/ --- (Updated Jan. 13, 2014, 11:26 p.m.) Review request for Asterisk Developers and Matt Jordan. Bugs: ASTERISK-23068 https://issues.asterisk.org/jira/browse/ASTERISK-23068 Repository: Asterisk Description --- Implements support for HTTP chunked mode Transfer-Encoding in JSON and Form (post vars) body content. Relocated code from ast_http_get_json() and ast_http_get_post_vars() that reads content from stream into new function ast_http_get_contents(), and added support for reading and decoding chunked mode transfers. Diffs - /branches/12/main/http.c 405282 Diff: https://reviewboard.asterisk.org/r/3125/diff/ Testing --- Testsuite test rest_api/chunked_transfer added (see https://reviewboard.asterisk.org/r/3126/) to insure correct operation of chunked and regular mode. 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] 3133: manager: Clarify eventfilter documentation
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3133/ --- (Updated Jan. 17, 2014, 9:46 a.m.) Review request for Asterisk Developers. Changes --- going +on Repository: Asterisk Description (updated) --- Hi, I wasn't too happy with the eventfilter documentation, so I tried to clarify it a bit. Suggestions are welcome. I'm still not fond of the DAHDI.* example, since it implies that there is anchoring going on, which there isn't. Diffs - /branches/1.8/main/manager.c 405159 /branches/1.8/configs/manager.conf.sample 405159 Diff: https://reviewboard.asterisk.org/r/3133/diff/ Testing --- No testing needed. Changes are textual only. Thanks, wdoekes -- _ -- 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] 3051: TestSuite: Add chan_pjsip path support tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3051/#review10619 --- Without going into the functionality of the patch, a few notes: asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/ua1_invite_recv.xml https://reviewboard.asterisk.org/r/3051/#comment20117 You're referencing more than the ;tag bit, below: To: [$remote_tag] asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/ua1_invite_recv.xml https://reviewboard.asterisk.org/r/3051/#comment20118 I think you can do: From: [last_To] To: [last_From] But don't take my word for it. asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/ua1_invite_recv.xml https://reviewboard.asterisk.org/r/3051/#comment20119 A bit inconsitent with contacts with and without angle brackets. Not that it matters. - wdoekes On Jan. 6, 2014, 9:53 p.m., opticron wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3051/ --- (Updated Jan. 6, 2014, 9:53 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-21084 https://issues.asterisk.org/jira/browse/ASTERISK-21084 Repository: testsuite Description --- This adds a test which covers path support for outbound registrations, inbound registrations, and outbound requests following an inbound registration. Diffs - asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/tests.yaml 4485 asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/test-config.yaml PRE-CREATION asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/ua1_register.xml PRE-CREATION asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/ua1_invite_recv.xml PRE-CREATION asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/registrar.xml PRE-CREATION asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/configs/ast1/pjsip.conf PRE-CREATION asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/configs/ast1/extensions.conf PRE-CREATION Diff: https://reviewboard.asterisk.org/r/3051/diff/ Testing --- Ensured the tests behaved as expected. Thanks, opticron -- _ -- 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] 3131: PJSIP: support 'allow=all'
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3131/#review10623 --- Ship it! Because my comments are so minor, I'm going ahead with a ship it! on the review. /branches/12/main/format_pref.c https://reviewboard.asterisk.org/r/3131/#comment20123 And the award for most nit-picky critique on a code review goes to: drumroll Mark Michelson! For his comment: The i.e. here should really be an e.g. since you are providing an example of an entry that could result in duplicates. /branches/12/main/format_pref.c https://reviewboard.asterisk.org/r/3131/#comment20121 Coding guidelines: add spaces around the plus sign. /branches/12/main/format_pref.c https://reviewboard.asterisk.org/r/3131/#comment20122 Coding guidelines: add spaces around the plus sign. - Mark Michelson On Jan. 16, 2014, 10:13 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3131/ --- (Updated Jan. 16, 2014, 10:13 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-23018 https://issues.asterisk.org/jira/browse/ASTERISK-23018 Repository: Asterisk Description --- Changes to support allow=all in PJSIP.conf: * added ast_codec_pref_append_all() that intelligently adds all yet unspecified codecs to the preference list * Sorcery function ast_parse_allow_disallow() now uses codec_pref_append_all on pref list when all is supplied * in create_outgoing_sdp_stream(), the loop checking codecs to add will now skip a codec that doesn't have a payload code instead of bailing (fixes issue with deferred sdp bombing on testlaw) * switched display of codecs (reverse codec_handler_fn in sorcery.c) from caps to prefs, so that cli 'pjsip show endpoint x' shows allowed codec list in preference order matching the configuration Note that it is now possible to put all at the end of an allow list, such as allow=ulaw,all which will set ulaw as preferred (first in list) but still have every other available codec listed. Also possible is to remove specific codecs after all, such as allow=ulaw,all,!g729 which puts ulaw first, then every other codec, but removes g729. The codec order otherwise is currently set by the internal codec table list order, which is arbitrary but functional. Diffs - /branches/12/res/res_pjsip_sdp_rtp.c 405637 /branches/12/main/sorcery.c 405637 /branches/12/main/frame.c 405637 /branches/12/main/format_pref.c 405637 /branches/12/include/asterisk/format_pref.h 405637 Diff: https://reviewboard.asterisk.org/r/3131/diff/ Testing --- Tested with new test allow_all_sdp (https://reviewboard.asterisk.org/r/3132/) with success. Also ran pjsip basic_calls. 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] 3118: Testsuite: Add tests for ARI control of external MWI (ARI mailboxes resource)
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3118/#review10624 --- Ship it! Ship It! - Mark Michelson On Jan. 16, 2014, 8:26 p.m., Jonathan Rose wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3118/ --- (Updated Jan. 16, 2014, 8:26 p.m.) Review request for Asterisk Developers, Kevin Harwell, Matt Jordan, and Mark Michelson. Repository: testsuite Description --- Somewhat similar to kharwell's device state tests. ARI is used to: * create a couple mailboxes * confirm that GET can be used to get one mailbox and that it matches expectations * confirm that GET can be used to get all the mailboxes and that they match expectations * modify one mailbox * delete another mailbox * verify that the mailbox that was deleted is no longer obtained by get * verify that the mailbox that was modified has the modified state. The nature of these operations is simple enough that this seemed appropriate to be a single test. Diffs - /asterisk/trunk/tests/rest_api/tests.yaml 4545 /asterisk/trunk/tests/rest_api/mailbox/baseline/test-config.yaml PRE-CREATION /asterisk/trunk/tests/rest_api/mailbox/baseline/mailbox_baseline.py PRE-CREATION /asterisk/trunk/tests/rest_api/mailbox/baseline/configs/ast1/extensions.conf PRE-CREATION Diff: https://reviewboard.asterisk.org/r/3118/diff/ Testing --- Ran the test. Varied the expectations from the results to cause failures for each substep. Thanks, Jonathan Rose -- _ -- 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] 3122: ARI: Add support for specifying channel variables during originate
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3122/#review10625 --- branches/12/res/ari/resource_channels.c https://reviewboard.asterisk.org/r/3122/#comment20125 I'm sure you looked into this, but is there no way to have this block of code in the auto generated file res_ari_channels.c? I'm guessing the answer is no as this is probably the crux of the problem, but figured I'd ask :-) branches/12/res/res_ari_channels.c https://reviewboard.asterisk.org/r/3122/#comment20124 Is this line auto generated or did you have to manually enter it? I know the file is and it would be a shame to have to manually edit that file from here on out. - Kevin Harwell On Jan. 13, 2014, 11:09 a.m., opticron wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3122/ --- (Updated Jan. 13, 2014, 11:09 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-23051 https://issues.asterisk.org/jira/browse/ASTERISK-23051 Repository: Asterisk Description --- This adds back in support for specifying channel variables during an originate without compromising the ability to specify query parameters in the JSON body. This was accomplished by generating the body-parsing code in a separate function instead of being integrated with the URI query parameter parsing code such that it could be called by paths with body parameters. This is transparent to the user of the API and prevents manual duplication of code or data structures. Diffs - branches/12/rest-api/api-docs/channels.json 405252 branches/12/rest-api-templates/res_ari_resource.c.mustache 405252 branches/12/rest-api-templates/param_parsing.mustache 405252 branches/12/rest-api-templates/body_parsing.mustache PRE-CREATION branches/12/rest-api-templates/asterisk_processor.py 405252 branches/12/rest-api-templates/ari_resource.h.mustache 405252 branches/12/res/res_ari_sounds.c 405252 branches/12/res/res_ari_playbacks.c 405252 branches/12/res/res_ari_device_states.c 405252 branches/12/res/res_ari_channels.c 405252 branches/12/res/res_ari_bridges.c 405252 branches/12/res/res_ari_asterisk.c 405252 branches/12/res/res_ari_applications.c 405252 branches/12/res/ari/resource_sounds.h 405252 branches/12/res/ari/resource_playbacks.h 405252 branches/12/res/ari/resource_device_states.h 405252 branches/12/res/ari/resource_channels.c 405252 branches/12/res/ari/resource_channels.h 405252 branches/12/res/ari/resource_bridges.h 405252 branches/12/res/ari/resource_asterisk.h 405252 branches/12/res/ari/resource_applications.h 405252 Diff: https://reviewboard.asterisk.org/r/3122/diff/ Testing --- Tested with a slightly modified originate_with_vars test created for the original functionality by Kevin Harwell. Thanks, opticron -- _ -- 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] 3134: pbx.c: avoid segfault on uninitialized time values
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3134/ --- Review request for Asterisk Developers. Bugs: ASTERISK-22861 https://issues.asterisk.org/jira/browse/ASTERISK-22861 Repository: Asterisk Description --- Merge of two patches - one from sebmurray and another from coreyfarrell - related to eliminating referencing uninitialized structures in pbx.c. Avoids destroying timing unless it was successfully built, and leaking an uninitialized timezone. Diffs - /branches/1.8/utils/extconf.c 405842 /branches/1.8/main/pbx.c 405842 Diff: https://reviewboard.asterisk.org/r/3134/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] 3130: testsuite: Update hold test to monitor device state of PJSIP channels
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3130/#review10626 --- Ship it! Ship It! - Mark Michelson On Jan. 16, 2014, 6:33 p.m., Jonathan Rose wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3130/ --- (Updated Jan. 16, 2014, 6:33 p.m.) Review request for Asterisk Developers, Matt Jordan and Mark Michelson. Repository: testsuite Description --- Tests that this is working: https://reviewboard.asterisk.org/r/3129/ Adds a hint monitoring the phone_A device so that device state changes to phone_A will issue ExtensionStatus events. These events are then evaluated to check for inuse, notinuse, and hold statuses. Diffs - /asterisk/trunk/tests/channels/pjsip/hold/run-test 4559 /asterisk/trunk/tests/channels/pjsip/hold/configs/ast1/extensions.conf 4559 Diff: https://reviewboard.asterisk.org/r/3130/diff/ Testing --- Ran test against unpatched Asterisk (https://reviewboard.asterisk.org/r/3129/) - Failed Ran test against patched Asterisk - Success Verified that the cause of the failures in each case was the lack of hold and subsequent inuse status changes. Thanks, Jonathan Rose -- _ -- 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] 3125: http: support chunked Transfer-Encoding
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3125/#review10627 --- Ship it! The chunked decode looks good according to the RFC! - opticron On Jan. 13, 2014, 5:26 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3125/ --- (Updated Jan. 13, 2014, 5:26 p.m.) Review request for Asterisk Developers and Matt Jordan. Bugs: ASTERISK-23068 https://issues.asterisk.org/jira/browse/ASTERISK-23068 Repository: Asterisk Description --- Implements support for HTTP chunked mode Transfer-Encoding in JSON and Form (post vars) body content. Relocated code from ast_http_get_json() and ast_http_get_post_vars() that reads content from stream into new function ast_http_get_contents(), and added support for reading and decoding chunked mode transfers. Diffs - /branches/12/main/http.c 405282 Diff: https://reviewboard.asterisk.org/r/3125/diff/ Testing --- Testsuite test rest_api/chunked_transfer added (see https://reviewboard.asterisk.org/r/3126/) to insure correct operation of chunked and regular mode. 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] 3125: http: support chunked Transfer-Encoding
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3125/ --- (Updated Jan. 17, 2014, 2:51 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers and Matt Jordan. Bugs: ASTERISK-23068 https://issues.asterisk.org/jira/browse/ASTERISK-23068 Repository: Asterisk Description --- Implements support for HTTP chunked mode Transfer-Encoding in JSON and Form (post vars) body content. Relocated code from ast_http_get_json() and ast_http_get_post_vars() that reads content from stream into new function ast_http_get_contents(), and added support for reading and decoding chunked mode transfers. Diffs - /branches/12/main/http.c 405282 Diff: https://reviewboard.asterisk.org/r/3125/diff/ Testing --- Testsuite test rest_api/chunked_transfer added (see https://reviewboard.asterisk.org/r/3126/) to insure correct operation of chunked and regular mode. 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] 3126: testsuite: check chunked Transfer-Encoding operation
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3126/ --- (Updated Jan. 17, 2014, 2:58 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers and Matt Jordan. Bugs: ASTERISK-23068 https://issues.asterisk.org/jira/browse/ASTERISK-23068 Repository: testsuite Description --- Test checks operation of http chunked transfer encoding in review https://reviewboard.asterisk.org/r/3125/. Uses requests library to initiate HTTP request, which uses chunked mode when a generator function supplies the data. Writes a global variable using a chunked request, and then reads it back with a regular request and compares the value to the original. Diffs - /asterisk/trunk/tests/rest_api/tests.yaml 4550 /asterisk/trunk/tests/rest_api/chunked-transfer/test-config.yaml PRE-CREATION /asterisk/trunk/tests/rest_api/chunked-transfer/run-test PRE-CREATION Diff: https://reviewboard.asterisk.org/r/3126/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] 3122: ARI: Add support for specifying channel variables during originate
On Jan. 17, 2014, 12:38 p.m., Kevin Harwell wrote: branches/12/res/ari/resource_channels.c, lines 753-756 https://reviewboard.asterisk.org/r/3122/diff/2/?file=51449#file51449line753 I'm sure you looked into this, but is there no way to have this block of code in the auto generated file res_ari_channels.c? I'm guessing the answer is no as this is probably the crux of the problem, but figured I'd ask :-) This block of code could easily be in the auto-generated code, but having this behavior be the default would break the model that Swagger operates on and would prevent the body parameter from being used in other ways. A body parameter is ALWAYS expected to consume the entirety of the body's JSON; this one just so happens to parse it as if there were no body parameter and only uses one element in the body. On Jan. 17, 2014, 12:38 p.m., Kevin Harwell wrote: branches/12/res/res_ari_channels.c, line 209 https://reviewboard.asterisk.org/r/3122/diff/2/?file=51456#file51456line209 Is this line auto generated or did you have to manually enter it? I know the file is and it would be a shame to have to manually edit that file from here on out. This line lives in res_ari_channels.c and so was auto-generated. - opticron --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3122/#review10625 --- On Jan. 13, 2014, 11:09 a.m., opticron wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3122/ --- (Updated Jan. 13, 2014, 11:09 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-23051 https://issues.asterisk.org/jira/browse/ASTERISK-23051 Repository: Asterisk Description --- This adds back in support for specifying channel variables during an originate without compromising the ability to specify query parameters in the JSON body. This was accomplished by generating the body-parsing code in a separate function instead of being integrated with the URI query parameter parsing code such that it could be called by paths with body parameters. This is transparent to the user of the API and prevents manual duplication of code or data structures. Diffs - branches/12/rest-api/api-docs/channels.json 405252 branches/12/rest-api-templates/res_ari_resource.c.mustache 405252 branches/12/rest-api-templates/param_parsing.mustache 405252 branches/12/rest-api-templates/body_parsing.mustache PRE-CREATION branches/12/rest-api-templates/asterisk_processor.py 405252 branches/12/rest-api-templates/ari_resource.h.mustache 405252 branches/12/res/res_ari_sounds.c 405252 branches/12/res/res_ari_playbacks.c 405252 branches/12/res/res_ari_device_states.c 405252 branches/12/res/res_ari_channels.c 405252 branches/12/res/res_ari_bridges.c 405252 branches/12/res/res_ari_asterisk.c 405252 branches/12/res/res_ari_applications.c 405252 branches/12/res/ari/resource_sounds.h 405252 branches/12/res/ari/resource_playbacks.h 405252 branches/12/res/ari/resource_device_states.h 405252 branches/12/res/ari/resource_channels.c 405252 branches/12/res/ari/resource_channels.h 405252 branches/12/res/ari/resource_bridges.h 405252 branches/12/res/ari/resource_asterisk.h 405252 branches/12/res/ari/resource_applications.h 405252 Diff: https://reviewboard.asterisk.org/r/3122/diff/ Testing --- Tested with a slightly modified originate_with_vars test created for the original functionality by Kevin Harwell. Thanks, opticron -- _ -- 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] 3131: PJSIP: support 'allow=all'
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3131/ --- (Updated Jan. 17, 2014, 3:53 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Bugs: ASTERISK-23018 https://issues.asterisk.org/jira/browse/ASTERISK-23018 Repository: Asterisk Description --- Changes to support allow=all in PJSIP.conf: * added ast_codec_pref_append_all() that intelligently adds all yet unspecified codecs to the preference list * Sorcery function ast_parse_allow_disallow() now uses codec_pref_append_all on pref list when all is supplied * in create_outgoing_sdp_stream(), the loop checking codecs to add will now skip a codec that doesn't have a payload code instead of bailing (fixes issue with deferred sdp bombing on testlaw) * switched display of codecs (reverse codec_handler_fn in sorcery.c) from caps to prefs, so that cli 'pjsip show endpoint x' shows allowed codec list in preference order matching the configuration Note that it is now possible to put all at the end of an allow list, such as allow=ulaw,all which will set ulaw as preferred (first in list) but still have every other available codec listed. Also possible is to remove specific codecs after all, such as allow=ulaw,all,!g729 which puts ulaw first, then every other codec, but removes g729. The codec order otherwise is currently set by the internal codec table list order, which is arbitrary but functional. Diffs - /branches/12/res/res_pjsip_sdp_rtp.c 405637 /branches/12/main/sorcery.c 405637 /branches/12/main/frame.c 405637 /branches/12/main/format_pref.c 405637 /branches/12/include/asterisk/format_pref.h 405637 Diff: https://reviewboard.asterisk.org/r/3131/diff/ Testing --- Tested with new test allow_all_sdp (https://reviewboard.asterisk.org/r/3132/) with success. Also ran pjsip basic_calls. 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] 2723: Add pass through support for both VP8 and Opus
On Jan. 16, 2014, 11:16 a.m., Sean Bright wrote: /trunk/channels/chan_sip.c, lines 13419-13421 https://reviewboard.asterisk.org/r/2723/diff/5/?file=44381#file44381line13419 Shouldn't this be a_audio instead of a_text, or does it matter? Good catch - feel free to patch away - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2723/#review10605 --- On Aug. 23, 2013, 10:42 a.m., Matt Jordan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2723/ --- (Updated Aug. 23, 2013, 10:42 a.m.) Review request for Asterisk Developers, Joshua Colp and Mark Michelson. Bugs: ASTERISK-21981 https://issues.asterisk.org/jira/browse/ASTERISK-21981 Repository: Asterisk Description --- Note: This patch was written by Lorenzo Miniero. I know he's at the IETF this week, but I figured we could get the formal code review going for him :-) This patch adds pass through support for Opus and VP8. That includes: * Format attribute negotiation for Opus. Note that unlike some other codecs, the draft RFC specifies having spaces delimiting the attributes in addition to ';', so you have attra=X; attrb=Y. This broke the attribute parsing in chan_sip, so a small tweak was also included in this patch for that. * A format attribute negotiation module for Opus * Fast picture update for VP8. Since VP8 uses a different RTCP packet number than FIR, this really is specific to VP8 at this time. Ideally this would be more generic and flexible for user preferences and other video codecs, but that could be done at a latter date. The only part of this work that I did was port over the fast picture update code to chan_pjsip. I *think* that chan_pjsip will still suck out the attributes in res_pjsip_sdp_rtp, but I could be mistaken (Josh?) Diffs - /trunk/res/res_rtp_asterisk.c 397424 /trunk/res/res_pjsip_sdp_rtp.c 397424 /trunk/res/res_format_attr_opus.c PRE-CREATION /trunk/main/rtp_engine.c 397424 /trunk/main/frame.c 397424 /trunk/main/format.c 397424 /trunk/main/channel.c 397424 /trunk/include/asterisk/opus.h PRE-CREATION /trunk/include/asterisk/format.h 397424 /trunk/channels/chan_sip.c 397424 /trunk/channels/chan_pjsip.c 397424 Diff: https://reviewboard.asterisk.org/r/2723/diff/ Testing --- 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] 3135: Protect ast_filestream object when on a channel
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/
---
Review request for Asterisk Developers and leifmadsen.
Repository: Asterisk
Description
---
The ast_filestream object gets tacked on to a channel via
chan-timingdata. It's a reference counted object, but the reference
count isn't used when putting it on a channel. It's theoretically
possible for another thread to interfere with the channel while it's
unlocked and cause the filestream to get destroyed.
Use the astobj2 reference count to make sure that as long as this code
path is holding on the ast_filestream and passing it into the file.c
playback code, that it knows it's valid.
-
More detail:
A crash occurs in voicemail. Here is the backtrace:
#0 0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at
file.c:779
#1 0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
#2 0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at
channel.c:3865
#3 0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
#4 0x004d5231 in waitstream_core (c=0x7f2644c49460,
breakon=0x7f262ed56f00 #*, forward=0x64c4a2 , reverse=0x64c4a2 ,
skip_ms=0, audiofd=-1, cmdfd=-1, context=0x0) at file.c:1253
#5 0x004d5783 in ast_waitstream (c=0x7f2644c49460,
breakon=0x7f262ed56f00 #*) at file.c:1344
#6 0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460,
ext=0x7f2609908c20 redacted, options=0x7f262ed56ff0) at app_voicemail.c:5773
#7 0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720
redacted) at app_voicemail.c:10722
(gdb) frame 0
#0 0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at
file.c:779
779 if (s-owner-timingfd -1) {
(gdb) p s
$6 = (struct ast_filestream *) 0x7f260856f580
The crash occurs here because the contents of the ast_filestream are garbage.
(gdb) p *s
$7 = {fmt = 0x6372756f530a0d74, flags = 924858981, mode = 875902769,
open_filename = 0x440a0d3330393734 Address 0x440a0d3330393734 out of bounds,
filename = 0x6974616e69747365 Address 0x6974616e69747365 out of bounds,
realfilename = 0x440a0d73203a6e6f Address 0x440a0d73203a6e6f out of bounds,
vfs = 0x6974616e69747365, trans = 0x7865746e6f436e6f, tr = 0x2d7473786e203a74,
lastwriteformat = 762475363, lasttimeout = 1969583473,
owner = 0x656c6c61430a0d65, f = 0x313722203a444972, fr = {frametype =
875836727, subclass = {integer = 926687266, codec = 3761973748257529890},
datalen = 809056052, samples = 168640051, mallocd = 1851877443,
mallocd_hdr_len = 7881689877736674080, offset = 762148397, src =
0xd61633832313030 Address 0xd61633832313030 out of bounds, data = {ptr =
0x616e69747365440a, uint32 = 1936016394, pad = \nDestina}, delivery = {
tv_sec = 7953753055737899380, tv_usec = 5785246594218354030}, frame_list
= {next = 0x36702d5453584e2f}, flags = 758134073, ts = 7010989772577650738, len
= 7163375912484959347, seqno = 1869182049},
buf = 0x614c0a0d65756575 Address 0x614c0a0d65756575 out of bounds, _private
= 0x203a617461447473, orig_chan_name = 0x7273632d7473786e Address
0x7273632d7473786e out of bounds,
write_buffer = 0x38312c2c2c2c712d Address 0x38312c2c2c2c712d out of bounds}
(gdb) p s-owner
$8 = (struct ast_channel *) 0x656c6c61430a0d65
(gdb) p *s-owner
Cannot access memory at address 0x656c6c61430a0d65
s-owner should have been 0x7f2644c49460, from higher up in the backtrace.
Here is where things get quite interesting ...
(gdb) frame 2
#2 0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at
channel.c:3865
3865func(data);
(gdb) list
3860/* save a copy of func/data before
unlocking the channel */
3861int (*func)(const void *) =
chan-timingfunc;
3862void *data = chan-timingdata;
3863chan-fdno = -1;
3864ast_channel_unlock(chan);
3865func(data);
3866} else {
3867ast_timer_set_rate(chan-timer, 0);
3868chan-fdno = -1;
3869ast_channel_unlock(chan);
(gdb) p chan-timingfunc
$9 = (int (*)(const void *)) 0
(gdb) p chan-timingdata
$10 = (void *) 0x0
(gdb) p func
$11 = (int (*)(const void *)) 0x4d3b6a ast_fsread_audio
(gdb) p data
$12 = (void *) 0x7f260856f580
This is the code inside of ast_read() that executes the callback
ast_fsread_audio() from main/file.c to play the next bits of the audio file to
the channel. We can see that chan-timingfunc and chan-timingdata have now
been reset since this code ran. That probably also means that the
Re: [asterisk-dev] [Code Review] 2723: Add pass through support for both VP8 and Opus
On Jan. 16, 2014, 5:16 p.m., Sean Bright wrote: /trunk/channels/chan_sip.c, lines 13419-13421 https://reviewboard.asterisk.org/r/2723/diff/5/?file=44381#file44381line13419 Shouldn't this be a_audio instead of a_text, or does it matter? Matt Jordan wrote: Good catch - feel free to patch away Committed in r405877 (12)/r405878 (trunk) - Sean --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2723/#review10605 --- On Aug. 23, 2013, 3:42 p.m., Matt Jordan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2723/ --- (Updated Aug. 23, 2013, 3:42 p.m.) Review request for Asterisk Developers, Joshua Colp and Mark Michelson. Bugs: ASTERISK-21981 https://issues.asterisk.org/jira/browse/ASTERISK-21981 Repository: Asterisk Description --- Note: This patch was written by Lorenzo Miniero. I know he's at the IETF this week, but I figured we could get the formal code review going for him :-) This patch adds pass through support for Opus and VP8. That includes: * Format attribute negotiation for Opus. Note that unlike some other codecs, the draft RFC specifies having spaces delimiting the attributes in addition to ';', so you have attra=X; attrb=Y. This broke the attribute parsing in chan_sip, so a small tweak was also included in this patch for that. * A format attribute negotiation module for Opus * Fast picture update for VP8. Since VP8 uses a different RTCP packet number than FIR, this really is specific to VP8 at this time. Ideally this would be more generic and flexible for user preferences and other video codecs, but that could be done at a latter date. The only part of this work that I did was port over the fast picture update code to chan_pjsip. I *think* that chan_pjsip will still suck out the attributes in res_pjsip_sdp_rtp, but I could be mistaken (Josh?) Diffs - /trunk/res/res_rtp_asterisk.c 397424 /trunk/res/res_pjsip_sdp_rtp.c 397424 /trunk/res/res_format_attr_opus.c PRE-CREATION /trunk/main/rtp_engine.c 397424 /trunk/main/frame.c 397424 /trunk/main/format.c 397424 /trunk/main/channel.c 397424 /trunk/include/asterisk/opus.h PRE-CREATION /trunk/include/asterisk/format.h 397424 /trunk/channels/chan_sip.c 397424 /trunk/channels/chan_pjsip.c 397424 Diff: https://reviewboard.asterisk.org/r/2723/diff/ Testing --- 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] 3136: cli: pjsip show endpoint endpoint shows allow/disallow codecs the same
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3136/ --- Review request for Asterisk Developers. Bugs: ASTERISK-23092 https://issues.asterisk.org/jira/browse/ASTERISK-23092 Repository: Asterisk Description --- Insert a ! prefix in the display of endpoint disallow value. Result is: disallow : !(ulaw|alaw) Diffs - /branches/12/res/res_pjsip/pjsip_cli.c 405882 Diff: https://reviewboard.asterisk.org/r/3136/diff/ Testing --- Ran command and checked output. 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] res_pjsip: MWI subscription missing name
Greetings, When a subscription to MWI comes into the res_pjsip_mwi module it will attempt to extract an aor_name from the URI on the request. Currently a warning message is logged and the request is rejected if a suitable AoR cannot be found by the given name. But what should happen if the request URI doesn't contain the name to begin with, but just the address? Currently the best solution that was thought of (by Mark Michelson) is (1) to just subscribe to all AoRs for the endpoint. I like this solution because a) it will work, b) it seems intuitive so easy for users to understand c) it is the least ugly as far as any configuration goes and least invasive as far as code changes go. Other options potentially include: (2) Use the contact - but then if a contact is part of multiple AoRs then what? (3) Attempt some sort of intersection between all AoRs on the endpoint and those associated with the contact and use a best guess. This isn't bad and may narrow things down a bit more, but the results may be unexpected (at least from a user perspective). (4) Just use the mailboxes on the endpoint. But what if they don't want unsolicited MWI? (5) Some other idea? I'll probably go with option one barring any other ideas. I can also add a configuration option on the endpoint to turn this on/off if people feel that is warranted. Thanks! -- Kevin Harwell Digium, Inc. | Software Developer 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- 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] 3136: cli: pjsip show endpoint endpoint shows allow/disallow codecs the same
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3136/#review10632 --- Not sure this is a good idea. First, I wouldn't hardcode a test for disallow in the generic ast_sip_cli_print_sorcery_objset and given the meaning of disallow, the ! doesn't really make sense. It actually negates the disallow. :) - George Joseph On Jan. 17, 2014, 3:46 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3136/ --- (Updated Jan. 17, 2014, 3:46 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-23092 https://issues.asterisk.org/jira/browse/ASTERISK-23092 Repository: Asterisk Description --- Insert a ! prefix in the display of endpoint disallow value. Result is: disallow : !(ulaw|alaw) Diffs - /branches/12/res/res_pjsip/pjsip_cli.c 405882 Diff: https://reviewboard.asterisk.org/r/3136/diff/ Testing --- Ran command and checked output. 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] 3135: Protect ast_filestream object when on a channel
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/#review10631
---
/branches/1.8/main/channel.c
https://reviewboard.asterisk.org/r/3135/#comment20131
This should not be done at all.
You are dropping a reference when timingdata doesn't really hold the
reference. In the only case where timingdata is an identified ao2 object, the
timingdata pointer is sharing the reference with c-stream. The reference is
dropped by ast_closestream() after clearing the timingdata with its own call to
ast_settimeout(). Otherwise, you will need to give timingdata a reference when
setting an identified ao2 object.
/branches/1.8/main/channel.c
https://reviewboard.asterisk.org/r/3135/#comment20130
Setting timingdata to NULL here is unnecessary since it is set with a new
value right after. It is the purpose of the function.
- rmudgett
On Jan. 17, 2014, 4:18 p.m., Russell Bryant wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/
---
(Updated Jan. 17, 2014, 4:18 p.m.)
Review request for Asterisk Developers and leifmadsen.
Repository: Asterisk
Description
---
The ast_filestream object gets tacked on to a channel via
chan-timingdata. It's a reference counted object, but the reference
count isn't used when putting it on a channel. It's theoretically
possible for another thread to interfere with the channel while it's
unlocked and cause the filestream to get destroyed.
Use the astobj2 reference count to make sure that as long as this code
path is holding on the ast_filestream and passing it into the file.c
playback code, that it knows it's valid.
-
More detail:
A crash occurs in voicemail. Here is the backtrace:
#0 0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at
file.c:779
#1 0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
#2 0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at
channel.c:3865
#3 0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
#4 0x004d5231 in waitstream_core (c=0x7f2644c49460,
breakon=0x7f262ed56f00 #*, forward=0x64c4a2 , reverse=0x64c4a2 ,
skip_ms=0, audiofd=-1, cmdfd=-1, context=0x0) at file.c:1253
#5 0x004d5783 in ast_waitstream (c=0x7f2644c49460,
breakon=0x7f262ed56f00 #*) at file.c:1344
#6 0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460,
ext=0x7f2609908c20 redacted, options=0x7f262ed56ff0) at
app_voicemail.c:5773
#7 0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720
redacted) at app_voicemail.c:10722
(gdb) frame 0
#0 0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at
file.c:779
779 if (s-owner-timingfd -1) {
(gdb) p s
$6 = (struct ast_filestream *) 0x7f260856f580
The crash occurs here because the contents of the ast_filestream are garbage.
(gdb) p *s
$7 = {fmt = 0x6372756f530a0d74, flags = 924858981, mode = 875902769,
open_filename = 0x440a0d3330393734 Address 0x440a0d3330393734 out of
bounds, filename = 0x6974616e69747365 Address 0x6974616e69747365 out of
bounds,
realfilename = 0x440a0d73203a6e6f Address 0x440a0d73203a6e6f out of
bounds, vfs = 0x6974616e69747365, trans = 0x7865746e6f436e6f, tr =
0x2d7473786e203a74, lastwriteformat = 762475363, lasttimeout = 1969583473,
owner = 0x656c6c61430a0d65, f = 0x313722203a444972, fr = {frametype =
875836727, subclass = {integer = 926687266, codec = 3761973748257529890},
datalen = 809056052, samples = 168640051, mallocd = 1851877443,
mallocd_hdr_len = 7881689877736674080, offset = 762148397, src =
0xd61633832313030 Address 0xd61633832313030 out of bounds, data = {ptr =
0x616e69747365440a, uint32 = 1936016394, pad = \nDestina}, delivery = {
tv_sec = 7953753055737899380, tv_usec = 5785246594218354030},
frame_list = {next = 0x36702d5453584e2f}, flags = 758134073, ts =
7010989772577650738, len = 7163375912484959347, seqno = 1869182049},
buf = 0x614c0a0d65756575 Address 0x614c0a0d65756575 out of bounds,
_private = 0x203a617461447473, orig_chan_name = 0x7273632d7473786e Address
0x7273632d7473786e out of bounds,
write_buffer = 0x38312c2c2c2c712d Address 0x38312c2c2c2c712d out of
bounds}
(gdb) p s-owner
$8 = (struct ast_channel *) 0x656c6c61430a0d65
(gdb) p *s-owner
Cannot access memory at address 0x656c6c61430a0d65
s-owner should have been 0x7f2644c49460, from higher up in the backtrace.
Here is where things get quite interesting ...
(gdb) frame 2
#2 0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at
channel.c:3865
3865
Re: [asterisk-dev] [Code Review] 3134: pbx.c: avoid segfault on uninitialized time values
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3134/#review10633 --- Only the corryfarrel patch is needed. The other patch is not complete and is supurfluous. - rmudgett On Jan. 17, 2014, 12:51 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3134/ --- (Updated Jan. 17, 2014, 12:51 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-22861 https://issues.asterisk.org/jira/browse/ASTERISK-22861 Repository: Asterisk Description --- Merge of two patches - one from sebmurray and another from coreyfarrell - related to eliminating referencing uninitialized structures in pbx.c. Avoids destroying timing unless it was successfully built, and leaking an uninitialized timezone. Diffs - /branches/1.8/utils/extconf.c 405842 /branches/1.8/main/pbx.c 405842 Diff: https://reviewboard.asterisk.org/r/3134/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] 3135: Protect ast_filestream object when on a channel
On Jan. 17, 2014, 11:50 p.m., rmudgett wrote:
/branches/1.8/main/channel.c, lines 3576-3579
https://reviewboard.asterisk.org/r/3135/diff/1/?file=52962#file52962line3576
This should not be done at all.
You are dropping a reference when timingdata doesn't really hold the
reference. In the only case where timingdata is an identified ao2 object,
the timingdata pointer is sharing the reference with c-stream. The
reference is dropped by ast_closestream() after clearing the timingdata
with its own call to ast_settimeout(). Otherwise, you will need to give
timingdata a reference when setting an identified ao2 object.
I intended to have a corresponding ao2_ref(obj, 1) in ast_settimeout() when it
gets stored on the channel. I'll fix that.
- Russell
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/#review10631
---
On Jan. 17, 2014, 10:18 p.m., Russell Bryant wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/
---
(Updated Jan. 17, 2014, 10:18 p.m.)
Review request for Asterisk Developers and leifmadsen.
Repository: Asterisk
Description
---
The ast_filestream object gets tacked on to a channel via
chan-timingdata. It's a reference counted object, but the reference
count isn't used when putting it on a channel. It's theoretically
possible for another thread to interfere with the channel while it's
unlocked and cause the filestream to get destroyed.
Use the astobj2 reference count to make sure that as long as this code
path is holding on the ast_filestream and passing it into the file.c
playback code, that it knows it's valid.
-
More detail:
A crash occurs in voicemail. Here is the backtrace:
#0 0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at
file.c:779
#1 0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
#2 0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at
channel.c:3865
#3 0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
#4 0x004d5231 in waitstream_core (c=0x7f2644c49460,
breakon=0x7f262ed56f00 #*, forward=0x64c4a2 , reverse=0x64c4a2 ,
skip_ms=0, audiofd=-1, cmdfd=-1, context=0x0) at file.c:1253
#5 0x004d5783 in ast_waitstream (c=0x7f2644c49460,
breakon=0x7f262ed56f00 #*) at file.c:1344
#6 0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460,
ext=0x7f2609908c20 redacted, options=0x7f262ed56ff0) at
app_voicemail.c:5773
#7 0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720
redacted) at app_voicemail.c:10722
(gdb) frame 0
#0 0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at
file.c:779
779 if (s-owner-timingfd -1) {
(gdb) p s
$6 = (struct ast_filestream *) 0x7f260856f580
The crash occurs here because the contents of the ast_filestream are garbage.
(gdb) p *s
$7 = {fmt = 0x6372756f530a0d74, flags = 924858981, mode = 875902769,
open_filename = 0x440a0d3330393734 Address 0x440a0d3330393734 out of
bounds, filename = 0x6974616e69747365 Address 0x6974616e69747365 out of
bounds,
realfilename = 0x440a0d73203a6e6f Address 0x440a0d73203a6e6f out of
bounds, vfs = 0x6974616e69747365, trans = 0x7865746e6f436e6f, tr =
0x2d7473786e203a74, lastwriteformat = 762475363, lasttimeout = 1969583473,
owner = 0x656c6c61430a0d65, f = 0x313722203a444972, fr = {frametype =
875836727, subclass = {integer = 926687266, codec = 3761973748257529890},
datalen = 809056052, samples = 168640051, mallocd = 1851877443,
mallocd_hdr_len = 7881689877736674080, offset = 762148397, src =
0xd61633832313030 Address 0xd61633832313030 out of bounds, data = {ptr =
0x616e69747365440a, uint32 = 1936016394, pad = \nDestina}, delivery = {
tv_sec = 7953753055737899380, tv_usec = 5785246594218354030},
frame_list = {next = 0x36702d5453584e2f}, flags = 758134073, ts =
7010989772577650738, len = 7163375912484959347, seqno = 1869182049},
buf = 0x614c0a0d65756575 Address 0x614c0a0d65756575 out of bounds,
_private = 0x203a617461447473, orig_chan_name = 0x7273632d7473786e Address
0x7273632d7473786e out of bounds,
write_buffer = 0x38312c2c2c2c712d Address 0x38312c2c2c2c712d out of
bounds}
(gdb) p s-owner
$8 = (struct ast_channel *) 0x656c6c61430a0d65
(gdb) p *s-owner
Cannot access memory at address 0x656c6c61430a0d65
s-owner should have been 0x7f2644c49460, from higher up in the backtrace.
Here is where things get quite interesting ...
(gdb) frame 2
#2 0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at
channel.c:3865
3865
Re: [asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/
---
(Updated Jan. 18, 2014, 12:58 a.m.)
Review request for Asterisk Developers and leifmadsen.
Changes
---
Addressed Richard's comments.
Thanks for the review!
Repository: Asterisk
Description
---
The ast_filestream object gets tacked on to a channel via
chan-timingdata. It's a reference counted object, but the reference
count isn't used when putting it on a channel. It's theoretically
possible for another thread to interfere with the channel while it's
unlocked and cause the filestream to get destroyed.
Use the astobj2 reference count to make sure that as long as this code
path is holding on the ast_filestream and passing it into the file.c
playback code, that it knows it's valid.
-
More detail:
A crash occurs in voicemail. Here is the backtrace:
#0 0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at
file.c:779
#1 0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
#2 0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at
channel.c:3865
#3 0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
#4 0x004d5231 in waitstream_core (c=0x7f2644c49460,
breakon=0x7f262ed56f00 #*, forward=0x64c4a2 , reverse=0x64c4a2 ,
skip_ms=0, audiofd=-1, cmdfd=-1, context=0x0) at file.c:1253
#5 0x004d5783 in ast_waitstream (c=0x7f2644c49460,
breakon=0x7f262ed56f00 #*) at file.c:1344
#6 0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460,
ext=0x7f2609908c20 redacted, options=0x7f262ed56ff0) at app_voicemail.c:5773
#7 0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720
redacted) at app_voicemail.c:10722
(gdb) frame 0
#0 0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at
file.c:779
779 if (s-owner-timingfd -1) {
(gdb) p s
$6 = (struct ast_filestream *) 0x7f260856f580
The crash occurs here because the contents of the ast_filestream are garbage.
(gdb) p *s
$7 = {fmt = 0x6372756f530a0d74, flags = 924858981, mode = 875902769,
open_filename = 0x440a0d3330393734 Address 0x440a0d3330393734 out of bounds,
filename = 0x6974616e69747365 Address 0x6974616e69747365 out of bounds,
realfilename = 0x440a0d73203a6e6f Address 0x440a0d73203a6e6f out of bounds,
vfs = 0x6974616e69747365, trans = 0x7865746e6f436e6f, tr = 0x2d7473786e203a74,
lastwriteformat = 762475363, lasttimeout = 1969583473,
owner = 0x656c6c61430a0d65, f = 0x313722203a444972, fr = {frametype =
875836727, subclass = {integer = 926687266, codec = 3761973748257529890},
datalen = 809056052, samples = 168640051, mallocd = 1851877443,
mallocd_hdr_len = 7881689877736674080, offset = 762148397, src =
0xd61633832313030 Address 0xd61633832313030 out of bounds, data = {ptr =
0x616e69747365440a, uint32 = 1936016394, pad = \nDestina}, delivery = {
tv_sec = 7953753055737899380, tv_usec = 5785246594218354030}, frame_list
= {next = 0x36702d5453584e2f}, flags = 758134073, ts = 7010989772577650738, len
= 7163375912484959347, seqno = 1869182049},
buf = 0x614c0a0d65756575 Address 0x614c0a0d65756575 out of bounds, _private
= 0x203a617461447473, orig_chan_name = 0x7273632d7473786e Address
0x7273632d7473786e out of bounds,
write_buffer = 0x38312c2c2c2c712d Address 0x38312c2c2c2c712d out of bounds}
(gdb) p s-owner
$8 = (struct ast_channel *) 0x656c6c61430a0d65
(gdb) p *s-owner
Cannot access memory at address 0x656c6c61430a0d65
s-owner should have been 0x7f2644c49460, from higher up in the backtrace.
Here is where things get quite interesting ...
(gdb) frame 2
#2 0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at
channel.c:3865
3865func(data);
(gdb) list
3860/* save a copy of func/data before
unlocking the channel */
3861int (*func)(const void *) =
chan-timingfunc;
3862void *data = chan-timingdata;
3863chan-fdno = -1;
3864ast_channel_unlock(chan);
3865func(data);
3866} else {
3867ast_timer_set_rate(chan-timer, 0);
3868chan-fdno = -1;
3869ast_channel_unlock(chan);
(gdb) p chan-timingfunc
$9 = (int (*)(const void *)) 0
(gdb) p chan-timingdata
$10 = (void *) 0x0
(gdb) p func
$11 = (int (*)(const void *)) 0x4d3b6a ast_fsread_audio
(gdb) p data
$12 = (void *) 0x7f260856f580
This is the code inside of ast_read() that executes the callback
ast_fsread_audio() from main/file.c to play the next bits of the audio file to
the channel. We can see that
