Closed vincentfretin closed 1 month ago
Thanks for your contribution, @vincentfretin! Please make sure you sign our CLA, as it's a required step before we can merge this.
That's a good catch, and I think there's indeed a leak in those cases. This should be done for 1.x
too, after merging, since it's missing there too. Just as a note, I don't think you need the if(pts)
check, since g_list_free
is a non-action if you pass NULL
(I noticed we have it in the existing code, but it's unneeded).
Ok, I removed the NULL check, but I didn't test that. If you want me to squash into one commit, I can do that, but you can also do it while you merge if you want.
I tested it, it doesn't crash, so I guess g_list_free(NULL)
works.
@arpu will do some tests to see if this fixes the direct leak he had.
I think those parts of the codes are only involved when using VP9 and/or H.264 profiles, so forcing those (e.g., in the VideoRoom) should help test the changes more quickly.
i can confirm this fixed the janus_sdp_get_codec_pt_full
mem leak with the rust plugin
I squashed the two commits into one and made the same changes for master branch in #3373 I just saw that all files were moved in src directory so it would have not been possible to cherry-pick the commit easily.
I just saw that all files were moved in src directory so it would have not been possible to cherry-pick the commit easily.
Actually, for files that are pretty much the same (and apart for a few major changes, sdp-utils.c
should belong to that category), git is apparently smart enough to figure out the different folder and do the cherry pick anyway. We've used it often for fixes in the AudioBridge, for instance (which is pretty much identical in both branches).
Anyway, looks good to me, thanks! I'll merge both PRs.
Fixed memory leak in
janus_sdp_get_codec_pt_full
,g_list_free(pts)
is not called for early return. This PR is against the 0.x branch.@arpu in #3367 produced this stacktrace for the rust plugin, but as far as I can see this is not a rust issue here. https://github.com/meetecho/janus-gateway/blob/a7767ad30b803d96e11b491547bcf5660cb7a937/sdp-utils.c#L736
pts = g_list_append(pts, GINT_TO_POINTER(pt))
is doing ag_malloc
, but in this particular stacktrace it's not freed, my guess is thatg_list_free(pts)
is not called https://github.com/meetecho/janus-gateway/blob/a7767ad30b803d96e11b491547bcf5660cb7a937/sdp-utils.c#L792 because one of the branch matches and returns early without callingg_list_free(pts)
, probably https://github.com/meetecho/janus-gateway/blob/a7767ad30b803d96e11b491547bcf5660cb7a937/sdp-utils.c#L775 I didn't reproduce the leak myself, it probably depends on the sdp offer the browser send, so I can't know for sure the changes I propose here is fixing the issue.