Fix segfault introduced by conversion to ACO API

The value "none" is specified in the config file as a valid value for
the "video_mode" option. The code prior to the ACO conversion did not
check for "none", but just ignored it and relied on the default zero
value. The parsing with ACO is more strict, so without handling
"none" specifically, parsing would fail.

When parsing failed, but the module loaded anyway, the config info
would never be stored, and one place in the code did not check for
this case and would segfault. It was also possible that the
aco_info struct's internals would be destroyed and used as well.

This patch keeps the module from loading after parse failures, adds
the "none" option to "video_mode", registers CLI functions only
after parsing has completed, checks the config data for NULL before
accessing it, and returns -1 on some allocation failures when
initializing.


(closes issue ASTERISK-20159)
Reported by: Birger "WIMPy" Harzenetter
Tested by: Birger "WIMPy" Harzenetter
Patches:
    confbridge_fix3.txt uploaded by Terry Wilson


git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@370341 65c4cc65-6c06-0410-ace0-fbb531ad65f3

2 files changed, 18 insertions, 6 deletions

diff --git a/apps/app_confbridge.c b/apps/app_confbridge.c
index 9b1df3404..2ea8d154e 100644
--- a/apps/app_confbridge.c
+++ b/apps/app_confbridge.c
@@ -2866,7 +2866,8 @@ static int load_module(void)
 	res |= ast_manager_register_xml("ConfbridgeStopRecord", EVENT_FLAG_CALL, action_confbridgestoprecord);
 	res |= ast_manager_register_xml("ConfbridgeSetSingleVideoSrc", EVENT_FLAG_CALL, action_confbridgesetsinglevideosrc);
 
-	conf_load_config(0);
+	res |= conf_load_config(0);
+
 	return res;
 }
 
diff --git a/apps/confbridge/conf_config_parser.c b/apps/confbridge/conf_config_parser.c
index dc81e3cc8..f4a960410 100644
--- a/apps/confbridge/conf_config_parser.c
+++ b/apps/confbridge/conf_config_parser.c
@@ -1183,6 +1183,8 @@ static int video_mode_handler(const struct aco_option *opt, struct ast_variable
 		ast_set_flag(b_profile, BRIDGE_OPT_VIDEO_SRC_LAST_MARKED);
 	} else if (!strcasecmp(var->value, "follow_talker")) {
 		ast_set_flag(b_profile, BRIDGE_OPT_VIDEO_SRC_FOLLOW_TALKER);
+	} else if (!strcasecmp(var->value, "none")) {
+		return 0;
 	} else {
 		return -1;
 	}
@@ -1255,10 +1257,7 @@ int conf_load_config(int reload)
 {
 	if (!reload) {
 		if (aco_info_init(&cfg_info)) {
-			goto error;
-		}
-		if (ast_cli_register_multiple(cli_confbridge_parser, ARRAY_LEN(cli_confbridge_parser))) {
-			goto error;
+			return -1;
 		}
 	}
 
@@ -1307,7 +1306,15 @@ int conf_load_config(int reload)
 	/* Menu options */
 	aco_option_register_custom(&cfg_info, "^[0-9A-D*#]+$", ACO_REGEX, menu_types, NULL, menu_option_handler, 0);
 
-	return aco_process_config(&cfg_info, reload) == ACO_PROCESS_ERROR;
+	if (aco_process_config(&cfg_info, reload) == ACO_PROCESS_ERROR) {
+		goto error;
+	}
+
+	if (!reload && ast_cli_register_multiple(cli_confbridge_parser, ARRAY_LEN(cli_confbridge_parser))) {
+		goto error;
+	}
+
+	return 0;
 error:
 	conf_destroy_config();
 	return -1;
@@ -1380,6 +1387,10 @@ const struct bridge_profile *conf_find_bridge_profile(struct ast_channel *chan,
 	struct func_confbridge_data *b_data = NULL;
 	RAII_VAR(struct confbridge_cfg *, cfg, ao2_global_obj_ref(cfg_handle), ao2_cleanup);
 
+	if (!cfg) {
+		return NULL;
+	}
+
 	if (chan) {
 		ast_channel_lock(chan);
 		if ((datastore = ast_channel_datastore_find(chan, &confbridge_datastore, NULL))) {