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

increased sip buffer size #3060

Merged
merged 1 commit into from
Sep 12, 2022

Conversation

kenangenjac
Copy link
Contributor

In the last few days we have been getting a lot of spam logs from Janus on our WebRTC platform.
The log messages looks like this: Truncation occurred, 2132 >= 2048, and are usually caused by destination buffer being close to the size of the sofia_log array max size , which is set to 2048 inside janus_sip.c file.

The truncation occurs by invoking janus_strlcat function on the 1744th and 1757th line inside janus_sip_sofia_logger function, located in janus_sip.c. When the janus_strlcat is invoked, destination buffer, source buffer and destination size in bytes are concatenated and truncation occurs.

In this PR I made a change to the sofia_log array, making it 3kB, instead of the original 2kB, as I saw that the return value of the janus_strlcat, which causes the overflow and truncation, is never more than 2.3kB.

@januscla
Copy link

januscla commented Sep 8, 2022

Thanks for your contribution, @kenangenjac! Please make sure you sign our CLA, as it's a required step before we can merge this.

@lminiero
Copy link
Member

lminiero commented Sep 8, 2022

That variable is only needed when using event handlers with the SIP plugin: is this what you're doing? If so, have you checked if you're also impacted by #3031, and if the diff attached in one of the comment helps?

@kenangenjac
Copy link
Contributor Author

We aren't using the event handlers, but we fear we might miss something important due to this message spamming our logs, which is why we proposed this buffer increase.

@lminiero
Copy link
Member

lminiero commented Sep 8, 2022

You're not missing anything, if you're not using event handlers. That said, the patch may still be useful for those that do.

@kenangenjac
Copy link
Contributor Author

Yes, I think it would possibly be useful for others as it could be a general issue. With that in mind, if the PR is not going to be accepted and is not for merging, could You please close it.

@lminiero
Copy link
Member

No need, I'll merge, and backport to 0.x. Thanks!

@lminiero lminiero merged commit 07e5464 into meetecho:master Sep 12, 2022
@kenangenjac
Copy link
Contributor Author

Great! Thank You, glad I contributed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants