Merged
Conversation
Contributor
Author
|
I'd like a chance to squash the changes in this PR before merging; I've included the separate commits since this is such a broad-ranging PR that touches so many files. Hopefully the separate commits will make it easier to identify and undo any undesirable changes. I haven't reviewed all of these changes with a fine-toothed comb just yet; I plan on doing that in the next day or two. I figured it would be valuable to get the PR in work though so Copilot can review and I can collect any feedback around the methodology or approach. My intent once this PR is complete is that there will be a project-standard, correct, and performant way of safely copying strings that can be used going forward. Along the way I also consolidated whitespace trimming functions, so hopefully those can also be used for any future needs. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes “safe” string utilities into utils/strings.c, replaces many strncpy usages with safe_strcpy, and standardizes several path buffers on MAX_PATH_LENGTH while also reshaping a few helpers (e.g., trimming) into the shared strings module.
Changes:
- Move
safe_strdup/safe_strcpy/safe_strcatintoutils/strings.*and expand string helper coverage (trim/copy-trim). - Replace many
strncpycall sites withsafe_strcpyand remove redundant manual termination/zeroing. - Increase/standardize path buffer sizing (more
MAX_PATH_LENGTH) and moverebuild_recordings.cintosrc/tools/with CMake wiring.
Reviewed changes
Copilot reviewed 129 out of 129 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Excludes tools from main build; wires rebuild_recordings from src/tools |
| examples/onvif_motion_recording_example.c | Updates example string copies and path buffer size |
| include/core/config.h | Expands mp4 storage path buffer to MAX_PATH_LENGTH |
| include/database/db_recordings.h | Expands recording file_path to MAX_PATH_LENGTH |
| include/storage/storage_manager.h | Expands recording info path to MAX_PATH_LENGTH |
| include/utils/memory.h | Removes string helpers from memory header |
| include/utils/strings.h | Adds safe string + trim helper API and inline trim helpers |
| include/web/api_handlers_timeline.h | Expands timeline segment file_path to MAX_PATH_LENGTH |
| include/web/http_server.h | Expands cert/key/pid paths to MAX_PATH_LENGTH |
| include/web/libuv_server.h | Expands deferred file path to MAX_PATH_LENGTH |
| rtsp_test.c | Fixes av_strerror argument typo (ret -> err) |
| src/core/daemon.c | Uses MAX_PATH_LENGTH PID path and safe_strcpy |
| src/core/logger.c | Uses safe_strcpy for thread context + filename/ident |
| src/core/logger_json.c | Uses safe_strcpy for JSON log filename |
| src/core/main.c | Adds strings.h, converts multiple copies to safe_strcpy |
| src/core/mqtt_client.c | Uses safe_strcpy for motion state fields |
| src/core/shutdown_coordinator.c | Uses safe_strcpy for component names |
| src/database/db_core.c | Uses safe_strcpy for DB path buffers and increases size to PATH_MAX |
| src/database/db_detections.c | Uses safe_strcpy for labels |
| src/database/db_events.c | Uses safe_strcpy for query prefix and result strings |
| src/database/db_migrations.c | Uses PATH_MAX buffers for migrations path discovery |
| src/database/db_motion_config.c | Uses safe_strcpy and expands path list buffer types |
| src/database/db_query_builder.c | Uses safe_strcpy for column names/defaults |
| src/database/db_recording_tags.c | Consolidates trimming via copy_trimmed_value + uses safe_strcpy |
| src/database/db_recordings_sync.c | Expands file path buffer and uses safe_strcpy |
| src/database/db_schema_cache.c | Uses safe_strcpy for cache keys |
| src/database/db_system_settings.c | Uses safe_strcpy for setting output |
| src/database/db_zones.c | Uses safe_strcpy for zone fields and includes strings helper |
| src/database/sqlite_migrate.c | Uses safe_strcpy for parsed filename parts and version output |
| src/storage/storage_manager.c | Expands storage path buffer to MAX_PATH_LENGTH and uses safe_strcpy |
| src/storage/storage_manager_streams.c | Uses safe_strcpy for stream names |
| src/telemetry/stream_metrics.c | Uses safe_strcpy for metric slot stream name |
| src/tools/rebuild_recordings.c | Moves tool to src/tools and converts copies to safe_strcpy |
| src/utils/memory.c | Removes safe_str* implementations from memory utilities |
| src/utils/strings.c | Adds safe_strdup/safe_strcpy/safe_strcat and trim/copy-trim helpers |
| src/video/api_detection.c | Uses safe_strcpy for detection labels/zone IDs and preview buffer init change |
| src/video/buffer_strategy_go2rtc.c | Uses safe_strcpy for ids/urls and includes strings helper |
| src/video/buffer_strategy_hls_segment.c | Uses safe_strcpy for segment path tracking and stream naming |
| src/video/buffer_strategy_memory_packet.c | Uses safe_strcpy for stream naming |
| src/video/buffer_strategy_mmap.c | Uses safe_strcpy for stream naming in mmap header/strategy |
| src/video/detection_model.c | Uses safe_strcpy for model type/path |
| src/video/detection_stream.c | Uses safe_strcpy for detection stream name |
| src/video/go2rtc/go2rtc_api.c | Uses safe_strcpy for previews and removes unnecessary zero-init for encoded URL buffers |
| src/video/go2rtc/go2rtc_consumer.c | Uses safe_strcpy for consumer state fields |
| src/video/go2rtc/go2rtc_integration.c | Uses safe_strcpy for tracked/original config fields |
| src/video/go2rtc/go2rtc_process.c | Uses safe_strcpy for path parsing/copying and log dir buffer changes |
| src/video/go2rtc/go2rtc_stream.c | Uses safe_strcpy for URL manipulation and buffer init adjustments |
| src/video/hls/hls_context.c | Uses safe_strcpy for stopping stream list management |
| src/video/hls/hls_directory.c | Uses safe_strcpy in directory creation logic |
| src/video/hls/hls_unified_thread.c | Uses safe_strcpy for thread-safe local copies |
| src/video/hls_streaming.c | Uses safe_strcpy for stream name copy in cleanup |
| src/video/hls_writer.c | Uses safe_strcpy for filenames/output dir/stream name |
| src/video/motion_detection.c | Uses safe_strcpy for labels and motion stream naming |
| src/video/mp4_recording_writer.c | Uses safe_strcpy for stream-name/path copies in registry lifecycle |
| src/video/mp4_writer_core.c | Uses safe_strcpy for writer initialization fields |
| src/video/mp4_writer_thread.c | Uses safe_strcpy for thread context and writer fields |
| src/video/mp4_writer_utils.c | Uses safe_strcpy when extracting directory path |
| src/video/onvif_detection.c | Uses safe_strcpy for substring extraction and subscription data |
| src/video/onvif_discovery.c | Uses safe_strcpy for selected network and device fields |
| src/video/onvif_discovery_network.c | Uses safe_strcpy for network parsing input copy |
| src/video/onvif_discovery_response.c | Uses copy_trimmed_value and safe_strcpy for parsing/preview buffers |
| src/video/onvif_motion_recording.c | Uses safe_strcpy for event/context fields |
| src/video/onvif_ptz.c | Uses safe_strcpy for preset token/name handling |
| src/video/packet_buffer.c | Uses safe_strcpy for names/paths and disk fallback |
| src/video/recording.c | Uses safe_strcpy for metadata + active recording paths/names and buffer sizing |
| src/video/sod_detection.c | Uses safe_strcpy for model/path and detection label handling |
| src/video/sod_integration.c | Uses safe_strcpy for model path resolution |
| src/video/sod_realnet.c | Uses safe_strcpy for detection labels |
| src/video/stream_manager.c | Uses safe_strcpy for thread-safe stream name copies and model path updates |
| src/video/stream_protocol.c | Uses safe_strcpy for safe URL copies and local URL buffer |
| src/video/stream_state.c | Uses safe_strcpy for stream name copies |
| src/video/stream_state_adapter.c | Uses safe_strcpy for stream name + model path updates |
| src/video/timestamp_manager.c | Uses safe_strcpy for stream name copies |
| src/video/unified_detection_thread.c | Uses safe_strcpy for context fields and local logging copy |
| src/video/zone_filter.c | Uses safe_strcpy for filter list copies and zone_id assignment |
| src/web/api_handlers_auth_backend_agnostic.c | Uses safe_strcpy for rate limit usernames, IP fallback, login fields |
| src/web/api_handlers_go2rtc_proxy.c | Uses safe_strcpy for response content-type |
| src/web/api_handlers_metrics.c | Uses safe_strcpy for response content-type and telemetry fields |
| src/web/api_handlers_motion.c | Uses safe_strcpy for config codec/quality |
| src/web/api_handlers_onvif_backend_agnostic.c | Uses safe_strcpy for device URL/credentials and safe URIs |
| src/web/api_handlers_ptz.c | Uses safe_strcpy for preset token |
| src/web/api_handlers_recordings_backend_agnostic.c | Uses safe_strcpy for file path copy and batch thread job_id |
| src/web/api_handlers_recordings_batch_download.c | Uses safe_strcpy and expands filename/tmp buffers to MAX_PATH_LENGTH |
| src/web/api_handlers_recordings_list.c | Uses shared trim/copy helpers for stream filters |
| src/web/api_handlers_streaming.c | Uses safe_strcpy for stream name parsing and storage path copies |
| src/web/api_handlers_streams_get.c | Uses safe_strcpy for safe URL + ONVIF credentials extraction copies |
| src/web/api_handlers_streams_test.c | Uses safe_strcpy for codec and URL copies |
| src/web/api_handlers_system.c | Adds trim-copy helper based on new trim primitives; uses safe_strcpy in system info |
| src/web/api_handlers_timeline.c | Uses safe_strcpy for SQL outputs and manifest path copies |
| src/web/api_handlers_users_backend_agnostic.c | Uses safe_strcpy for user fields and removes some zero-init |
| src/web/api_handlers_zones.c | Reorders includes and uses safe_strcpy for zone fields |
| src/web/batch_delete_progress.c | Uses safe_strcpy for job id/status/error messages |
| src/web/go2rtc_proxy_thread.c | Uses safe_strcpy for request method/content-type and response content-type |
| src/web/libuv_connection.c | Uses safe_strcpy for request parsing fields and expands static path buffers |
| src/web/libuv_file_serve.c | Uses safe_strcpy for ctx content-type/headers |
| src/web/libuv_server.c | Uses safe_strcpy for handler registration and client IP |
| src/web/request_response.c | Uses safe_strcpy for headers/content-type and deferred file serve fields |
| tests/test_sod_unified.c | Switches label copy to safe_strcpy |
| tests/unit/test_api_handlers_system.c | Expands temp path buffers and switches to safe_strcpy |
| tests/unit/test_config.c | Standardizes test path buffers on MAX_PATH_LENGTH |
| tests/unit/test_cross_stream_motion_trigger.c | Switches config field copies to safe_strcpy |
| tests/unit/test_db_auth.c | Switches allowed CIDR copies to safe_strcpy |
| tests/unit/test_db_detections.c | Switches label/path copies to safe_strcpy |
| tests/unit/test_db_maintenance.c | Switches metadata field copies to safe_strcpy |
| tests/unit/test_db_motion_config.c | Switches codec/quality and stream fields to safe_strcpy |
| tests/unit/test_db_recordings_extended.c | Switches metadata/label copies to safe_strcpy |
| tests/unit/test_db_recordings_sync.c | Switches metadata field copies to safe_strcpy |
| tests/unit/test_db_streams.c | Switches config fields to safe_strcpy |
| tests/unit/test_db_zones.c | Switches zone/config fields to safe_strcpy |
| tests/unit/test_go2rtc_process_detection.c | Switches temp dir copy to safe_strcpy |
| tests/unit/test_logger_json.c | Uses MAX_PATH_LENGTH for tmp log path |
| tests/unit/test_memory.c | Removes safe_str* tests (moved to strings tests) |
| tests/unit/test_request_response.c | Switches request/header field copies to safe_strcpy |
| tests/unit/test_storage_manager_retention.c | Switches metadata/config copies to safe_strcpy and minor decl splits |
| tests/unit/test_storage_retention_sqlite.c | Switches metadata field copies to safe_strcpy |
| tests/unit/test_stream_manager.c | Switches config copies to safe_strcpy |
| tests/unit/test_stream_state.c | Switches config copies to safe_strcpy |
| tests/unit/test_strings.c | Adds unit tests for safe_strdup/safe_strcpy/safe_strcat |
| tests/unit/test_zone_filter.c | Switches detection/zone/config field copies to safe_strcpy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
@matteius mind re-queuing the Copilot review? I should have things in better shape now. |
Contributor
Author
|
Edit: I've still got some fencepost errors I'm working out. |
Contributor
Author
|
Marking this as draft until I write some unit tests and I'm happy with the result. |
dpw13
force-pushed
the
fix/audit-strcpy
branch
2 times, most recently
from
35e3efc to
8aabc37
Compare
Contributor
Author
|
Unit tests added, and interactive testing looks good. Ready for re-review. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 134 out of 134 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
src/web/api_handlers_system.c:86
trim_copy_value()computesstart/endbased on trimmed positions, but then (1) checks the closing quote using*endeven thoughrtrim_pos()returns a pointer after the last printable char (should check*(end-1)), and (2) copies fromsrcinstead ofstart, so leading whitespace/quotes are not actually removed. Update the quote check and copy from the trimmed start pointer (and ensureendsemantics stay as “one past last char”).
src/tools/rebuild_recordings.c:217stream_name_endis computed but ignored;safe_strcpy(info->stream_name, stream_name_start, ..., 0)will copy the rest of the path (including/.../recording.mp4) intoinfo->stream_name. Use the bounded-copy form (passstream_name_end - stream_name_startassrc_size) so only the stream name segment is copied.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
String-related changes: * Move safe_str* from memory.c to strings.c * Replace (most) instances of strncpy with safe_strcpy * Removed now-duplicate termination and zero-initialization of strings where possible * Consolidate whitespace trimming functions to strings.c * Add unit tests for new functions in strings.c Incidental changes along the way: * Move rebuild_recordings.c from utils/ to tools/ to separate source utility functions from useful CLI programs * Use MAX_PATH_LENGTH for paths in a few more places
Contributor
Author
|
Squashed and ready, unless there are any further issues that need resolving. |
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.
String-related changes:
safe_str*from memory.c to strings.cstrncpywithsafe_strcpyIncidental changes along the way:
MAX_PATH_LENGTHfor paths in a few more placesThis PR attempts to replace all simple string copy implementations with a single, safe implementation in strings.c. Most places in the code where more complex string manipulation or parsing occurs that use
memcpyhave been left intact unless they were obviously trivial.