meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.17k stars 2.47k forks source link

Add note about freeing transaction in handle_message #3374

Closed vincentfretin closed 4 months ago

vincentfretin commented 4 months ago

Add note about freeing transaction after calling push_event in plugin interface documentation.

The only place this was mentioned seems to be in this comment in the janus.c https://github.com/meetecho/janus-gateway/blob/9be6a74452e27c5ae295f39dd1e2d710c25e44df/src/janus.c#L1759 and in plugin implementation like https://github.com/meetecho/janus-gateway/blob/9be6a74452e27c5ae295f39dd1e2d710c25e44df/src/plugins/janus_textroom.c#L1261-L1265

We had a memory leak for this in the rust sfu plugin I fixed in https://github.com/networked-aframe/janus-plugin-sfu/pull/11/commits/4226d0091b386576d221988c76e11c0d0ef7d215 that only showed in ASan output when specifying loop_events option. The original creator of the plugin probably overlooked this because this was not documented in plugin.h.

lminiero commented 4 months ago

Add note about freeing transaction after calling push_event in plugin interface documentation.

This is incorrect. You only need to free transaction if you allocated it yourself, since push_event will treat it as a const. None of the existing plugins free it for instance, since we pass an existing transaction from a message, which typically comes from an incoming request. The transaction you must free is the one you get from the core via handle_message (which all core plugins do, and maybe you're not doing).

vincentfretin commented 4 months ago

You're right, it's the one from handle_message I'm freeing now. I put the comment in the wrong place then, I'll update the PR.

vincentfretin commented 4 months ago

I moved the comment at the right place.

vincentfretin commented 4 months ago

I made the changes. I also added the \c for the note in push_event where I mainly copied the text from.

vincentfretin commented 4 months ago

That's valid feedback. I did the changes.

lminiero commented 4 months ago

Thanks! Merging :+1: