Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sipsess/connect: set sess->established immediately on 200 receival #1128

Merged
merged 1 commit into from
May 27, 2024
Merged

sipsess/connect: set sess->established immediately on 200 receival #1128

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/sipsess/connect.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,15 +148,15 @@ static void invite_resp_handler(int err, const struct sip_msg *msg, void *arg)
}
else if (msg->scode < 300) {

sess->established = true;

sess->hdrs = mem_deref(sess->hdrs);

err = sip_dialog_established(sess->dlg) ?
sip_dialog_update(sess->dlg, msg) :
sip_dialog_create(sess->dlg, msg);
if (err)
goto out;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about this change

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a dialog sending a BYE is not possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dialog is created in sipsess_connect. Even if this fails, a BYE can still be sent.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we send ACK if sip_dialog_create fails?

Copy link
Collaborator

@cspiel1 cspiel1 May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If yes, then I would prefer:

if (sdp && !err) {
/*call answer/offer handler*/
}

instead of a second sipsess_ack()

Copy link
Contributor Author

@maximilianfridrich maximilianfridrich May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two cases for why sip_dialog_create might fail:

  1. We received a malformed response (Contact or Record-Route header are not a valid SIP URI or To tag is not set).
  2. Something internally failed on our side (e.g. ENOMEM).

Regarding point 1: I looked through RFC 3261 thoroughly and I could not find anything that states what should be done when a malformed response is received to an INVITE. At this point in the code a lot of validation has happened before (sip_msg_decode and the Via branch is matched in ctrans.c.).

So I believe sending an ACK is okay even if the Contact or Record-Route headers are malformed - a BYE is sent immediately afterwards anyways to terminate the session.

Same for the second case, I think we should still try to send an ACK even if something failed internally and then we try to terminate the session.


if (sdp) {
if (sdp && !err) {
if (sess->neg_state == SDP_NEG_LOCAL_OFFER) {
sess->neg_state = SDP_NEG_DONE;
err = sess->answerh(msg, sess->arg);
Expand All @@ -179,7 +179,6 @@ static void invite_resp_handler(int err, const struct sip_msg *msg, void *arg)
&& mbuf_get_left(desc))
sess->neg_state = SDP_NEG_DONE;

sess->established = true;
mem_deref(desc);

if (err || sess->terminated)
Expand Down
Loading