[asterisk-commits] mjordan: trunk r392564 - /trunk/res/res_fax.c
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Sat Jun 22 08:58:09 CDT 2013
Author: mjordan
Date: Sat Jun 22 08:58:07 2013
New Revision: 392564
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=392564
Log:
Fix a deadlock and possible crash in res_fax
This patch fixes two bugs.
(1) It unlocks the channel in the framehook handlers before attempting to grab
the peer from the bridge. The locking order for the bridging framework is
bridge first, then channel - having the channel locked while attempting to
obtain the bridge lock causes a locking inversion and a deadlock. This
patch bumps the channel ref count prior to releasing the lock in the
framehook to avoid lifetime issues.
Note that this does expose a subtle problem in framehooks; that is,
something could modify the framehook list while we are executing, causing
issues in the framehook list traversal that the callback executes in.
Fixing this is a much larger problem that is beyond the scope of this
patch - (a) we already unlock the channel in this particular framehook
and we haven't run into a problem yet (as modifying the framehook list
when a channel is about to perform a fax gateway would be a very odd
operation) and (b) migrating to an ao2 container of framehooks would be
more invasive at this point. See the referenced ASTERISK issue for more
information.
(2) Directly packing channel variables into a JSON object turned out to be
unsafe. A condition existed where the strings in the JSON blob were no
longer safe to be accessed if the channel object itself was disposed of.
(issue ASTERISK-21951)
Modified:
trunk/res/res_fax.c
Modified: trunk/res/res_fax.c
URL: http://svnview.digium.com/svn/asterisk/trunk/res/res_fax.c?view=diff&rev=392564&r1=392563&r2=392564
==============================================================================
--- trunk/res/res_fax.c (original)
+++ trunk/res/res_fax.c Sat Jun 22 08:58:07 2013
@@ -58,7 +58,7 @@
* \addtogroup configuration_file Configuration Files
*/
-/*!
+/*!
* \page res_fax.conf res_fax.conf
* \verbinclude res_fax.conf.sample
*/
@@ -1775,15 +1775,26 @@
ast_json_array_append(json_array, json_filename);
{
+ const char *remote_station_id;
+ const char *local_station_id;
+ const char *fax_pages;
+ const char *fax_resolution;
+ const char *fax_bitrate;
SCOPED_CHANNELLOCK(lock, chan);
- json_object = ast_json_pack("s: s, s: s, s: s, s: s, s: s, s: s, s: o",
- "type", "receive"
- "remote_station_id", S_OR(pbx_builtin_getvar_helper(chan, "REMOTESTATIONID"), ""),
- "local_station_id", S_OR(pbx_builtin_getvar_helper(chan, "LOCALSTATIONID"), ""),
- "fax_pages", S_OR(pbx_builtin_getvar_helper(chan, "FAXPAGES"), ""),
- "fax_resolution", S_OR(pbx_builtin_getvar_helper(chan, "FAXRESOLUTION"), ""),
- "fax_bitrate", S_OR(pbx_builtin_getvar_helper(chan, "FAXBITRATE"), ""),
+ remote_station_id = S_OR(pbx_builtin_getvar_helper(chan, "REMOTESTATIONID"), "");
+ local_station_id = S_OR(pbx_builtin_getvar_helper(chan, "LOCALSTATIONID"), "");
+ fax_pages = S_OR(pbx_builtin_getvar_helper(chan, "FAXPAGES"), "");
+ fax_resolution = S_OR(pbx_builtin_getvar_helper(chan, "FAXRESOLUTION"), "");
+ fax_bitrate = S_OR(pbx_builtin_getvar_helper(chan, "FAXBITRATE"), "");
+
+ json_object = ast_json_pack("{s: s, s: s, s: s, s: s, s: s, s: s, s: O}",
+ "type", "receive",
+ "remote_station_id", remote_station_id,
+ "local_station_id", local_station_id,
+ "fax_pages", fax_pages,
+ "fax_resolution", fax_resolution,
+ "fax_bitrate", fax_bitrate,
"filenames", json_array);
if (!json_object) {
return -1;
@@ -1793,7 +1804,6 @@
if (!message) {
return -1;
}
-
stasis_publish(ast_channel_topic(chan), message);
}
return 0;
@@ -2256,14 +2266,26 @@
}
{
+ const char *remote_station_id;
+ const char *local_station_id;
+ const char *fax_pages;
+ const char *fax_resolution;
+ const char *fax_bitrate;
SCOPED_CHANNELLOCK(lock, chan);
+
+ remote_station_id = S_OR(pbx_builtin_getvar_helper(chan, "REMOTESTATIONID"), "");
+ local_station_id = S_OR(pbx_builtin_getvar_helper(chan, "LOCALSTATIONID"), "");
+ fax_pages = S_OR(pbx_builtin_getvar_helper(chan, "FAXPAGES"), "");
+ fax_resolution = S_OR(pbx_builtin_getvar_helper(chan, "FAXRESOLUTION"), "");
+ fax_bitrate = S_OR(pbx_builtin_getvar_helper(chan, "FAXBITRATE"), "");
+
json_obj = ast_json_pack("{s: s, s: s, s: s, s: s, s: s, s: s, s: o}",
"type", "send"
- "remote_station_id", S_OR(pbx_builtin_getvar_helper(chan, "REMOTESTATIONID"), ""),
- "local_station_id", S_OR(pbx_builtin_getvar_helper(chan, "LOCALSTATIONID"), ""),
- "fax_pages", S_OR(pbx_builtin_getvar_helper(chan, "FAXPAGES"), ""),
- "fax_resolution", S_OR(pbx_builtin_getvar_helper(chan, "FAXRESOLUTION"), ""),
- "fax_bitrate", S_OR(pbx_builtin_getvar_helper(chan, "FAXBITRATE"), ""),
+ "remote_station_id", remote_station_id,
+ "local_station_id", local_station_id,
+ "fax_pages", fax_pages,
+ "fax_resolution", fax_resolution,
+ "fax_bitrate", fax_bitrate,
"filenames", json_filenames);
if (!json_obj) {
return -1;
@@ -2980,6 +3002,10 @@
struct ast_channel *active;
RAII_VAR(struct ast_fax_session_details *, details, NULL, ao2_cleanup);
RAII_VAR(struct ast_channel *, peer, NULL, ao2_cleanup);
+ RAII_VAR(struct ast_channel *, chan_ref, chan, ao2_cleanup);
+
+ /* Ref bump channel for when we have to unlock it */
+ ao2_ref(chan_ref, 1);
if (gateway->s) {
details = gateway->s->details;
@@ -3000,7 +3026,10 @@
ast_set_read_format(chan, &gateway->chan_read_format);
ast_set_read_format(chan, &gateway->chan_write_format);
- if ((peer = ast_channel_bridge_peer(chan))) {
+ ast_channel_unlock(chan);
+ peer = ast_channel_bridge_peer(chan);
+ ast_channel_lock(chan);
+ if (peer) {
ast_set_read_format(peer, &gateway->peer_read_format);
ast_set_read_format(peer, &gateway->peer_write_format);
ast_channel_make_compatible(chan, peer);
@@ -3019,7 +3048,10 @@
}
/* If we aren't bridged or we don't have a peer, don't do anything */
- if (!(peer = ast_channel_bridge_peer(chan))) {
+ ast_channel_unlock(chan);
+ peer = ast_channel_bridge_peer(chan);
+ ast_channel_lock(chan);
+ if (!peer) {
return f;
}
@@ -3290,7 +3322,11 @@
struct ast_fax_session_details *details;
struct ast_control_t38_parameters *control_params;
RAII_VAR(struct ast_channel *, peer, NULL, ao2_cleanup);
+ RAII_VAR(struct ast_channel *, chan_ref, chan, ao2_cleanup);
int result = 0;
+
+ /* Ref bump the channel for when we have to unlock it */
+ ao2_ref(chan, 1);
details = faxdetect->details;
@@ -3314,7 +3350,10 @@
case AST_FRAMEHOOK_EVENT_DETACHED:
/* restore audio formats when we are detached */
ast_set_read_format(chan, &faxdetect->orig_format);
- if ((peer = ast_channel_bridge_peer(chan))) {
+ ast_channel_unlock(chan);
+ peer = ast_channel_bridge_peer(chan);
+ ast_channel_lock(chan);
+ if (peer) {
ast_channel_make_compatible(chan, peer);
}
return NULL;
@@ -4138,8 +4177,8 @@
* Module loading including tests for configuration or dependencies.
* This function can return AST_MODULE_LOAD_FAILURE, AST_MODULE_LOAD_DECLINE,
* or AST_MODULE_LOAD_SUCCESS. If a dependency or environment variable fails
- * tests return AST_MODULE_LOAD_FAILURE. If the module can not load the
- * configuration file or other non-critical problem return
+ * tests return AST_MODULE_LOAD_FAILURE. If the module can not load the
+ * configuration file or other non-critical problem return
* AST_MODULE_LOAD_DECLINE. On success return AST_MODULE_LOAD_SUCCESS.
*/
static int load_module(void)
More information about the asterisk-commits
mailing list