emiago / sipgo

SIP library for writing fast SIP services in GO
BSD 2-Clause "Simplified" License
545 stars 72 forks source link

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

Open ayonpals opened 1 week ago

ayonpals commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

It needs probably test for all to be covered

ayonpals commented 1 week ago

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 commented 1 week ago

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