Fix BGR encoding, thread safety, path traversal, and count limit in SavePicture service

[Copilot]

The SavePicture service had several correctness, safety, and security issues: wrong image encoding label, latest_image_ updated only via a self-subscription that couldn't run while the service callback was sleeping, unbounded count, arbitrary filesystem write paths, and a misleading log message.

Changes

• BGR encoding: cv::imdecode returns BGR — encoding was incorrectly declared as rgb8, now bgr8
• Thread safety / frame acquisition: Removed circular self-subscription (image_sub_). Each shot in handle_save_picture now waits up to 5 s for the next genuinely fresh frame from the read thread using a std::condition_variable and a sequence number (latest_image_seq_). notify_all() is called inside the lock after each frame is stored in handle_image_message. This guarantees every save receives a new camera frame rather than a potentially stale buffered one.
• Path traversal mitigation: save_directory in the request is now validated before use:
  • Absolute paths are rejected
  • .. components are rejected
  • weakly_canonical() is used to confirm the resolved path stays under the configured base save_directory_ (guards against symlink escapes)
• Bounded count: count > MAX_PICTURE_COUNT (100) is rejected with an error response
• Log message: Fixed to show actual service name /picam_client/save_picture
• SavePicture.srv: Comments updated to document the relative-path constraint and max count

// Example: rejected requests
request->save_directory = "/etc/cron.d";        // absolute path → error
request->save_directory = "../../var/app/data"; // ".." component → error
request->count = 500;                           // exceeds MAX_PICTURE_COUNT → error

---

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

[TomHagdorn]

@copilot Why even use mutexes in the first place. I can also just grab the next fram right? No need to always have one on hand

[Copilot]

@copilot Why even use mutexes in the first place. I can also just grab the next fram right? No need to always have one on hand

You still need a mutex — condition_variable requires one for correct wakeup/predicate semantics since the read thread and service callback run concurrently. But the design is now "grab next frame" as you suggested (commit 9b35b08):

Each shot records the current frame sequence number, then calls wait_for (5 s timeout) until the read thread delivers a new frame (latest_image_seq_ > wait_seq). No stale buffered image is used — every save waits for a genuinely fresh frame from the camera.