Kgenjac/save sip reason state on callback

[kenangenjac]

Hello,

We didn't have variables from the sip reason header on failure events such as nua_i_terminated or on nua_i_state event where sip is NULL, so this PR adds an option to save the state of these variables.

[lminiero]

Not sure what specific problem this is trying to fix, but this will definitely cause memory leaks when nua_i_bye and nua_i_cancel are handled, since a new g_strdup would be done in their branches, replacing the value of the variable you allocate in your patch making the previous one memory that will never be freed. I'd say that rather than always doing it, as you're doing now, it would be better to add that info to the cases you're interested in.

[kenangenjac]

Oh yes, I overlooked that, thank you for pointing that out.

The problem is that we need to read the reason header from final SIP responses, but SIP object is only present on nua_i_bye and nua_i_cancel events, whereas on nua_i_state and nua_i_terminated SIP object is NULL, so we cannot get the SIP reason header in those cases.

If it is a satisfiable solution, I think freeing the memory and setting these variables to NULL before a new g_strdup in nua_i_bye and nua_i_cancel branches would solve this problem.

[lminiero]

I'm not sure that would be enough: your code doesn't do any g_free before the g_strdup either, meaning that a state followed by a terminated or viceversa would still cause the same leak. Maybe a simpler fix is to only set those variables where you do (so removing the allocation in cancel/bye) and ensuring there's a free before the strdup, even though I feel it could be semantically inaccurate, and there would be a ton of reallocations any time there's an incoming message (who all have a reason, even e.g., 200 OK).

Or maybe you should figure out why you're not getting the info in your case: I understand state/terminated don't have the sip object, but the call before probably does, are you getting to those states not via cancel/bye but somehow else? Maybe it's there that the reason string should also be added to? If you are going there via cancel/bye, I'd argue that you do have the reason: you need to intercept bye/cancel events too.

[kenangenjac]

After some exploring, I found that we could use SIP reason received on nua_r_invite with the similar logic of allocation as in nua_i_bye. For reference, this diff shows what I mean: https://github.com/meetecho/janus-gateway/compare/master...kenangenjac:janus-gateway:kgenjac/log-reasons

If this seems okay, I would discard the previous changes and update the PR with the new ones.

[lminiero]

That still saves the response code even for provisional and successful responses too. Shouldn't that code only be in the error branch? e.g., error codes >= 400?

[kenangenjac]

Yes, I updated the PR to save the reason only on error codes > 400, I also added it for 401 and 407 cases and synced with master. Also, I moved the code to a method as it would now be repeated 4 times. If the code is okay, I hope the method naming and method placement is okay, too.

[lminiero]

Considering my additional comment, I think it's safe to merge as it. We can then track if memory leaks occur anyway. Thanks!

[kenangenjac]

Thank you for merging!

[lminiero]

On second thought I think I'll add the g_free anyway, just to stay on the safe side.