[Asterisk-code-review] Resolve 2 discrete memory leaks in DTLS/SRTP code path (asterisk[11])

[Joshua Colp]

Joshua Colp has posted comments on this change.

Change subject: Resolve 2 discrete memory leaks in DTLS/SRTP code path
......................................................................


Patch Set 2: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/268/2//COMMIT_MSG
Commit Message:

Line 7: Resolve 2 discrete memory leaks in DTLS/SRTP code path
      : 
      : 1) ao2 ref leak in res_rtp_asterisk.c when a DTLS policy is created.
      : The resources are linked into a table, but the original alloc refs
      : are never released.
      : 
      : 2) ast_strdup leak in rtp_engine.c. If ast_rtp_dtls_cfg_copy() is
      : called twice on the same destination struct, a pointer to an alloc'd
      : string is overwritten before the string is free'd.
Please follow the documentation at https://wiki.asterisk.org/wiki/display/AST/Commit+Messages for constructing a commit message.


https://gerrit.asterisk.org/#/c/268/2/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

Line 1960: 	ao2_t_ref(local_policy, -1, "Drop local_policy alloc ref, now has table ref");
         : 	ao2_t_ref(remote_policy, -1, "Drop remote_policy alloc ref, now has table ref");
This isn't correct. The fact that the policies are implemented as an astobj2 object should be invisible to the caller. To that extent the API provides a destroy callback which is used to destroy the policies.

The fix here would be to invoke the error case further down which destroys the policies, but return the correct value instead of just always returning -1.

This is what the SDES implementation does.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I62a8ceb8679709f6c3769136dc6aa9a68202ff9b
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Steve Davies <steve at one47.co.uk>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: Yes