Repository navigation
Conversation
Recorders send a namespace prefix and a username token even when authentication is disabled. Strip the prefix, ignore the token unless authentication is on, and answer the device and media calls used before the stream opens. Keep the RTSP session when CSeq starts at zero or the client sends only line feeds. The main picture is stream=0, which is also the default. stream=1 is a separate H.264 640x360 encoder with its own parameter sets.
PR Summary by QodoSupport recorder ONVIF requests and a dedicated RTSP substream
AI Description
Diagram
High-Level Assessment
Files changed (29)
|
Code Review by Qodo
1. Unauthenticated clients can change the clock
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+576
to
+578
| time_t stamp = onvif_timegm(year, month, day, hour, min, sec); | ||
| struct timeval tv = { .tv_sec = stamp, .tv_usec = 0 }; | ||
| if (stamp == (time_t)-1 || settimeofday(&tv, NULL) != 0) |
There was a problem hiding this comment.
1. Unauthenticated clients can change the clock 🐞 Bug ⛨ Security
SetSystemDateAndTime now calls settimeofday() under the same authentication condition as read-only ONVIF requests. When ONVIF is enabled with its default enable_auth: false, any client that can reach the service can submit a timestamp and change the camera clock.
Agent Prompt
## Issue description
The new clock-setting endpoint permits unauthenticated clock changes under the default ONVIF authentication setting.
## Fix Focus Areas
- src/server.c[628-638]
- src/server.c[653-656]
- src/onvif.c[575-586]
## Recommended Fix
Require authorization specifically for clock-changing requests, or decline the mutation when authentication is disabled.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+970
to
+971
| if (EQUALS(action, "GetRelayOutputs")) return onvif_static(response, respLen, onvif_relay); | ||
| if (EQUALS(action, "SetNetworkInterfaces")) return onvif_static(response, respLen, onvif_setnetif); |
There was a problem hiding this comment.
2. Network changes falsely report success 🐞 Bug ≡ Correctness
onvif_dispatch_extra() answers SetNetworkInterfaces with a fixed successful response without reading or applying the requested configuration. A client that submits a new address receives RebootNeeded=false, while the camera retains its previous interface settings.
Agent Prompt
## Issue description
`SetNetworkInterfaces` reports success without changing any network setting.
## Fix Focus Areas
- src/onvif.c[779-783]
- src/onvif.c[969-973]
## Recommended Fix
Implement and verify the requested interface update, or return an unsupported-operation SOAP fault rather than a success response.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+995
to
+997
| if (EQUALS(action, "GetImagingSettings")) return onvif_static(response, respLen, onvif_image); | ||
| if (EQUALS(action, "GetOptions")) return onvif_static(response, respLen, onvif_imageopt); | ||
| if (EQUALS(action, "SetImagingSettings")) return onvif_static(response, respLen, onvif_imageset); |
There was a problem hiding this comment.
3. Imaging changes falsely report success 🐞 Bug ≡ Correctness
SetImagingSettings returns a static success document without parsing the request or updating camera controls. A subsequent GetImagingSettings still returns the hard-coded values of 50, so a client cannot tell from the setter response that its changes were discarded.
Agent Prompt
## Issue description
`SetImagingSettings` acknowledges updates that it never applies.
## Fix Focus Areas
- src/onvif.c[837-858]
- src/onvif.c[994-998]
## Recommended Fix
Apply supported settings and report failures, or return an unsupported-operation SOAP fault instead of success.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+714
to
+715
| media_main_ch = -1; | ||
| media_sub_ch = -1; |
There was a problem hiding this comment.
4. Recorders receive stale substream settings 🐞 Bug ≡ Correctness
media_mp4_disable() clears the new stream channel IDs but does not clear the RTSP parameter-set caches for either slot. After the settings endpoint recreates an encoder with different parameters, __retrieve_sprop() retains its old SPS and PPS, so a later DESCRIBE can advertise parameter sets from the previous stream.
Agent Prompt
## Issue description
RTSP parameter sets survive main and sub encoder replacement, leaving later SDP descriptions stale.
## Fix Focus Areas
- src/media.c[711-731]
- src/rtsp/rtp.c[364-428]
- src/rtsp/rtsp.c[913-918]
## Recommended Fix
Synchronize encoder replacement with RTSP access and invalidate the affected slot's cached parameter sets before advertising frames from the replacement encoder.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+578
to
+580
| if (stamp == (time_t)-1 || settimeofday(&tv, NULL) != 0) | ||
| HAL_WARNING("onvif", "Could not set the clock to %04d-%02d-%02d %02d:%02d:%02d\n", | ||
| year, month, day, hour, min, sec); |
There was a problem hiding this comment.
5. Clock-setting failures appear successful 🐞 Bug ≡ Correctness
onvif_respond_settime() only logs a failed settimeofday() call and then unconditionally constructs SetSystemDateAndTimeResponse. If the process lacks permission to set time, or the supplied date is invalid, the recorder receives success even though the camera clock was not updated.
Agent Prompt
## Issue description
The clock-setting handler acknowledges failed or invalid updates as successful.
## Fix Focus Areas
- src/onvif.c[562-586]
- src/server.c[653-656]
## Recommended Fix
Validate the timestamp fully and send an appropriate SOAP fault when validation or `settimeofday()` fails; emit success only after the update succeeds.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+984
to
+986
| if (EQUALS(action, "GetAudioEncoderConfigurations") || | ||
| EQUALS(action, "GetAudioEncoderConfiguration")) | ||
| return onvif_static(response, respLen, onvif_audioenc); |
There was a problem hiding this comment.
6. A single audio query gets the wrong reply 🐞 Bug ≡ Correctness
onvif_dispatch_extra() routes both audio-encoder configuration actions to the plural GetAudioEncoderConfigurationsResponse document. A client requesting the singular GetAudioEncoderConfiguration action therefore receives a response for a different operation instead of a matching result or fault.
Agent Prompt
## Issue description
The singular audio-encoder query is answered using the plural operation's response element.
## Fix Focus Areas
- src/onvif.c[814-817]
- src/onvif.c[984-986]
## Recommended Fix
Return an operation-matching singular response or a SOAP fault for the singular query; keep the plural response for the plural query.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+456
to
+457
| if (strstr(buf, "trackID=")) | ||
| STR_KEY_NUM(buf, "trackID=", con->track_id); |
There was a problem hiding this comment.
7. Crafted rtsp request writes past track table 🐞 Bug ⛨ Security
The request-line parsers store client-supplied trackID= and track= values in con->track_id without checking them against the two-element trans array. Before authentication, a client can send SETUP rtsp://cam/trackID=2, causing both __method_setup and Transport-header parsing to index past that array.
Agent Prompt
## Issue description
An RTSP client can set `con->track_id` through `trackID=` or `track=`, and an unchecked value is used to index the two-element transport array during SETUP and Transport-header parsing.
## Fix Focus Areas
- src/rtsp/rtsp.c[282-309]
- src/rtsp/rtsp.c[437-458]
## Recommended Fix
Validate `con->track_id` against the size of `trans` immediately after both request-line `STR_KEY_NUM` calls, before any transport access. Reject invalid SETUP requests with 400 Bad Request, or reset an out-of-range value to 0.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+688
to
+692
| int which = (soap && strstr(soap, "stream=1")) ? 2 : 1; | ||
| if (!response || !respLen) return; | ||
| onvif_fill_profiles(profile, which); | ||
| if (!profile[0]) | ||
| onvif_fill_profiles(profile, 1); |
There was a problem hiding this comment.
8. Mjpeg-only camera returns an empty profile 🐞 Bug ≡ Correctness
onvif_respond_profile sets which = 1 unless the SOAP body contains stream=1, but the MJPEG branch of onvif_fill_profiles only adds a profile when which != 1. The fallback onvif_fill_profiles(profile, 1) repeats the same empty case, so with mp4 disabled and mjpeg enabled, any GetProfile request without stream=1 gets a GetProfileResponse with no profile in it.
Agent Prompt
## Issue description
The GetProfile fallback calls `onvif_fill_profiles(profile, 1)`, the same filter that just returned nothing, so MJPEG-only configs return an empty profile.
## Fix Focus Areas
- src/onvif.c[688-692]
## Recommended Fix
Change the fallback to `onvif_fill_profiles(profile, 0)`, which returns all profiles. The Profiles→Profile rename then applies to the first profile only.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The H.264 constant-bitrate block does not put the frame rate where the driver reads it, and the bitrate target is ignored, so the picture stays at the worst quantizer. Apply the requested bitrate as a fixed quantizer: 38 at 1024 kbps, 36 at 2048, 34 at 4096, 32 at 8192, never below 30. H.265 still uses constant bitrate, with the frame rate in the vendor layout. MJPEG was treating the quality factor as a quantizer and emitting one frame every N seconds. Quality 1 is now quantizer 98 and quality 99 is quantizer 0, at the configured frame rate. GC4653 starts from the 1920x1080 tables, 0x203 at 25 fps and 0x204 at 30. That matches the shipped IQ file. The 4MP tables ask for an ISP buffer that does not fit beside the encoder in the 35 MB zone, so a larger request stays at 1080p. The ISP helper is called with the sensor operations after the ISP is up. Automatic white balance on this sensor still locks about 2.4x gain on red and blue, so gc4653_mipi holds unity gains. A reported NAL that runs past the userspace mapping faults in memcpy and the process exits with the watchdog still armed. Copy a NAL only when its bytes, plus the extra bytes memcpy reads, sit inside one mapping. A second encoder that does not match an open VPSS channel opens its own channel instead of scaling the main picture.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Recorders send ONVIF actions with a namespace prefix and a username token even when authentication is turned off. The camera treated the prefix as part of the action name and did not answer, so the recorder reported a bad username or password and never opened RTSP.
This accepts those calls, answers the device and media requests a recorder uses before it plays, and keeps the RTSP session when CSeq starts at zero. The main picture is
stream=0, which is also used when the parameter is absent.stream=1is a separate H.264 640×360 encoder with its own parameter sets. When that encoder does not match an open scaler channel, it opens one instead of stretching the main picture.On FH8852 the H.264 constant-bitrate block does not place the frame rate where the driver reads it, and the bitrate target is ignored, so the picture stays at the worst quantizer. The requested bitrate is applied as a fixed quantizer: 38 at 1024 kbps, 36 at 2048, 34 at 4096, 32 at 8192, and never below 30. H.265 still uses the driver's constant-bitrate control, with the frame rate in the field the vendor layout uses. MJPEG was using the quality factor as a quantizer and emitting one frame every N seconds. Quality 1 is now quantizer 98 and quality 99 is quantizer 0, at the configured frame rate.
GC4653 is started from the 1920×1080 tables in the sensor library, format 0x203 at 25 fps and 0x204 at 30 fps. That is the resolution of the shipped tuning file. The 4 megapixel tables request an ISP buffer of about 21 MB once the full picture is enabled, and that allocation does not fit in the 35 MB zone on this board, so a request for 2560×1440 stays at 1080p.
The ISP helper was previously called with no sensor table, and before the ISP was initialized, so it never attached to GC4653. It is now called with the sensor operations after
API_ISP_Init. Automatic white balance on this sensor still settles near 2.4× gain on red and blue, which is a magenta cast. Forgc4653_mipiautomatic white balance is disabled and the gains are held at 256.The encoder can return an H.264 unit whose length runs past the mapping it handed to userspace. Copying that length faults inside memcpy, the process exits without stopping the watchdog, and the camera resets. A unit is copied only when its bytes, and the extra bytes memcpy reads beyond the length, lie inside a single mapping. Anything else is left out of the stream.