Aravis: fix memory safety bugs, including a crash on any camera with a fixed ROI - #981
Open
HazenBabcock wants to merge 3 commits into
Open
Aravis: fix memory safety bugs, including a crash on any camera with a fixed ROI#981HazenBabcock wants to merge 3 commits into
HazenBabcock wants to merge 3 commits into
Conversation
… camera. ArvCheckError() took its GError by value, so g_clear_error() freed the error but cleared only the local copy of the pointer. Every caller was left holding a dangling non-NULL pointer, passed it to its next Aravis call, and checked it again -- reading freed memory. Initialize() calls ClearROI(), whose first act is arv_camera_set_region(); on a camera that does not allow the region to be set, that fails, and the next check dereferences the freed error. The result was a segfault while opening the camera, on any camera with a fixed ROI. ArvCheckError() now takes a GError **. Also in this commit: - The camera name was copied into malloc(strlen(name)) using CDeviceUtils::CopyLimitedString(), which writes strlen(name) + 1 bytes. The terminating NUL landed one byte past the allocation for every camera. The name is now a std::string. - The stream's buffer pop was a side effect of g_assert(). Built with G_DISABLE_ASSERT the pop would disappear and the stream would starve once its buffers ran out; a genuine mismatch aborted the application instead of being handled. The pop is now unconditional, and a NULL or unexpected buffer is logged. - Shutdown() did nothing. It now stops a running acquisition, releases the stream and the snap buffer, and frees the image buffer. It is idempotent, and the destructor calls it, so unloading a configuration no longer leaks or leaves a stream writing into a freed buffer. - `capturing` is written by the Micro-Manager thread and read by the Aravis stream callback thread; it is now std::atomic<bool>. A mutex guards the image buffer, which the callback may reallocate while GetImageBuffer() reads it. - SnapImage() passed a zero timeout to arv_camera_acquisition(), which Aravis documents as "no timeout" and implements with a blocking pop. A camera waiting on a hardware trigger, or a single dropped packet, hung the application with no way out. It now waits five times the exposure time, with a five second floor, and logs a timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ArvPixelFormatUpdate()'s default branch logged and returned without touching the image description, leaving whatever the previous format had set -- and on a camera whose format is unsupported from the start, that is the constructor's zeros. Zero components is not one, so ArvBufferUpdate() took the packed-RGB path and rgb_to_rgba() wrote four bytes per pixel into a buffer malloc'd for width * height * 0. Snapping an image on such a camera segfaulted. The default branch now zeroes the description explicitly and names the format in the log rather than on stdout, which a Micro-Manager user never sees. ArvBufferUpdate() refuses to copy when the description is unusable, and also checks that the Aravis buffer actually holds as many bytes as the copy will read -- for the packed RGB formats that is three bytes per pixel, not the four the destination gets. The camera now fails to produce an image and says why, instead of crashing. Recognising more formats is a separate change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Initialize() calls arv_camera_is_binning_available() and then drops the answer into a local variable. Every other binning entry point asks the camera again: OnBinning()'s BeforeGet, which Micro-Manager drives on every property refresh, and GetBinning(), which MMCore calls whenever it needs the binning factor -- including from getPixelSizeUm(), which is taken once per image while tagging metadata. On a camera with no BinningHorizontal feature each of those calls fails, and every failure is logged, so live acquisition filled the CoreLog with Aravis Error: [BinningHorizontal] Not found Keep the probe result and consult it instead. The Binning property is still created, fixed at 1, because Micro-Manager expects a camera to have one; reporting it no longer costs a failed register read. The bug predates this branch but was unreachable: before the GError fix a camera with a fixed ROI crashed during Initialize() and never got far enough to log anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
AI Tools Note
The following summary was generated by Claude Code. I have reviewed it's summary
and the changes it proposed and believe them to be correct.
Summary
Six memory-safety fixes in the Aravis camera adapter. One is a crash that stops
any camera whose ROI cannot be set from opening at all.
No new features, and no intended behaviour change beyond not crashing — the
pixel formats recognised, the ROI handling and the property set are all as
before. Correctness fixes (
GetROI, bit depth reporting, RGB byte order) andcapability handling are deliberately left for follow-up changes to keep this one
reviewable.
The crash
ArvCheckError()took itsGError *by value:g_clear_error()frees the error and NULLs the pointer it is given — but thatis this function's own copy. The caller's pointer still points at freed memory,
and callers reuse one
GError *across several Aravis calls:Initialize()callsClearROI(), whose first action isarv_camera_set_region().On a camera that does not allow the region to be set that call fails, and the
next check dereferences the freed error. The result is a segfault while opening
the camera.
This is why it has gone unnoticed: on a camera with an adjustable ROI the
set_region()succeeds, no error is ever allocated, and the path is nevertaken.
Under AddressSanitizer:
ArvCheckError()now takes aGError **. That is the bulk of the diff: 70 callsites, mechanically changed to pass
&gerror.Also in this PR
The camera name was copied into a buffer one byte too small. The
constructor did
malloc(strlen(name))and thenCDeviceUtils::CopyLimitedString(), which writesstrlen(name) + 1bytes, sothe terminating NUL landed past the end of the allocation for every camera.
The name is now a
std::string.The stream's buffer pop was a side effect of
g_assert(). Built withG_DISABLE_ASSERTthe pop would disappear and the stream would starve onceits buffers ran out; a genuine mismatch aborted the application rather than
being handled. The pop is now unconditional, and a NULL or unexpected buffer
is logged.
Shutdown()did nothing. It now stops a running acquisition, releases thestream and the snap buffer, and frees the image buffer. It is idempotent and
the destructor calls it, so unloading a configuration no longer leaks or
leaves a stream writing into a freed buffer.
capturingwas a plainboolwritten by the Micro-Manager thread andread by the Aravis stream callback thread. It is now
std::atomic<bool>, anda mutex guards the image buffer, which the callback may reallocate while
GetImageBuffer()is reading it.SnapImage()could hang forever. It passed a zero timeout toarv_camera_acquisition(), which Aravis documents as "no timeout" andimplements with a blocking pop. A camera waiting on a hardware trigger, or a
single dropped packet, blocked the calling thread with no way out. It now
waits five times the exposure time with a five second floor, and logs on
timeout.
An unimplemented pixel format overflowed the image buffer.
ArvPixelFormatUpdate()'sdefaultbranch returned without touching theimage description, leaving the constructor's zeros on a camera whose format is
unsupported from the start. Zero components is not one, so
ArvBufferUpdate()took the packed-RGB path and wrote four bytes per pixel into a buffer
allocated for
width * height * 0. The default branch now zeroes thedescription explicitly and logs to the Micro-Manager log rather than stdout,
and
ArvBufferUpdate()refuses to copy when the description is unusable orwhen the source buffer is smaller than the copy would read.
Testing
Linux, Aravis 0.8.36, against a GigE Vision emulator over loopback presenting
five capability profiles: a minimal camera (exposure only, fixed geometry), a
mono camera (Mono8/10/12/16), a feature-rich camera (gain, black level, gamma,
auto modes, trigger, binning, temperature), a colour camera (Bayer, RGB8, BGR8),
and a camera whose current format the adapter does not implement.
InitializeInitializeInitializeInitializeInitializeWith the adapter built
-fsanitize=address: no use-after-free, noheap-buffer-overflow, and no leak report naming the adapter. Ten
load / initialize / snap / unload cycles grow RSS by 0 kB, which covers both
that
Shutdown()releases and that it tolerates running twice.