[Asterisk-code-review] res pjsip: Fix leak of persistent endpoint references. (asterisk[master])

[George Joseph]

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/6657 )

Change subject: res_pjsip: Fix leak of persistent endpoint references.
......................................................................


Patch Set 1:

(2 comments)

https://gerrit.asterisk.org/#/c/6657/1/res/res_pjsip/pjsip_configuration.c
File res/res_pjsip/pjsip_configuration.c:

https://gerrit.asterisk.org/#/c/6657/1/res/res_pjsip/pjsip_configuration.c@1310
PS1, Line 1310: 		persistent->aors = ast_strdup(endpoint->aors);
> Move this outside the 'if (!persistant)' block and add an 'else'
 > block to free the existing aors?

Yeah.  If you move it out, then, you can just...
if (!persistent->aors || strcmp(persistent->aors, endpoint->aors)) {
    ast_free(persistent->aors);
    persistent->aors = ast_strdup(endpoint->aors);

}

 > 
 > In that case if we fail to allocate the aors we'd have to unlink
 > from persistent_endspoints in addition to returning NULL?

If the persistent endpoint exists then the endpoint already exists as well.  If the apply functions returns an error (via returning NULL here), the existing endpoint object is kept so we need to keep the original persistent endpoint as well.

 > 
 > persistant is allocated without a lock, do we need to lock anything
 > to make it safe to free the existing aors?

I don't think we need to.  The endpoint itself is in flux at this point anyway so i'm not sure locking the persistent version just to update the pointer is necessary.


https://gerrit.asterisk.org/#/c/6657/1/res/res_pjsip/pjsip_configuration.c@1320
PS1, Line 1320: 	ao2_ref(persistent->endpoint, +1);
> So maybe this should be moved into the 'if (!persistant)' block?

Hmmm, yeah otherwise we're bumping its ref even on an existing endpoint but you have to know whether this is an apply on an existing endpoint or a new endpoint.  Look at config_transport.  I can't ATM but IIRC it has logic to determine if it's a new object or an existine one.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59bbfc8da8a14d5f4af8c5bb1e71f8592ae823eb
Gerrit-Change-Number: 6657
Gerrit-PatchSet: 1
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Comment-Date: Fri, 06 Oct 2017 17:05:45 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171006/a7513d53/attachment.html>