All SIP messages of a TCP stream are ignored if that TCP stream contains a partial SIP message

[ayonpals]

If I pump SIP register messages with tools like SIPp, it is seen that Register handler is called less number of times than the actual SIP messages pumped. On further debugging it is seen that if a TCP stream contains a partial SIP message, all the other SIP messages in that TCP stream is ignored. Thats why the register handler is called less number of times.

The following changes solved the issue

diff --git a/sip/parser_stream.go b/sip/parser_stream.go
index 5e4545a..a5f36cc 100644
--- a/sip/parser_stream.go
+++ b/sip/parser_stream.go
@@ -166,7 +169,7 @@ func (p *ParserStream) ParseSIPStream(data []byte) (msgs []Message, err error) {
        case ErrParseLineNoCRLF, ErrParseReadBodyIncomplete:
            reader.Reset()
            reader.Write(unparsed)
-           return nil, ErrParseSipPartial
+           return msgs, ErrParseSipPartial
        }

        if err != nil {
 }

diff --git a/sip/transport_tcp.go b/sip/transport_tcp.go
index ed8660f..a765893 100644
--- a/sip/transport_tcp.go
+++ b/sip/transport_tcp.go
@@ -183,20 +183,20 @@ func (t *transportTCP) readConnection(conn *TCPConnection, laddr string, raddr s

 func (t *transportTCP) parseStream(par *ParserStream, data []byte, src string, handler MessageHandler) {
    msgs, err := par.ParseSIPStream(data)
-   if err == ErrParseSipPartial {
-       return
-   }

-   for _, msg := range msgs {
-       if err != nil {
-           t.log.Error().Err(err).Str("data", string(data)).Msg("failed to parse")
-           return
-       }
+    if err != nil && err != ErrParseSipPartial {
+        t.log.Error().Err(err).Str("data", string(data)).Msg("failed to parse")
+        return
+    }

+   for _, msg := range msgs {
        msg.SetTransport(t.Network())
        msg.SetSource(src)
        handler(msg)
    }
 }

[emiago]

ah ok I see. I think it was due to large transfer buffer size as well. Therefore more messages can fit. Thanks for reporting, I will consider your diff here is shared for patching.

[emiago]

It needs probably test for all to be covered

[ayonpals]

Looking into the code it looks like if parsing of any SIP message of a TCP stream fails, then also all the other SIP messages are ignored. Is it possible to enhance the code to send "400 Bad Request" response to client only for the bad request and process the other messages normally?

[emiago]

Hi ayonpals, thanks providing more context. Yes we need to deal this differentely.

[ayonpals]

Hi @emiago , thanks for your response. Is there any possibility to have this fix in some near releases?

[emiago]

Hi @ayonpals issue should be fixed before 1.0, that is all I can say. Anyway priorities can be changed ;)

[ayonpals]

We are evaluating sipgo for one of our project. @emiago any tentative date for 1.0 will be helpful

[emiago]

Hi @ayonpals. I have not yet set date for 1.0 as my focus now is more on diago project and media. Sipgo is already tested from many different projects/tools so it has enough features, but there are some couple things open that modern stack requires, and probably I see more optimizations opportunities. So even before 1.0 you are already working with stack that has no need to change too much. Most of things I think now makes more sense to add on diago project.

Of course I see this issue important so I will try to prioritize it in following period. Have you tried decreasing buffer size? Please DM me on mail as well if you want to share more about your project, to understand better your situation.

[emiago]

Hi @ayonpals . I took time to read this again. So actually parsing in single buffer multiple messages has no issue, but of course if you are receiving corrupted all are gone. Now considering this is TCP, receiving this corrupted(partial) messages should not be normally the case or client is actually fishy or maybe even insecure.

So still to handle or reduce this problem I have now exported TransportBufferReadSize . Reducing this size should disable parser buffering multiple msgs and therefore remove/minimize behavior of messages discarded.

With above said, I would leave this still open if you think otherwise. So please comment when you find time, otherwise issue may be closed as expected behavior. Also if you are loadtesting, sharing results would be great for community and this library.

[ayonpals]

Thanks @emiago. We will change the value of TransportBufferReadSize and let you know the result. In our usecase, the Client does not directly connect to our SIP application. We have a proxy in between. Also the SIP messages length is around 5000-6000 bytes but can vary. If millions of clients request to our application, there will be high rate of SIP message exchanges between the proxy and our application. This will cause a single TCP segment from proxy to our application to have a variety of SIP messages like REGISER, INVITE, OPTIONS etc of different clients. This increases the chances of occurrence of the issue and will impact several clients if a message is discarded or an erroneous SIP message is present.

[emiago]

Hi @ayonpals Thanks for explaining the use case. Yes I will keep this still open before 1.0

[emiago]

@ayonpals added now better handling of SIP TCP. Message per message parsed. Check latest code. If problem fixed, support sipgo further development ;)

[ayonpals]

Hi @emiago, We have run load tests and observed that the issue is no longer reproducible with the latest changes and also setting the TransportBufferReadSize to a value lower than 65535 (i.e 36635). You can confidently close this issue. We are eagerly looking forward to seeing this change in the next build of sipgo. Thank you for your support and contributions

[emiago]

closing.