[Asterisk-code-review] res_pjsip_registrar: Remove unavailable contacts if exceeds max_contacts (asterisk[16])

[Kevin Harwell]

Attention is currently required from: Sean Bright, George Joseph, Joe.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16160 )

Change subject: res_pjsip_registrar: Remove unavailable contacts if exceeds max_contacts
......................................................................


Patch Set 11: Code-Review-1

(2 comments)

File res/res_pjsip_registrar.c:

https://gerrit.asterisk.org/c/asterisk/+/16160/comment/72d87b5a_9fe9ccf6 
PS11, Line 733: 			remove_excess_contacts(unavail_contacts, contacts, contact_count - aor->max_contacts, aor->remove_existing);
If for some reason 'unavail_contacts' is NULL here (which it could because 'ast_sip_location_retrieve_aor_contacts_filtered' could return such) then this will crash within the function call.


https://gerrit.asterisk.org/c/asterisk/+/16160/comment/af383db4_f192e797 
PS11, Line 736: 			contacts = ast_sip_location_retrieve_aor_contacts_nolock(aor);
              : 			if (!contacts) {
              : 				response->code = 500;
              : 				pjsip_endpt_release_pool(ast_sip_get_pjsip_endpoint(), details.pool);
              : 				return;
              : 			}
Were you able to determine why the contacts are not being removed from the 'contacts' container in the 'remove_excess_contacts' call? Or why the 'contacts' object is not properly updating? It doesn't seem like there is a reason why you'd have to re-retrieve contacts.

As is, I don't think this is the right thing to do here as I am not sure what side effects could occur by doing such. 

First, 'ast_sip_location_retrieve_aor_contacts_nolock' returns a reference counted object. I'm not seeing where it gets unref'd, so that means upon function return there will be a ref leak on contacts.

Second, in this function, 'contacts' is a pointer (or copy of one) local to this function to a reference counted object. With this code here, the calling function has a list of contacts, and now this function has a separate list which may differ in memory. Thus there is the potential for them to be out of sync which may cause problems elsewhere/later (i.e. upon return the original container may be 'stale')



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ia2711b08f2b4d1177411b1be23e970d7fdff5784
Gerrit-Change-Number: 16160
Gerrit-PatchSet: 11
Gerrit-Owner: Joe <ynadiv at corpit.xyz>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-CC: George Joseph <gjoseph at digium.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Attention: Joe <ynadiv at corpit.xyz>
Gerrit-Comment-Date: Tue, 21 Sep 2021 22:21:31 +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/20210921/82994551/attachment-0001.html>