Improve handling of some buffer size limits.

[codyherzog-zz]

Hello.

Several of my scenarios use TCP/TLS with very large SIP messages, and I was running into buffer size limitations which caused crashes or errors.

My changes allow a new CPPFLAG to increase the size of a few important buffers in the code.

Example usage:

./build.sh --with-openssl CPPFLAGS=-DBUFFER_SIZE_MULTIPLIER=16

Thanks for the consideration.

[wdoekes]

Hi! Thanks for the PR. But I don't like this fix.

What kind of SIP messages do you have that could possibly exceed 64K?

Do you have examples or (even better) a test case that triggers these crashes?

[codyherzog-zz]

Hello. Thanks for responding.

In our use case, the very large messages are NOTIFY message which contain big RLS contact lists in XML format. Here are some related RFC resources:

https://tools.ietf.org/html/rfc4662
https://tools.ietf.org/html/rfc4826

Some of our users have massive contact lists, and our SIP server will generate big NOTIFY messages and send them as a single SIP message over TLS.

Here's a redacted version of one such large NOTIFY message:

big-notify-01.txt

I will look into adding a unit test which triggers the problems I mentioned.

Thanks again.

[codyherzog-zz]

Initially, I was thinking that it would be nice to convert all the relevant fixed-size arrays into dynamically sized std::vector objects, but I suspect that would require a lot more code changes.

[codyherzog-zz]

Hello.

Instead of a multiplier, an alternate idea would be to allow for the individual buffer sizes to be overridden independently with separate CPPFLAGS. Would that be more desirable?

Thanks.

[wdoekes]

Well. I think enlarging the SIPP_MAX_MSG_SIZE makes sense (if it can be done dynamically, it'd be even better, but might be troublesome).

Enlarging the screen_last_error doesn't make sense to me. Who needs a large blob of >32k in their screen output? This is to prevent a crash, am I right? In that case the appropriate length-checking functions strNcpy, sNprintf or whatever should be used to fix those.

As for MAX_CHAR_BUFFER_SIZE: that's only a problem for you in FileContents::FileContent (infile.cpp) right? That can be solved by using more std::stuff and fewer char[]s.

[codyherzog-zz]

Thanks for the feedback.

I agree with your suggestions. I will make some changes in the next week or so along those lines, then ask for another review.

Would you prefer to have SIPP_MAX_MSG_SIZE increased arbitrarily by some amount, like 4X, or would you prefer that it be configurable with a CPPFLAG?

If configurable, the code would look something like this:

#ifndef SIPP_MAX_MSG_SIZE
  #define SIPP_MAX_MSG_SIZE 65536
#endif

This would allow me to build as follows to meet my needs:

./build.sh --with-openssl CPPFLAGS=-DSIPP_MAX_MSG_SIZE=262144

Thanks again.

[codyherzog-zz]

Hello.

I've taken another pass at it.

I still have to do some more testing, but please let me know if this is moving in the right direction.

I stopped short of doing a big refactor of parsing code and went with a compromise approach.

[wdoekes]

This was what I was thinking indeed 👍

[wdoekes]

Please add spaces around / and +=.

(When you're touching lines, might as well fix the code style.)

And, you cannot add the return value of (v)snprintf() to c, because:

If the output was truncated due to this limit, then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available.

So that'll need fixing.

[codyherzog-zz]

Good catch on the return value. Turns out I also need to check for a negative return value. I've just pushed a new version of the fix. Thanks.

[codyherzog-zz]

As far as I can tell, the changes are working well. I was able to do a fair amount of testing.