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

SIP plugin: add missing SIP Contact header in re-INVITE request #3286

Closed ycherniavskyi closed 9 months ago

ycherniavskyi commented 11 months ago

Upon transitioning to a newer version of Sofia SIP (version >= 1.13, inferred from the query_contact_header variable initialization comment), we began encountering errors from certain soft-switches, such as "400 Syntax Check For Dialog Failed: ('header', 'contact')" when executing the "hold" request.

During my investigation, I came across #2708 and #2973 which address this type of issue, albeit not comprehensively.

The first commit in this MR rectifies the "hold" request issue. Following that, the second commit applies the same technique to all nua_invite function calls proactively and also carries out minor refactoring to ensure code consistency.

lminiero commented 10 months ago

Thanks! I'm not entirely sure about the changes. What I noticed is that we seem to in some cases (correctly) refer to the different purpose of handles (master vs helper), while in others we just assume the handle owns the stack without taking into account it might be a helper (and so not own a SIP stack instance of its own). I guess you're not using helpers and so never encountered a problem, but it's something that would probably need to be taken into account as part of this PR (especially considering you're adding this to more requests).

ycherniavskyi commented 10 months ago

Understood.

I propose we divide this PR into two distinct segments:

  1. Inclusion of the Contact header: According to "Table 2: Summary of header fields" in RFC3261 (page 161), the Contact header is a required header for each nua_invite. If this interpretation aligns with your understanding, we can proceed to the next point.
  2. The logic for constructing the contact_header variable differs between nua_subscribe and nua_invite: The current PR adopts the existing approach from nua_invite for consistency. However, if the consensus is to utilize the method of creating contact_header akin to what's used for nua_subscribe, then I will introduce a helper function to harmonize its construction across all usage places.
ycherniavskyi commented 10 months ago

@lminiero, could you kindly advise on any further evidence or explanations required to facilitate the acceptance of this PR?

lminiero commented 10 months ago

I've been busy at the IETF meeting, and I'm only now on my way back home. I'll have a look on Monday.

lminiero commented 10 months ago

Looks good to me! Have you tested this with and without helpers?

evilmuscleman-super commented 10 months ago

111

lminiero commented 10 months ago

@ycherniavskyi I did a couple of tests with master and helper sessions, using INVITE and hold/unhold, and it seems to be working fine. If you could do some more tests with the other requests you updated, that might help, since my SIP setup is quite limited. Once that's done, I think it will be good to merge for me :+1:

lminiero commented 9 months ago

I see no replies to my request. Rather than wait forever for feedback, I'll merge, and in case issues are reported, revert the commit. Thanks :+1:

ycherniavskyi commented 9 months ago

@lminiero, apologies for the delayed response. I wanted to conduct thorough testing in my environment before commenting. As of today, I've completed my tests and am ready to report that everything works as expected (in my environment) 😊.