Conversation
Three fixes: 1. Fix race condition on sessions_ map - accept_task_function now locks session_mutex_ when modifying sessions_, matching the locking in session_task_function that iterates/erases sessions. 2. Teardown session after failed SETUP - when TCP transport is rejected (or any SETUP parse failure), the session is properly torn down so it gets cleaned up instead of remaining as a zombie. 3. Include CSeq in 461 response - the "Unsupported Transport" response now includes the sequence number per RTSP protocol. Co-authored-by: finger563 <213467+finger563@users.noreply.github.com>
Co-authored-by: finger563 <213467+finger563@users.noreply.github.com>
Co-authored-by: finger563 <213467+finger563@users.noreply.github.com>
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical crash in the RTSP server that occurs when clients request TCP transport (e.g., Home Assistant connections). The server was becoming permanently unresponsive due to a race condition and zombie sessions.
Changes:
- Fixed data race on
sessions_map by adding mutex protection during session insertion - Added session teardown on failed SETUP requests to prevent zombie sessions
- Fixed RTSP protocol violation by including CSeq header in 461 "Unsupported Transport" responses
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| components/rtsp/src/rtsp_server.cpp | Added mutex lock guard around sessions_.emplace() to fix race condition between accept_task and session_task threads |
| components/rtsp/src/rtsp_session.cpp | Added teardown() call on SETUP failure and included CSeq in TCP transport rejection response |
| // the parse function will send the response, so we just need to | ||
| // teardown the session since setup failed and streaming cannot proceed |
There was a problem hiding this comment.
The comment states "the parse function will send the response," but parse_rtsp_setup_request only sends a response for the TCP transport rejection case (line 385). Other parse failures (empty rtsp_path at line 361-362, missing/malformed Transport header at lines 367-377, or malformed client_port at lines 394-401) return false without sending any error response to the client, which violates RTSP protocol requirements. Consider sending an appropriate error response (e.g., 400 Bad Request with CSeq) for all parse failure cases before calling teardown(), or update the comment to clarify that only the TCP rejection sends a response.
| // the parse function will send the response, so we just need to | |
| // teardown the session since setup failed and streaming cannot proceed | |
| // the request could not be parsed; send an error response and teardown | |
| handle_rtsp_invalid_request(request); |
RTSP server becomes permanently unresponsive after a client connects requesting TCP interleaved transport (e.g., Home Assistant). No subsequent clients can connect until reboot.
Changes
Fix data race on
sessions_map —accept_task_functionwas mutatingsessions_without holdingsession_mutex_, whilesession_task_functioniterates/erases under that same mutex on a different thread. Added lock guard around theemplace.Teardown session on failed SETUP — After rejecting TCP transport with 461, the session was left alive (
closed_=false,session_active_=false) as a zombie. Now callsteardown()sosession_task_functioncleans it up on the next iteration.Include CSeq in 461 response — The "Unsupported Transport" response was missing the CSeq header, violating RTSP protocol.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.