<p> Attention is currently required from: Maximilian Fridrich. </p>
<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/18837">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Patchset:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18837?tab=comments">Patch Set #1:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">This requires alembic[1] updates, as well as a CHANGES[2] entry, and test coverage.</p><p style="white-space: pre-wrap; word-wrap: break-word;">[1] https://alembic.sqlalchemy.org/en/latest/tutorial.html#create-a-migration-script<br>[2] https://wiki.asterisk.org/wiki/display/AST/CHANGES+and+UPGRADE.txt</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/res_pjsip.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18837/comment/e2a02daf_70e99ffd">Patch Set #1, Line 315:</a> <code style="font-family:monospace,monospace"> SORCERY_OBJECT(details);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This doesn't need to be a sorcery object.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18837/comment/f6c4faf8_612341e0">Patch Set #1, Line 397:</a> <code style="font-family:monospace,monospace"> /*! The security mechanism list of the contact (RFC 3329). */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Comment how this is dynamic and can change.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip_outbound_registration.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18837/comment/d8156714_0b5e4c69">Patch Set #1, Line 1038:</a> <code style="font-family:monospace,monospace"> if ((reg = ast_sorcery_retrieve_by_id(ast_sip_get_sorcery(), "registration", response->client_state->registration_name)) &&</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18837/comment/405ed30f_7efe5d8e">Patch Set #1, Line 1043:</a> <code style="font-family:monospace,monospace"> contact_status = ast_sip_get_contact_status(contact);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">All of these objects appear to be leaked.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip_rfc3329.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18837/comment/5d2c6d4c_eacfa39c">Patch Set #1, Line 49:</a> <code style="font-family:monospace,monospace"> contact_status = ast_sip_get_contact_status(session->contact);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">contact_status is leaked</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18837/comment/7e1277f9_476f9a41">Patch Set #1, Line 53:</a> <code style="font-family:monospace,monospace"> if (AST_VECTOR_SIZE(&session->server_security_mechanisms) && contact_has_mechanisms) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is not thread safe. Two threads could be operating on contact_status at the same time.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18837/comment/23c0ec51_3b5594d3">Patch Set #1, Line 86:</a> <code style="font-family:monospace,monospace"> contact_status = ast_sip_get_contact_status(session->contact);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">contact_status is leaked</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18837">change 18837</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/18837"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Ia7f5b5ba42db18074fdd5428c4e1838728586be2 </div>
<div style="display:none"> Gerrit-Change-Number: 18837 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Maximilian Fridrich <m.fridrich@commend.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Attention: Maximilian Fridrich <m.fridrich@commend.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 26 Jul 2022 14:27:55 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>