[Asterisk-code-review] res pjsip: Fix infinite recursion when loading transports f... (asterisk[13])

[George Joseph]

George Joseph has posted comments on this change.

Change subject: res_pjsip:  Fix infinite recursion when loading transports from realtime
......................................................................


Patch Set 10:

(5 comments)

https://gerrit.asterisk.org/#/c/2129/10/res/res_pjsip/config_transport.c
File res/res_pjsip/config_transport.c:

Line 218: 	if (state->id) {
        : 		ast_free(state->id);
        : 	}
> This if is not necessary. It's fine to call ast_free() on a NULL pointer.
Done


Line 250: 		key = ast_alloca(MAX_POINTER_STRING);
        : 		if (!key) {
        : 			return NULL;
        : 		}
> FYI: ast_alloca() effectively can never fail since it is allocating from th
Done


Line 245: 	const char *key;
        : 
        : 	if (type == PERMANENT_STATE) {
        : 		key = ast_sorcery_object_get_id(transport);
        : 	} else {
        : 		key = ast_alloca(MAX_POINTER_STRING);
        : 		if (!key) {
        : 			return NULL;
        : 		}
        : 		snprintf((char *)key, MAX_POINTER_STRING, "%p", transport);
        : 	}
        : 
        : 	state = ao2_find(transport_states, key, OBJ_SEARCH_KEY | OBJ_NOLOCK);
> This can be replaced with a call to find_internal_state_by_transport()
Done


Line 291: 	internal_state->state = ao2_alloc(sizeof(*internal_state->state), sip_transport_state_destroy);
        : 	if (!internal_state->state) {
        : 		ao2_cleanup(internal_state);
        : 		return NULL;
        : 	}
        : 	internal_state->state->id = ast_strdup(internal_state->id);
        : 	internal_state->state->type = transport->type;
        : 
        : 	pjsip_tls_setting_default(&internal_state->state->tls);
        : 	internal_state->state->tls.ciphers = internal_state->state->ciphers;
> I don't think line 300 is going to do anything productive. internal_state->
This was a cut and paste from the original sip_transport_alloc.  I think the original-original intent was that setting state->tls.ciphers (which is a pointer) to the address of state->ciphers (which is an array) was so that when transport_cipher_add adds ciphers to state->cipher, they're automatically available in the tls struct.  Unfortunately the array was publically exposed in transport.


Line 312: static struct ast_sip_transport_state *find_or_create_temporary_state(struct ast_sip_transport *transport)
> If I understand correctly, the reason for temporary states is essentially t
Well, the ultimate goal is to remove the non-sorcery items from the transport structure altogether so storing it on transport is probably not a good idea.  They'll actually be a follow-on for master to remove all the deprecated items.

I can look at tls.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7a836ea8e786e8def51fe3f8cce855ea54f5f19
Gerrit-PatchSet: 10
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes