[Asterisk-code-review] main/config options: Check for existance of internal object ... (asterisk[13])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: main/config_options: Check for existance of internal object before derefing
......................................................................


Patch Set 3:

> Can you provide an example configuration which will do this and
 > what is output? This feels like something that should be caught
 > earlier, so that the object can't be created at all.

Yup. This will do it:

[res_pjsip]
identify=astdb,ps_endpoint_id_ips

Since that isn't managed by res_pjsip, Asterisk does complain about it:

[2015-09-15 15:29:36] WARNING[13638]: config_options.c:978 xmldoc_update_config_type: Cannot update type 'identify' in module 'res_pjsip' because it has no existing documentation!

Run this:

curl -X PUT -u asterisk:asterisk http://localhost:8088/ari/asterisk/config/dynamic/res_pjsip/identify/alice

(assuming asterisk:asterisk is valid) and you'll crash.

If we print out the type at this point, we'll get the following:

(gdb) print *type
$2 = {type = ACO_ITEM, name = 0x2768d08 "identify", category = 0x648d58 ".?", matchfield = 0x0, matchvalue = 0x0, matchfunc = 0x0, category_match = ACO_BLACKLIST, 
  item_offset = 0, hidden = 0, item_alloc = 0x7f8359c629a0 <ip_identify_alloc>, item_find = 0x0, item_pre_process = 0x0, item_prelink = 0x0, internal = 0x0}


At this point, we've already created the item type in sorcery, as evidenced by __ast_sorcery_object_register:

int __ast_sorcery_object_register(struct ast_sorcery *sorcery, const char *type, unsigned int hidden, unsigned int reloadable, aco_type_item_alloc alloc, sorcery_transform_handler transform, sorcery_apply_handler apply)
{
	RAII_VAR(struct ast_sorcery_object_type *, object_type, ao2_find(sorcery->types, type, OBJ_KEY), ao2_cleanup);

	if (!object_type || object_type->type.item_alloc) {
		return -1;
	}

	object_type->type.name = object_type->name;
	object_type->type.type = ACO_ITEM;
	object_type->type.category = ".?";
	object_type->type.item_alloc = alloc;
	object_type->type.hidden = hidden;

	object_type->reloadable = reloadable;
	object_type->transform = transform;
	object_type->apply = apply;
	object_type->file->types[0] = &object_type->type;
	object_type->file->types[1] = NULL;

	if (aco_info_init(object_type->info)) {
		return -1;
	}

	if (ast_sorcery_object_fields_register(sorcery, type, "^@", sorcery_extended_config_handler, sorcery_extended_fields_handler)) {
		return -1;
	}

	NOTIFY_INSTANCE_OBSERVERS(sorcery->observers, object_type_registered,
		sorcery->module_name, sorcery, type);

	return 0;
}

Note that the call that fails is going to be aco_info_init. The object_type->info has been disposed of in config_options, but the actual object_type still exists in the sorcery->types due to a previous call to __ast_sorcery_apply_default - it is, however, now not completely initialized.

Note that the endpoint identifier not loading doesn't matter. We created this type in the sorcery instance managed by res_pjsip. We can't get rid of that. We could ostensibly remove the object_type from the sorcery instance on __ast_sorcery_object_register, but it's hard to say that we can actually fully clean up all the types created by a module if registration fails.

The fact that we have a two step initialization should already make this a moot point - if you're going to let things sneak in between creation and initialization, you should probably check that things are okay from the public APIs.

-- 
To view, visit https://gerrit.asterisk.org/1192
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54079fb94a1440992f4735a9a1bbf1abb1c601ac
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: No