Fix segfault introduced by conversion to ACO API
authorTerry Wilson <twilson@digium.com>
Sat, 21 Jul 2012 13:25:26 +0000 (13:25 +0000)
committerTerry Wilson <twilson@digium.com>
Sat, 21 Jul 2012 13:25:26 +0000 (13:25 +0000)
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

apps/app_confbridge.c
apps/confbridge/conf_config_parser.c
main/config_options.c

index 9b1df34..2ea8d15 100644 (file)
@@ -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;
 }
 
index dc81e3c..f4a9604 100644 (file)
@@ -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))) {
index a958342..1801f93 100644 (file)
@@ -606,10 +606,12 @@ static int internal_type_init(struct aco_type *type)
 
        if (!(type->internal->regex = build_regex(type->category))) {
                internal_type_destroy(type);
+               return -1;
        }
 
        if (!(type->internal->opts = aco_option_container_alloc())) {
                internal_type_destroy(type);
+               return -1;
        }
 
        return 0;