NDPluginROIStat: return after rank guard to prevent heap OOB on 3-D (RGB) arrays#594
Merged
Merged
Conversation
…rrays
The "number of array dimensions must be 1 or 2" guard in
processCallbacks() only printed a warning and fell through to the
geometry loop:
for (dim=0; dim<pArray->ndims; dim++) {
pROI->offset[dim] = ...;
pROI->size[dim] = ...;
pROI->arraySize[dim] = ...;
}
NDROI_t sizes offset[], size[] and arraySize[] to two elements each, so
for any array with ndims == 3 -- which includes every NDColorModeRGB1/2/3
frame -- dim reaches 2 and the loop writes index 2 of each 2-element
array. arraySize is the struct's last member, so arraySize[2] is written
8 bytes past the NDROI object, and for the last ROI past the
new NDROI[maxROIs_] allocation: a heap out-of-bounds write reachable from
an ordinary colour-detector configuration, with corrupted stats and a
likely crash in the delete[] that follows.
Bail out of processCallbacks() (releasing the pROIs allocation and
leaving the mutex locked, as the exit contract requires) instead of
merely warning, matching the identical guard in NDPluginTimeSeries which
already returns.
Verified with an AddressSanitizer proof driver mirroring the NDROI_t
layout and the dim loop: a 3-D array triggers a heap-buffer-overflow
WRITE of size 8 without the return, and returns cleanly with it.
Member
|
@physwkim thanks for finding all of the bugs in ADCore and asyn. I am curious how you did this. Did you have an AI agent study the code? If so, that seems very useful. |
Contributor
Author
@MarkRivers Come across this while working on epics-rs, a Rust port of EPICS - spotted the upstream bug during the port and reported it. |
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
2 participants
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.
The "dimensions must be 1 or 2" guard in
processCallbacks()only warns and falls through to the geometry loopfor (dim=0; dim<pArray->ndims; dim++), which writesoffset/size/arraySize[dim].NDROI_tsizes those to 2 elements, so anyndims==3frame — every RGB1/2/3 colour image — writes index 2, past each array and, for the last ROI, past thenew NDROI[maxROIs_]allocation: a heap out-of-bounds write reachable from an ordinary colour-detector setup, with corrupt stats and a likely crash in the followingdelete[]. Bail out of the callback (freeingpROIs, leaving the mutex locked per the exit contract), matching the identical guard inNDPluginTimeSerieswhich already returns. Proven with an AddressSanitizer driver: the buggy path reports a heap-buffer-overflow WRITE of size 8 at thearraySize[dim]store; the fixed path returns cleanly.