Skip to content
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

RTSP: Trim whitespace from CSeq header before parsing #77

Closed

Conversation

nemosupremo
Copy link
Contributor

Working with Longse (also may share the same software/firmware with Herospeed) cameras we found that Retina was unable to communicate with these cameras. Upon analyzing the packets, we saw that these cameras were adding a space after the Cseq header value (So instead of Cseq: 101\r\n, we saw Cseq: 101 \r\n).

This PR simply adds a trim method before the Cseq is parsed so that these cameras can be supported successfully.

longse.pkt.zip

Copy link
Owner

@scottlamb scottlamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the report and packet capture!

I wonder if we should handle this in the rtsp-types layer for all headers. RTSP/1.0 RFC 2636 section 4.1 refers to HTTP/1.1 RFC 2068 section 4.1 which is not very clear about if trailing whitespace should be stripped.

          message-header = field-name ":" [ field-value ] CRLF

          field-name     = token
          field-value    = *( field-content | LWS )

          field-content  = <the OCTETs making up the field-value
                           and consisting of either *TEXT or combinations
                           of token, tspecials, and quoted-string>

but the HTTP/1.1 RFCs have since been updated/clarified. RFC 9110 explicitly says the following:

A field value does not include leading or trailing whitespace. When a specific version of HTTP allows such whitespace to appear in a message, a field parsing implementation MUST exclude such whitespace prior to evaluating the field value.

Would you like to file a rtsp-types issue about this? If not, I can.

Regardless of whether we fix this in retina or rtsp-types, I'd like to add a unit test of this scenario also.

@nemosupremo
Copy link
Contributor Author

I see, the RFC is pretty explicit so it seems the issue lies with sdroege/rtsp-types; for completeness (there might be other headers) it makes sense to fix the issue there.

@nemosupremo
Copy link
Contributor Author

Closing this PR as the solution has been implemented in sdroege/rtsp-types 0.0.5

@nemosupremo nemosupremo closed this Feb 2, 2023
@scottlamb
Copy link
Owner

I'll prep a new Retina release. For whatever reason rtsp-types releases have been 0.0.z, and according to cargo's rules:

"0.0.z" releases are always major changes.

so retina's Cargo.toml has to change to pick it up.

scottlamb added a commit that referenced this pull request Feb 2, 2023
scottlamb added a commit that referenced this pull request Feb 2, 2023
@scottlamb
Copy link
Owner

0.4.5 released.

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