-
Notifications
You must be signed in to change notification settings - Fork 26
Fix RTSP server crash on TCP transport connect #602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Copilot
wants to merge
7
commits into
main
Choose a base branch
from
copilot/fix-rtsp-stream-crash-tcp
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
123cf61
Initial plan
Copilot cf1b1c1
Fix RTSP server crash on TCP transport connect
Copilot e9218a5
Fix RTSP server crash on TCP transport connect
Copilot 21cad23
Remove codeql artifact
Copilot c8ebb26
Merge branch 'main' into copilot/fix-rtsp-stream-crash-tcp
finger563 a86ad63
ensure all false return paths from `parse_rtsp_setup_request` respond…
finger563 0b59735
update to use existing function modified instead of new function
finger563 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,7 +152,9 @@ bool RtspSession::handle_rtsp_setup(std::string_view request) { | |
| int client_rtp_port; | ||
| int client_rtcp_port; | ||
| if (!parse_rtsp_setup_request(request, rtsp_path, client_rtp_port, client_rtcp_port)) { | ||
| // the parse function will send the response, so we just need to return | ||
| // the parse function will send the response, so we just need to | ||
| // teardown the session since setup failed and streaming cannot proceed | ||
finger563 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| teardown(); | ||
| return false; | ||
| } | ||
| // parse the sequence number from the request | ||
|
|
@@ -215,11 +217,10 @@ bool RtspSession::handle_rtsp_teardown(std::string_view request) { | |
| return send_response(code, message, sequence_number, headers); | ||
| } | ||
|
|
||
| bool RtspSession::handle_rtsp_invalid_request(std::string_view request) { | ||
| bool RtspSession::handle_rtsp_invalid_request(std::string_view request, int code, | ||
| std::string_view message) { | ||
| logger_.info("RTSP invalid request"); | ||
| // create a response | ||
| int code = 400; | ||
| std::string message = "Bad Request"; | ||
| int sequence_number = 0; | ||
| if (!parse_rtsp_command_sequence(request, sequence_number)) { | ||
| return send_response(code, message); | ||
|
|
@@ -357,29 +358,36 @@ bool RtspSession::parse_rtsp_setup_request(std::string_view request, std::string | |
| // parse the rtsp path from the request | ||
| rtsp_path = parse_rtsp_path(request); | ||
| if (rtsp_path.empty()) { | ||
| logger_.error("Failed to parse RTSP path from request"); | ||
| handle_rtsp_invalid_request(request); | ||
| return false; | ||
| } | ||
| logger_.debug("Parsing setup request:\n{}", request); | ||
| // parse the transport header from the request | ||
| auto transport_index = request.find("Transport: "); | ||
| if (transport_index == std::string::npos) { | ||
| logger_.error("Failed to parse Transport header (start) from request"); | ||
| handle_rtsp_invalid_request(request); | ||
| return false; | ||
| } | ||
| auto transport_end_index = request.find('\r', transport_index); | ||
| if (transport_end_index == std::string::npos) { | ||
| logger_.error("Failed to parse Transport header (end) from request"); | ||
| handle_rtsp_invalid_request(request); | ||
| return false; | ||
| } | ||
| std::string_view transport = | ||
| request.substr(transport_index + 11, transport_end_index - transport_index - 11); | ||
| if (transport.empty()) { | ||
| logger_.error("Transport header is empty"); | ||
| handle_rtsp_invalid_request(request); | ||
| return false; | ||
| } | ||
| logger_.debug("Transport header: {}", transport); | ||
| // we don't support TCP, so return an error if the transport is not RTP/AVP/UDP | ||
| if (transport.find("RTP/AVP/TCP") != std::string::npos) { | ||
| logger_.error("TCP transport is not supported"); | ||
| // TODO: this doesn't send the sequence number back to the client | ||
| send_response(461, "Unsupported Transport"); | ||
| handle_rtsp_invalid_request(request, 461, "Unsupported Transport"); | ||
|
||
| return false; | ||
| } | ||
|
|
||
|
|
@@ -389,12 +397,16 @@ bool RtspSession::parse_rtsp_setup_request(std::string_view request, std::string | |
| std::string_view rtp_port = | ||
| request.substr(client_port_index + 12, dash_index - client_port_index - 12); | ||
| if (rtp_port.empty()) { | ||
| logger_.error("Failed to parse client RTP port from request"); | ||
| handle_rtsp_invalid_request(request); | ||
| return false; | ||
| } | ||
| // parse the rtcp port from the request | ||
| std::string_view rtcp_port = | ||
| request.substr(dash_index + 1, request.find('\r', client_port_index) - dash_index - 1); | ||
| if (rtcp_port.empty()) { | ||
| logger_.error("Empty client RTCP port in request"); | ||
| handle_rtsp_invalid_request(request); | ||
| return false; | ||
| } | ||
| // convert the rtp and rtcp ports to integers | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function documentation should be updated to explain that this method now handles both generic invalid requests and specific protocol errors, and clarify when custom codes/messages should be used versus defaults.