Fixed memory leak in janus_sdp_get_codec_pt_full (0.x branch)

[vincentfretin]

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.

janus-gateway/sdp-utils.c

Line 736 in a7767ad

pts = g_list_append(pts, GINT_TO_POINTER(pt));

pts = g_list_append(pts, GINT_TO_POINTER(pt)) is doing a g_malloc, but in this particular stacktrace it's not freed, my guess is that g_list_free(pts) is not called

janus-gateway/sdp-utils.c

Line 792 in a7767ad

g_list_free(pts);

because one of the branch matches and returns early without calling g_list_free(pts), probably

janus-gateway/sdp-utils.c

Line 775 in a7767ad

return pt;

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.

Direct leak of 48 byte(s) in 2 object(s) allocated from:
    #0 0x7f79fef33b37 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f79feaa6af9 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62af9) (BuildId: 49d1a4c955cc447b21862b89b3891779a39b2b02)
    #2 0x7f79fea9be20 in g_list_append (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e20) (BuildId: 49d1a4c955cc447b21862b89b3891779a39b2b02)
    #3 0x56482e41378d in janus_sdp_get_codec_pt_full /janus-gateway/sdp-utils.c:736
    #4 0x7f79ee209149 in janus_plugin::sdp::Sdp::get_payload_type_full::h72f70b2a7a12139d src/sdp.rs:183
    #5 0x7f79ee1515fc in janus_plugin_sfu::process_offer::hd385b4c7c84200b3 src/lib.rs:646
    #6 0x7f79ee153a8c in janus_plugin_sfu::process_jsep::h28774db68fc38472 src/lib.rs:705
    #7 0x7f79ee1364ef in janus_plugin_sfu::handle_message_async::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::hf5f0ab61e8bca90c src/lib.rs:734
    #8 0x7f79ee19536e in core::result::Result$LT$T$C$E$GT$::and_then::hbf38a3c22ea8ffb5 /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:1321
    #9 0x7f79ee155a52 in janus_plugin_sfu::handle_message_async::h4d531760d491b8fe src/lib.rs:734
    #10 0x7f79ee1351d2 in janus_plugin_sfu::init::_$u7b$$u7b$closure$u7d$$u7d$::h33f120f8cd1f2efd src/lib.rs:278
    #11 0x7f79ee173821 in std::sys_common::backtrace::__rust_begin_short_backtrace::hfbe9a55541accd3f /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:155
    #12 0x7f79ee0b8582 in std::panic::catch_unwind::h5cbf28717df94206 /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:149
    #13 0x7f79ee19b42d in core::ops::function::FnOnce::call_once$u7b$$u7b$vtable.shim$u7d$$u7d$::he3d78cfb2c60c061 /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250
    #14 0x7f79ee727196 in _$LT$alloc..boxed..Box$LT$F$C$A$GT$$u20$as$u20$core..ops..function..FnOnce$LT$Args$GT$$GT$::call_once::h5a7c9b5d720b4a3d /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:2022
    #15 0x7f79ee78b3e3 in std::sys::pal::unix::thread::Thread::new::thread_start::hcfd4651d9724d93f src/sys/pal/unix/thread.rs:108
    #16 0x7f79fee96109 in asan_thread_start ../../../../src/libsanitizer/asan/asan_interceptors.cpp:234

[januscla]

Thanks for your contribution, @vincentfretin! Please make sure you sign our CLA, as it's a required step before we can merge this.

[lminiero]

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).

[vincentfretin]

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.

[vincentfretin]

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.

[lminiero]

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.

[arpu]

i can confirm this fixed the janus_sdp_get_codec_pt_full mem leak with the rust plugin

[vincentfretin]

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.

[lminiero]

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.