[Asterisk-code-review] res pjsip exten state: Use the extension for publishing to. (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

Change subject: res_pjsip_exten_state: Use the extension for publishing to.
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/2808/1//COMMIT_MSG
Commit Message:

PS1, Line 13: his also extends extends
two extends.


https://gerrit.asterisk.org/#/c/2808/1/res/res_pjsip_outbound_publish.c
File res/res_pjsip_outbound_publish.c:

PS1, Line 538: 	struct sip_outbound_publisher *publisher;
             : 
             : 	publisher = ao2_find(client->publishers, user, OBJ_SEARCH_KEY);
             :         if (!publisher) {
             : 		SCOPED_WRLOCK(lock, &load_lock);
             : 		if (!(publisher = sip_outbound_publish_client_add_publisher(client, user))) {
             : 			return NULL;
             : 		}
             : 	}
This has a potential race condition between searching and adding the publisher.


PS1, Line 564: 	struct sip_outbound_publisher *publisher;
             : 
             : 	publisher = ao2_find(client->publishers, user, OBJ_SEARCH_KEY);
             : 	if (!publisher) {
             : 		SCOPED_WRLOCK(lock, &load_lock);
             : 		if (!(publisher = sip_outbound_publish_client_add_publisher(client, user))) {
             : 			return NULL;
             : 		}
             : 	}
Same race condition is here too (between searching and adding the publisher).

Since this code is identical to that in the "get from" and "ast_sip_publish_client_user_send" functions I'd suggest a separate utility function for it.


PS1, Line 945: 	/* The sip_outbound_publisher_set_uris ensures the From and To are NULL terminated */
             : 	publisher->from_uri = ast_strdup(from_uri.ptr);
             : 	if (!publisher->from_uri) {
             : 		pjsip_endpt_release_pool(ast_sip_get_pjsip_endpoint(), pool);
             : 		pjsip_publishc_destroy(publisher->client);
             : 		return -1;
             : 	}
             : 
             : 	publisher->to_uri = ast_strdup(to_uri.ptr);
             : 	if (!publisher->to_uri) {
             : 		pjsip_endpt_release_pool(ast_sip_get_pjsip_endpoint(), pool);
             : 		pjsip_publishc_destroy(publisher->client);
             : 		return -1;
             : 	}
Moving these string dupes into sip_outbound_publisher_set_uris might simplify the code a little.

Also could these values possibly be set more than once? Should they first be freed before setting?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I668fdf697b1e171d4c7e6f282b2e1590f8356ca1
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes