[Asterisk-code-review] Add mediasec capabilities (asterisk[16])

[Joshua Colp]

Attention is currently required from: Maximilian Fridrich.
Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18837 )

Change subject: Add mediasec capabilities
......................................................................


Patch Set 1: Code-Review-1

(8 comments)

Patchset:

PS1: 
This requires alembic[1] updates, as well as a CHANGES[2] entry, and test coverage.

[1] https://alembic.sqlalchemy.org/en/latest/tutorial.html#create-a-migration-script
[2] https://wiki.asterisk.org/wiki/display/AST/CHANGES+and+UPGRADE.txt


File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/c/asterisk/+/18837/comment/e2a02daf_70e99ffd 
PS1, Line 315: 	SORCERY_OBJECT(details);
This doesn't need to be a sorcery object.


https://gerrit.asterisk.org/c/asterisk/+/18837/comment/f6c4faf8_612341e0 
PS1, Line 397: 	/*! The security mechanism list of the contact (RFC 3329). */
Comment how this is dynamic and can change.


File res/res_pjsip_outbound_registration.c:

https://gerrit.asterisk.org/c/asterisk/+/18837/comment/d8156714_0b5e4c69 
PS1, Line 1038: 			if ((reg = ast_sorcery_retrieve_by_id(ast_sip_get_sorcery(), "registration", response->client_state->registration_name)) &&
This introduces a TON of queries including on cases where this is not applicable. This concerns me. The case where mediasec is not used is by far the most common case, and should not be affected by this - that is it should not introduce additional queries for objects.


https://gerrit.asterisk.org/c/asterisk/+/18837/comment/405ed30f_7efe5d8e 
PS1, Line 1043: 				contact_status = ast_sip_get_contact_status(contact);
All of these objects appear to be leaked.


File res/res_pjsip_rfc3329.c:

https://gerrit.asterisk.org/c/asterisk/+/18837/comment/5d2c6d4c_eacfa39c 
PS1, Line 49: 		contact_status = ast_sip_get_contact_status(session->contact);
contact_status is leaked


https://gerrit.asterisk.org/c/asterisk/+/18837/comment/7e1277f9_476f9a41 
PS1, Line 53: 	if (AST_VECTOR_SIZE(&session->server_security_mechanisms) && contact_has_mechanisms) {
This is not thread safe. Two threads could be operating on contact_status at the same time.


https://gerrit.asterisk.org/c/asterisk/+/18837/comment/23c0ec51_3b5594d3 
PS1, Line 86: 	contact_status = ast_sip_get_contact_status(session->contact);
contact_status is leaked



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ia7f5b5ba42db18074fdd5428c4e1838728586be2
Gerrit-Change-Number: 18837
Gerrit-PatchSet: 1
Gerrit-Owner: Maximilian Fridrich <m.fridrich at commend.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: N A <mail at interlinked.x10host.com>
Gerrit-Attention: Maximilian Fridrich <m.fridrich at commend.com>
Gerrit-Comment-Date: Tue, 26 Jul 2022 14:27:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220726/787428e2/attachment-0001.html>