[Asterisk-code-review] ARI: Added new functionality to get information on a single ... (asterisk[13])

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

Change subject: ARI: Added new functionality to get information on a single module.
......................................................................


Patch Set 3: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/790/3/res/ari/resource_asterisk.c
File res/ari/resource_asterisk.c:

Line 170: 	module_info = ast_json_pack("{s: s, s: s, s: i, s: s, s: s}",
        : 	                            "name", module,
        : 	                            "description", description,
        : 	                            "use_count", usecnt,
        : 	                            "status", status,
        : 	                            "support_level", ast_module_support_level_to_string(support_level));
        : 
        : 	if (!module_info) {
        : 		return 0;
        : 	}
        : 
        : 	*out = module_info;
In the "get_module" code you create a json object and it gets passed in to this function as "data", which is pointed to by the out variable. However, the out variable pointer is then made to point to module_info which is a newly created json object, thus memory is leaked.

A couple of choices here:

1) don't create the json object in "get_module" and the memory won't be leaked.

2) Instead of using ast_json_pack here use ast_json_object_set. Then you can also drop the last few lines (module_info check and setting the *out variable).

Option 2 might work slightly better in this scenario since the data here might be passed to another callback and it makes more sense to update the data object than replace it.


Line 199: 	if (ast_strlen_zero(args->module_name)) {
        : 		ast_ari_response_error(
        : 		  response, 400, "Bad Request",
        : 		  "Module name is required");
        : 		return;
        : 	}
        : 
        : 	if (!ast_module_check(args->module_name)) {
        : 		ast_ari_response_error(
        : 		  response, 404, "Not Found",
        : 		  "Module does not exist");
        : 		return;
        : 	}
These two error paths leak the json object. I'd just create the json object after these checks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibce5a94e70ecdf4e90329cf0ba66c33a62d37463
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes