rtsp: fix connection teardown races and keep serving past the connection pool

[Jamp]

Found while chasing a one-off SIGSEGV and an RTP stream that go2rtc rejected as malformed (PT=60: bad cseq) right after clients came and went:

• A request cut short (fgets returning NULL mid-request) detached the connection, which could reset it on the spot, and the handler then went on to answer through a NULL FILE * (SIGSEGV); an unknown method detached it twice.
• The client fd was closed three times (two fdopens on the same fd plus close), sometimes from the video thread, so a new client that got the same number could lose its socket or share it.
• Responses went through vfprintf on a non-blocking socket; with the send buffer full musl drops the rest and RTP frames follow a truncated response. They are now formatted into a buffer and sent whole, under the connection's write mutex.
• A split $ interleaved packet left bytes behind that were then parsed as a request.
• Transports and SSRC were not cleared between clients, and interleaved tracks sent an RTCP SR with every frame.
• Connection 17 (the pool holds 16) stopped the RTSP thread. Extra clients are now accepted and closed with a rate-limited warning, and EMFILE/ENFILE, EINTR and friends no longer end the thread.
• GET_PARAMETER, used as a keepalive, is answered instead of dropping the client.

Each fix is its own commit, and each comes with a host test (test/rtsp_con_test.c, built with make -C test) that failed before it. The test/ harness is new and plain C; happy to drop it from the PR if you would rather not carry it.

On the camera (SSC323, H.264 1080p20, go2rtc connected throughout): 988 half-sent TEARDOWNs in two minutes with a TCP viewer attached, same PID, fd count stable, 20 fps; 20 simultaneous connections, 5 refused, the server keeps serving and a new ffprobe gets the stream.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Prevent RTSP teardown races and keep serving when connections are full

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Prevent premature connection resets, duplicate socket closes, and truncated responses during
 client teardown.
• Handle split interleaved packets, reset reused transport state, and answer GET_PARAMETER
 keepalives.
• Refuse excess clients without stopping the server; add host regression tests.

Diagram

