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.

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

Add newline for SIP headers that are overflown in length #3184

Merged
merged 3 commits into from
Apr 18, 2023

Conversation

zayim
Copy link
Contributor

@zayim zayim commented Mar 17, 2023

Before this PR, if some header (key+value) is overflown (longer than 256 chars), it would result in it being truncated, but no newline was added, so it would result in next header actually being written on same line as overflown one, hence it was not interpreted as separate header on SIP level.

This PR makes sure that newline is added always, even if header (key+value) is longer than 256 chars.

@lminiero
Copy link
Member

I'm not sure I understand why this should work. If the original string fills the string already, trying to append \r\n with janus_strlcat after that would still result in truncation and those two characters not being added. Am I missing something?

@zayim
Copy link
Contributor Author

zayim commented Mar 17, 2023

that might be true if we overflow entire custom_headers string, I was solving the case when we overflow one single header that is being written in h string.

I can add handling of the case when entire custom_headers string is overflown, make sure that we always have newline at the end of it

@lminiero
Copy link
Member

I think that might help, thanks!

@zayim
Copy link
Contributor Author

zayim commented Mar 23, 2023

When you have time, check this again, it is now guaranteed that custom_headers will also end with \r\n, even if it is overflown.

@lminiero
Copy link
Member

Hi @zayim, apologies for this very late reply, but I've been abroad the past few weeks and have just come back to work. I'll review the changes later today and let you know, thanks!

@lminiero lminiero added the multistream Related to Janus 1.x label Apr 11, 2023
@lminiero
Copy link
Member

Sorry again for the late review, was sidetracked by other activities... merging, thanks! I'll backport this to 0.x too, since it's an easy patch to apply.

@lminiero lminiero merged commit 1be58ce into meetecho:master Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants