[asterisk-dev] dvossel: branch 1.4 r183126 - in /branches/1.4: apps/ include/asterisk/ res/

[Russell Bryant]

SVN commits to the Digium repositories wrote:
> Author: dvossel
> Date: Thu Mar 19 11:15:16 2009
> New Revision: 183126
> 
> URL: http://svn.digium.com/svn-view/asterisk?view=rev&rev=183126
> Log:
> Allow disconnect feature before a call is bridged
> 
> feature.conf has a disconnect option.  By default this option is set to '*', but it could be anything.  If a user wishes to disconnect a call before the other side answers, only '*' will work, regardless if the disconnect option is set to something else.  This is because features are unavailable until bridging takes place.  The default disconnect option, '*', was hardcoded in app_dial, which doesn't make any sense from a user perspective since they may expect it to be something different.  This patch allows features to be detected from outside of the bridge, but not operated on.  In this case, the disconnect feature can be detected before briding and handled outside of features.c.
> 
> (closes issue #11583)
> Reported by: sobomax
> Patches:
> 	patch-apps__app_dial.c uploaded by sobomax (license 359)
> 	11583.latest-patch uploaded by murf (license 17)
> 	detect_disconnect.diff uploaded by dvossel (license 671)
> Tested by: sobomax, dvossel
> Review: http://reviewboard.digium.com/r/195/

> +static int detect_disconnect(struct ast_channel *chan, char code, char *featurecode, int len)
> +{
> +	struct ast_flags features = { AST_FEATURE_DISCONNECT }; /* only concerned with disconnect feature */
> +	struct ast_call_feature feature;
> +	char *tmp;
> +	int res;
> +
> +	if ((strlen(featurecode)) < (len - 2)) { 
> +		tmp = &featurecode[strlen(featurecode)];
> +		tmp[0] = code;
> +		tmp[1] = '\0';
> +	} else {
> +		featurecode[0] = 0;
> +		return -1; /* no room in featurecode buffer */
> +	}
> +
> +	res = ast_feature_detect(chan, &features, featurecode, &feature);
> +
> +	if (res != FEATURE_RETURN_STOREDIGITS) {
> +		featurecode[0] = '\0';
> +	}
> +	if (feature.feature_mask & AST_FEATURE_DISCONNECT) {
> +		return 1;
> +	}
> +
> +	return 0;
>  }

ast_feature_detect() does not guarantee that the ast_call_feature 
argument gets set to something.  So, you have the potential for 
accessing uninitialized memory at the end of this function.  You should 
simply ensure that feature is initialized when it is declared at the 
beginning of this function.

> +/*! \brief detect a feature before bridging 
> +    \param chan, ast_flags ptr, code, ast_call_feature ptr to be set if found */
> +int ast_feature_detect(struct ast_channel *chan, struct ast_flags *features, char *code, struct ast_call_feature *feature);

The format of this documentation is not valid.  Each parameter must be 
documented with its own \param tag.  Also, the return values should be 
documented, as well, using the \return tag or \retval tags.

> +int ast_feature_detect(struct ast_channel *chan, struct ast_flags *features, char *code, struct ast_call_feature *feature) {
> +
> +	char *dynamic_features;
> +	ast_channel_lock(chan);
> +	dynamic_features = ast_strdupa(S_OR(pbx_builtin_getvar_helper(chan, "DYNAMIC_FEATURES"),""));
> +	ast_channel_unlock(chan);
> +
> +	return feature_interpret_helper(chan, NULL, NULL, code, 0, dynamic_features, features, 0, feature);
> +}

feature_interpret_helper() actually executes the feature if it is 
matched.  So, in the case of app_dial, when you _only_ want to detect 
and execute the disconnect feature, you are now also detecting and 
executing any dynamic features that have been enabled on this channel. 
This is undesired behavior.

-- 
Russell Bryant
Digium, Inc. | Senior Software Engineer, Open Source Team Lead
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org