graph TD
  A["RTSP listener"] --> B["Select loop"] --> C["Accept client"] --> D{"Free slot?"} -->|Yes| E["Request parser"] --> F["Shared send"]
  D -->|No| G["Close client"]
  E -->|Disconnect| H["Connection reset"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Incremental non-blocking input parser

• ➕ Avoids waiting in the server loop for the remainder of a split interleaved packet.
• ➖ Requires persistent input buffering and a broader rewrite of request and frame parsing.

Recommendation: The targeted fixes fit the existing server and have focused regression coverage. Consider an incremental parser if split packets frequently stall other clients: the new bounded wait can hold the single RTSP loop while bytes arrive.

Files changed (6) +610 / -76

Bug fix (3) +181 / -76

rtp.cReuse complete-send helper for interleaved RTP +2/-15

Reuse complete-send helper for interleaved RTP

• Replaces the RTP-specific partial-write loop with the shared non-blocking send helper. The flush still clears its staged buffer after the send attempt.

src/rtsp/rtp.c

rtsp.cHarden RTSP parsing, output, teardown, and admission +155/-59

Harden RTSP parsing, output, teardown, and admission

• Sends formatted responses completely under the write mutex, detaches cut-short requests once, and closes the client socket through a single owning stream. Handles split interleaved packets, GET_PARAMETER keepalives, and transport reset; excess clients and recoverable socket errors no longer stop the server.

src/rtsp/rtsp.c

rtsp.hDefine shared send and single-stream connection ownership +24/-2

Define shared send and single-stream connection ownership

• Adds GET_PARAMETER to method state and a retrying non-blocking send helper. Removes the separate write stream and leaves detachment of incomplete requests to the caller.

src/rtsp/rtsp.h

Tests (3) +429 / -0

MakefileBuild and run the host RTSP regression suite +15/-0

Build and run the host RTSP regression suite

• Adds a plain-C test target with host compatibility flags. The test includes rtsp.c directly to exercise its static functions.

test/Makefile

check.hAdd lightweight host-test assertions +25/-0

Add lightweight host-test assertions

• Provides failure-counting checks and a final pass/fail result for the new C test executable.

test/check.h

rtsp_con_test.cCover RTSP connection and protocol regressions +389/-0

Cover RTSP connection and protocol regressions

• Exercises truncated requests, full send buffers, single socket closure, transport reuse, split interleaved packets, pool and descriptor exhaustion, and GET_PARAMETER responses. Uses socket pairs and loopback clients to drive the server code on a host.

test/rtsp_con_test.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. One client that stops reading freezes all streams ✓ Resolved

Description

__rtsp_write calls __tcp_send_all, which retries EAGAIN indefinitely with 100 ms waits but no
deadline or retry cap, ignoring poll timeouts unless a hard send error occurs. If a client stops
reading during a response or a link drops without FIN or RST, rtspThrFxn remains in that loop
holding rtsp_lock, blocking accepts, other replies, and the video thread’s per-frame
__rtp_setup_transfer for every viewer.

Code

src/rtsp/rtsp.c[R152-154]

+    pthread_mutex_lock(&p->write_mutex);
+    ret = __tcp_send_all(p->client_fd, (unsigned char *)msg, len);
  pthread_mutex_unlock(&p->write_mutex);

Evidence

__tcp_send_all loops on EAGAIN through __wait_out without exiting when the wait times out.
__rtsp_write invokes it from __message_proc_sock under a write mutex, while rtspThrFxn calls
the response handler holding rtsp_lock; the RTP functions in rtp.c also take rtsp_lock for every
frame, so they remain blocked until the send loop exits.

src/rtsp/rtsp.h[217-228]
src/rtsp/rtsp.c[917-932]
src/rtsp/rtp.c[504-506]
src/rtsp/rtsp.h[211-226]
src/rtsp/rtsp.c[141-155]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`__rtsp_write` sends responses through `__tcp_send_all`, which can retry EAGAIN indefinitely when a client does not drain its socket. Because the RTSP thread holds `rtsp_lock` during the response, this blocks other clients and the video thread’s per-frame work.
## Fix Focus Areas
- src/rtsp/rtsp.h[211-228]
- src/rtsp/rtsp.c[141-159]
## Recommended Fix
Give `__tcp_send_all`, or a response-only variant, a finite deadline or retry limit of roughly 1–2 seconds. Check poll results and return FAILURE if the socket remains unwritable; when `__rtsp_write` receives FAILURE, set `con_state = __CON_S_DISCONNECTED` so the connection is detached and swept.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Parameter bodies disrupt later requests ✓ Resolved

Description

__method_get_parameter() answers immediately, while the request parser stops at the blank header
line without consuming a declared body. A client that sends GET_PARAMETER with a body leaves those
bytes to be read as the start of its next RTSP request, which can cause the connection to be
disconnected.

Code

src/rtsp/rtsp.c[R325-329]

+static void __method_get_parameter(struct connection_item_t *p, rtsp_handle h)
+{
+    __rtsp_write(p, "RTSP/1.0 200 OK\r\n"
+            "CSeq: %d\r\n"
+            "\r\n", p->cseq);

Evidence

The parser reads header lines only, the new handler sends its reply without reading a body, and the
next invocation interprets the remaining bytes as a fresh request.

src/rtsp/rtsp.c[454-475]
src/rtsp/rtsp.h[185-208]
src/rtsp/rtsp.c[324-330]
src/rtsp/rtsp.c[531-535]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The newly supported GET_PARAMETER method leaves a request body unread, so its bytes can be parsed as a subsequent request.
## Fix Focus Areas
- src/rtsp/rtsp.c[454-531]
- src/rtsp/rtsp.h[185-208]
## Recommended Fix
Parse and validate Content-Length, then consume exactly that many body bytes before dispatching the request. Add a test with a body-bearing GET_PARAMETER followed by another request on the same connection.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Server shutdown leaves a descriptor open 🐞 Bug ☼ Reliability

Description

__accept_proc_sock() stores its /dev/null spare descriptor in a function-static variable with no
shutdown cleanup. After the server has accepted a client and rtsp_finish() stops it, that
descriptor remains open for the rest of the process.

Code

src/rtsp/rtsp.c[R809-810]

+    if (spare_fd < 0)
+        spare_fd = open("/dev/null", O_RDONLY | O_CLOEXEC);

Evidence

The accept path opens a static descriptor, while the thread exit closes only the listening socket
and the server cleanup does not close the spare descriptor.

src/rtsp/rtsp.c[799-826]
src/rtsp/rtsp.c[933-965]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new spare descriptor remains open after the RTSP server shuts down.
## Fix Focus Areas
- src/rtsp/rtsp.c[799-826]
- src/rtsp/rtsp.c[933-965]
## Recommended Fix
Make the spare descriptor part of the server or RTSP-thread state and close it when that thread exits, including error exits.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. A trickled client packet stalls video for all ✓ Resolved

Description

__read_interleaved sleeps up to 10×20 ms in poll, and its retry budget restarts on every call.
The header and each 4096-byte payload chunk are separate calls, so one $ packet sent slowly can
hold rtsp_lock for several seconds, blocking accepts, other clients and the per-frame RTP path.

Code

src/rtsp/rtsp.c[R394-400]

+    while ((got += fread((char *)dst + got, 1, len - got, con->fp_tcp_read)) < len) {
+        struct pollfd pfd = { .fd = con->client_fd, .events = POLLIN };
+        if (!ferror(con->fp_tcp_read) || errno != EAGAIN || ++waits > 10)
+            return FAILURE;
+        clearerr(con->fp_tcp_read);
+        poll(&pfd, 1, 20);
+    }

Evidence

waits is local to each __read_interleaved call, and the caller at line 429 calls it once per
chunk of up to 4096 bytes, for payloads of up to 65535 bytes. All of this runs inside
__message_proc_sock under rtsp_lock.

src/rtsp/rtsp.c[389-431]
src/rtsp/rtsp.c[917-928]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Skipping a split interleaved packet blocks the RTSP thread in `poll`, with a fresh wait budget for each chunk, while it holds `rtsp_lock`.
## Fix Focus Areas
- src/rtsp/rtsp.c[389-403]
- src/rtsp/rtsp.c[426-431]
## Recommended Fix
Store the number of bytes still to skip in `connection_item_t` and finish skipping on later select wakeups instead of sleeping. If waiting is kept, use a single total budget per packet rather than one per call.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Jamp]

One more fix, found on the camera after a day of streaming: __transfer_nal_h26x read the first byte of the NAL before checking its size, and on SSC323 the encoder now and then hands out a pack with no data, so the video thread read address 0 (twice in about three hours; a core dump put it at rtp.c:70). The size check now comes first. test/rtp_nal_test.c crashes without it and passes with it.