Skip to content

sip: only treat a CRLF-only read as a keep-alive between messages - #357

Open
fridokus wants to merge 1 commit into
emiago:mainfrom
fridokus:oskar-fridell/keepalive-upstream
Open

fridokus wants to merge 1 commit into
emiago:mainfrom
fridokus:oskar-fridell/keepalive-upstream

Conversation

@fridokus

@fridokus fridokus commented Sep 7, 2026 •

Copy link
Copy Markdown

The bug

readConnection treats any read of ≤4 bytes containing only CR and LF as an RFC 5626 §3.5.1 keep-alive, wherever it lands. But a read is whatever TCP chose to deliver, so the LF ending a header line can arrive on its own — and is then swallowed, leaving the parser on a bare CR.

The next message fails to frame with line has no CRLF even though it is well formed, and the connection is closed.

Splitting a real 1568-byte REGISTER at any of its 22 CRLFs reproduces it every time. Before #352 the connection went deaf instead, seen in production as a proxy that stopped processing SIP entirely, sockets draining normally and CPU idle.

The change

A keep-alive only counts between messages. ParseSIPStream already knows when it is part way through one — it returns ErrParseSipPartial until it has drained its buffer — so parseStream passes that error up instead of folding it into nil, and the read loop keeps the last parse result and checks it.

Guarding on "the parser's buffer is empty" instead looks equivalent and is not: body bytes are copied out as they arrive, so a message waiting for the last two bytes of its body has an empty buffer.

@emiago

emiago commented Sep 13, 2026

Copy link
Copy Markdown
Owner

I could see point to have this merged, but fix could be simpler. Instead me spending tokens,
let your AI have 1 regression test, return sip partial error up to stack and handle check in loop instead converting to bool.

@fridokus
fridokus force-pushed the oskar-fridell/keepalive-upstream branch from 07cbac1 to 106217a Compare September 14, 2026 04:08
@fridokus

Copy link
Copy Markdown
Author

Done, all three.

  • `parseStream` keeps its `error` signature and returns `ErrParseSipPartial` up instead of folding it into `nil`. Net effect is that it is now simpler than before the PR.
  • The read loop keeps the last parse result and checks it — no bool.
  • One regression test, down from four.

The test splits the message at each of its CRLFs so the LF lands in a read of its own. Without the guard it delivers 0 messages instead of 2, at the first boundary. Full suite passes.

RFC 5626 section 3.5.1 puts a keep-alive between SIP messages, but
readConnection treated any read of four bytes or fewer holding nothing but
CR and LF as one, wherever it landed.

A read is whatever TCP chose to deliver, so the LF ending a header line
can arrive on its own. It was then swallowed and the parser left on a bare
CR: the next message failed to frame with "line has no CRLF" although
nothing was wrong with it, and the connection was closed. Splitting a real
1568-byte REGISTER at any of its 22 CRLFs reproduces it every time. Before
emiago#352 the connection went deaf instead, seen in production as a proxy that
stopped processing SIP while its CPU sat idle.

parseStream now returns ErrParseSipPartial up instead of folding it into
nil, and the read loop keeps the last parse result: while it is that error
the parser is part way through a message, so a CRLF-only read is a
continuation rather than a keep-alive.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@fridokus
fridokus force-pushed the oskar-fridell/keepalive-upstream branch from 106217a to fb17ef5 Compare October 5, 2026 12:46
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.

2 participants