[Asterisk-code-review] bridges: Remove reliance on stasis caching (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/10227 )

Change subject: bridges:  Remove reliance on stasis caching
......................................................................


Patch Set 6: Code-Review-1

(20 comments)

https://gerrit.asterisk.org/#/c/10227/6/CHANGES
File CHANGES:

https://gerrit.asterisk.org/#/c/10227/6/CHANGES@15
PS6, Line 15: Bridging
            : ------------------
This change isn't quite in the format of the rest of the file.


https://gerrit.asterisk.org/#/c/10227/6/UPGRADE.txt
File UPGRADE.txt:

https://gerrit.asterisk.org/#/c/10227/6/UPGRADE.txt@30
PS6, Line 30: Bridging
            : ------------------
This change isn't quite in the format of the rest of the other UPGRADE files.


https://gerrit.asterisk.org/#/c/10227/6/include/asterisk/bridge.h
File include/asterisk/bridge.h:

https://gerrit.asterisk.org/#/c/10227/6/include/asterisk/bridge.h@77
PS6, Line 77: #include "asterisk/astobj2.h"
Since the only thing this header file needs from astobj2.h is the ao2_container struct name, this include could be replaced by:

struct ao2_container;


https://gerrit.asterisk.org/#/c/10227/6/main/bridge.c
File main/bridge.c:

https://gerrit.asterisk.org/#/c/10227/6/main/bridge.c@5068
PS6, Line 5068: 		ast_cli(a->fd, FORMAT_ROW,
              : 			bridge->uniqueid,
              : 			bridge->num_channels,
              : 			S_OR(bridge->v_table->name, "<unknown>"),
              : 			S_OR(bridge->technology->name, "<unknown>"));
Get the latest snapshot on the bridge to display this information as it is immutable.  ast_bridge_get_snapshot() and check for NULL return.

Accessing things in bridge without locking it is dangerous.  The worst one is getting the bridge->technology->name as that is most likely to change while being accessed.


https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c
File main/manager_bridges.c:

https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@451
PS6, Line 451: 	struct ast_bridge_snapshot *snapshot = ast_bridge_get_snapshot(bridge);
It is possible for snapshot to be NULL


https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@454
PS6, Line 454: 	RAII_VAR(struct ast_str *, bridge_info, ast_manager_build_bridge_state_string(snapshot), ast_free);
* Since the bridge filtering is done in this function, you shouldn't generate the bridge_info string before the bridge is filtered.

* This RAII_VAR can be eliminated after the bridge is filtered.


https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@458
PS6, Line 458: 	ast_log(LOG_ERROR, "f: %s br: %s %s\n", S_OR(list_data->type_filter, "none"), bridge->uniqueid, bridge->technology->name);
This ERROR message appears to be left over debugging.


https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@460
PS6, Line 460: 	if (!ast_strlen_zero(list_data->type_filter) && strcmp(list_data->type_filter, bridge->technology->name))
Use the bridge snapshot not the live bridge object to check the technology.  You could crash without locking the bridge.


https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@460
PS6, Line 460: 	if (!ast_strlen_zero(list_data->type_filter) && strcmp(list_data->type_filter, bridge->technology->name))
             : 	{
guidelines: curly
guidelines: wrap long line


https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@489
PS6, Line 489: 		list_data.id_text = ast_alloca(strlen(id) + strlen("ActionID: \r\n") + 1);
This opens us up to a security issue.  Allocating from the stack a string that comes from the outside world could blow the stack if an attacker sent a very very long string.


https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@502
PS6, Line 502: 		list_data.type_filter = ast_strdupa(type_filter);
Why allocate this from the stack?  Just put the pointer into list_data.type_filter.


https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@575
PS6, Line 575: 	snapshot = ast_bridge_get_snapshot_by_uniqueid(bridge_uniqueid);
We may not find the snapshot if the bridge no longer exists.
Check for NULL.


https://gerrit.asterisk.org/#/c/10227/6/main/stasis_bridges.c
File main/stasis_bridges.c:

https://gerrit.asterisk.org/#/c/10227/6/main/stasis_bridges.c@1323
PS6, Line 1323: 	ao2_lock(bridge);
use ast_bridge_lock/ast_bridge_unlock instead


https://gerrit.asterisk.org/#/c/10227/6/main/stasis_bridges.c@1324
PS6, Line 1324: 	snapshot = bridge->current_snapshot;
bump snapshot here under lock protection


https://gerrit.asterisk.org/#/c/10227/6/main/stasis_bridges.c@1338
PS6, Line 1338: 	ao2_lock(bridge);
use ast_bridge_lock/ast_bridge_unlock instead


https://gerrit.asterisk.org/#/c/10227/6/main/stasis_bridges.c@1339
PS6, Line 1339: 	snapshot = bridge->current_snapshot;
bump snapshot under lock protection


https://gerrit.asterisk.org/#/c/10227/6/res/ari/resource_bridges.c
File res/ari/resource_bridges.c:

https://gerrit.asterisk.org/#/c/10227/6/res/ari/resource_bridges.c@907
PS6, Line 907: 		struct ast_bridge_snapshot *snapshot = ast_bridge_get_snapshot(bridge);
Need to protect from a NULL snapshot


https://gerrit.asterisk.org/#/c/10227/6/res/stasis/app.c
File res/stasis/app.c:

https://gerrit.asterisk.org/#/c/10227/6/res/stasis/app.c@183
PS6, Line 183: 	if (!forwards->topic_forward && bridge) {
             : 		forwards_unsubscribe(forwards);
             : 		ao2_ref(forwards, -1);
             : 		return NULL;
             : 	}
Missed this change:

if (!forwards->topic_forward) {
	ao2_ref(forwards, -1);
	return NULL;
}


https://gerrit.asterisk.org/#/c/10227/6/tests/test_cel.c
File tests/test_cel.c:

PS6: 
The leaks fixed in this file should be put into their own patch as they apply to v13 too.


https://gerrit.asterisk.org/#/c/10227/6/tests/test_cel.c@2143
PS6, Line 2143: 	ao2_cleanup(cel_expected_events);
              : 	ao2_cleanup(cel_received_events);
Should set these containers to NULL to better ensure that the unit test can be run again.



-- 
To view, visit https://gerrit.asterisk.org/10227
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7049b80efa88676ce5c4666f818fa18ad1985369
Gerrit-Change-Number: 10227
Gerrit-PatchSet: 6
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 18:51:51 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181019/558ef00b/attachment.html>