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

[Mark Michelson]

Mark Michelson has posted comments on this change.

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


Patch Set 10: Code-Review-1

(4 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.


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()


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->state is ao2_alloced, meaning that all its constituent parts are zeroed by default. In the subsequent lines, internal_state->state's id, type, and tls fields are set/initialized. Then on line 300, you refer to internal_state->state->ciphers, which has not been set to anything at this point. If the intent is just to zero internal_state->state->tls.ciphers, then that's fine. It just seems that if that were the case, a memset to zero would convey the intent more clearly.


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 to build up the state options when reading in configuration files/database rows. The temporary state repeatedly has to be found as each option's custom handler is called.

Storing the temporary state in the ao2 container and looking it up is probably not necessarily slow, but I have a feeling you could save yourself a lot of trouble by not doing it this way.

You could either:
1) Store the temporary state on the transport itself. Once all options have been parsed and it comes time to apply the options, you then copy the temporary state to the transport's permanent state. Anything that actually needs to use the transport references the transport's permanent state, not the temporary state.

2) Use thread-local storage for the temporary state. Retrieve the temporary state from thread-local storage for each option parsed. Then apply that thread-local temporary state to the permanent state once complete.

The idea here is that you presumably could get rid of some of the areas where you have to take different actions based on whether the state is temporary or permanent. You also would not be "polluting" the container of transport states with transient structures.


-- 
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-HasComments: Yes