Skip to content

Conversation

HTRamsey
Copy link
Collaborator

@HTRamsey HTRamsey commented Aug 9, 2025

This should fix a use after free issue
Just stop requesting messages during the RequestMessageTest rather than deleting the manager itself.
Formatting fixes

@HTRamsey HTRamsey requested a review from Copilot August 9, 2025 10:19
Copilot

This comment was marked as outdated.

@DonLakeFlyer
Copy link
Contributor

Should the prepareDelete do a camera manager stop now instead of nothing. Just to be safe?

@HTRamsey HTRamsey requested a review from Copilot August 22, 2025 01:05
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the CameraManager to fix a use-after-free issue in the RequestMessageTest by implementing a stop mechanism instead of deleting the manager entirely. The changes also include code reorganization and cleanup for better maintainability.

  • Replaces deleteCameraManager() with stopCameraManager() to gracefully stop camera requests during testing
  • Reorganizes Camera Manager code into a dedicated section within the Vehicle class for better maintainability
  • Improves code quality through consistent formatting, explicit types, and safer memory management

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/Vehicle/RequestMessageTest.cc Updated test to use stopCameraManager() instead of deleteCameraManager() to prevent use-after-free
src/VideoManager/VideoManager.cc Added include for MavlinkCameraControl header
src/Vehicle/Vehicle.h Reorganized camera manager declarations into dedicated section and updated API
src/Vehicle/Vehicle.cc Moved camera manager implementation to dedicated section and simplified lifecycle management
src/QmlControls/QmlObjectListModel.h Added isEmpty() convenience method
src/Camera/QGCCameraManager.h Refactored class structure with improved organization and added start/stop functionality
src/Camera/QGCCameraManager.cc Comprehensive refactor with improved error handling, memory safety, and code organization
custom-example/src/FirmwarePlugin/CustomFirmwarePlugin.h Removed unused CustomCameraManager forward declaration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@HTRamsey
Copy link
Collaborator Author

HTRamsey commented Aug 22, 2025

@DonLakeFlyer so one question I have is the original code only removes one stale camera at a time in _checkForLostCameras, any reason not to iterate all of them? I guess it's just deferring the rest of the cleanup to 500ms later but I don't see why you wouldn't just perform the full loop.

@HTRamsey HTRamsey force-pushed the dev-cameramanager branch 4 times, most recently from ef12add to ae4e191 Compare August 26, 2025 20:08
@HTRamsey HTRamsey merged commit 5a48df6 into mavlink:master Aug 27, 2025
13 checks passed
@HTRamsey HTRamsey deleted the dev-cameramanager branch August 27, 2025 00:48
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