From efd0ff01c7027f4a0d3df6fb208f16da8a8f0ec6 Mon Sep 17 00:00:00 2001 From: Kevin Scrudders Date: Wed, 19 Aug 2026 13:50:57 -0400 Subject: [PATCH] Fix out-of-bounds reads when marshalling SDK strings Lim_FileGetAttributes, Lim_FileGetTextinfo, Lim_FileGetMetadata, Lim_FileGetFrameMetadata and Lim_FileGetExperiment each return a LIMSTR allocated to exactly strlen+1 bytes, and the SDK offers no size query. The readers guessed that length instead of finding it: setdatatype(p, type, N) only records the declared extent, but the p.Value that follows memcpys all N bytes out of the raw address, so every read past the terminator ran off the end of the heap block. Measured on a 5-channel TIRF file, the previous code read 12000 bytes from a 6142-byte textinfo buffer and from a 7098-byte metadata buffer, 500 bytes from a 210-byte attributes buffer, and 3000 bytes from a 408-byte experiment buffer. Such a read usually lands on a mapped page and appears to work, which is why it went unnoticed, but when the block sits near the end of a committed region it faults and kills the MATLAB process with an access violation rather than raising a catchable error. Add ND2ReadString, which finds the terminator one byte at a time. Byte k is read only after bytes 0..k-1 are known to be non-NUL, which proves the allocation holds at least k+1 bytes, so no read is ever out of bounds. The payload is then taken in a single exact read. Each call site passes the pointer type it used before, so text handling is unchanged byte for byte. ND2Info keeps 'int8Ptr', where char() folds bytes above 127 to NUL and CheckInfo's Description(Description == 0) = ' ' compensates; ND2Open and SeqInfo keep 'uint8Ptr'. SeqInfo's existing free was itself an overrun of the same kind: it re-declared the string as 'voidPtr' with the same element count before calling Lim_FileFreeString, and calllib marshals the full declared extent of a lib.pointer argument, so MATLAB read eight bytes per element. Add ND2FreeString, which shrinks the extent first, and use it there. Incidentally this also removes a latent bug in SeqInfo's frame loop, which reused TestLength from the frame 0 probe: a frame whose metadata was longer produced an empty index and an error out of jsondecode. This change is limited to reads that can fault the process. The missing Lim_FileFreeString calls in ND2Info and ND2Open, the missing Lim_DestroyPicture in ND2ReadSingle, and the unchecked Lim_FileOpenForReadUtf8 return value are left for a separate change. --- ND2FreeString.m | 26 +++++++++++++++ ND2Info.m | 42 +++++------------------- ND2Open.m | 7 ++-- ND2ReadString.m | 87 +++++++++++++++++++++++++++++++++++++++++++++++++ SeqInfo.m | 27 +++++---------- 5 files changed, 133 insertions(+), 56 deletions(-) create mode 100644 ND2FreeString.m create mode 100644 ND2ReadString.m diff --git a/ND2FreeString.m b/ND2FreeString.m new file mode 100644 index 0000000..3665465 --- /dev/null +++ b/ND2FreeString.m @@ -0,0 +1,26 @@ +function [] = ND2FreeString(StringPointer, DataType) +% ND2FreeString releases a LIMSTR returned by the ND2 SDK. +% +% ND2FreeString(StringPointer, DataType) calls Lim_FileFreeString on the +% buffer referenced by StringPointer, which Nd2ReadSdk.h requires for every +% string returned by Lim_FileGetAttributes, Lim_FileGetTextinfo, +% Lim_FileGetMetadata, Lim_FileGetFrameMetadata and Lim_FileGetExperiment. +% A null pointer is ignored. DataType defaults to 'uint8Ptr'. +% +% The declared extent is shrunk to one element first. calllib marshals the +% full declared extent of a lib.pointer argument, so passing a pointer that +% is still declared as thousands of elements makes MATLAB read past the end +% of the buffer while marshalling the argument, which can fault. + + if nargin < 2 + DataType = 'uint8Ptr'; + end + + if ~isa(StringPointer, 'lib.pointer') || StringPointer.isNull + return + end + + setdatatype(StringPointer, DataType, 1); + calllib('Nd2ReadSdk', 'Lim_FileFreeString', StringPointer); + +end diff --git a/ND2Info.m b/ND2Info.m index 4c8399e..de625dc 100644 --- a/ND2Info.m +++ b/ND2Info.m @@ -9,24 +9,16 @@ [FilePointer] = calllib('Nd2ReadSdk', 'Lim_FileOpenForReadUtf8', FileID); numImages = calllib('Nd2ReadSdk', 'Lim_FileGetSeqCount', FilePointer); CoordSize = calllib('Nd2ReadSdk', 'Lim_FileGetCoordSize', FilePointer); +% 'int8Ptr' is passed explicitly at every call site in this file so the existing +% text handling is preserved byte for byte. char() maps a negative int8 to 0, so +% bytes above 127 become NUL here, which is what CheckInfo's +% Description(Description == 0) = ' ' already compensates for. Attributes = calllib('Nd2ReadSdk', 'Lim_FileGetAttributes', FilePointer); -setdatatype(Attributes, 'int8Ptr', 500) -AttributesValue = Attributes.Value'; -Attributeslength = find(AttributesValue == 0, 1); -AttributesJson = char(AttributesValue(1:Attributeslength - 1)); +AttributesJson = ND2ReadString(Attributes, 'int8Ptr'); AttributesStru = jsondecode(AttributesJson); TextInfo = calllib('Nd2ReadSdk', 'Lim_FileGetTextinfo', FilePointer); -TestLength=3000; -setdatatype(TextInfo, 'int8Ptr', TestLength) -TextInfoValue = TextInfo.Value'; -while isempty(find(TextInfoValue == 0, 1)) - TestLength=TestLength*2; - setdatatype(TextInfo, 'int8Ptr', TestLength) - TextInfoValue = TextInfo.Value'; -end -TextInfolength = find(TextInfoValue == 0, 1); -TextInfoJson = char(TextInfoValue(1:TextInfolength - 1)); +TextInfoJson = ND2ReadString(TextInfo, 'int8Ptr'); TextInfoStru = jsondecode(TextInfoJson); Metadata = calllib('Nd2ReadSdk', 'Lim_FileGetMetadata', FilePointer); @@ -34,16 +26,7 @@ if Metadata.isNull MetadataStru=[]; else - TestLength=3000; - setdatatype(Metadata, 'int8Ptr', TestLength) - MetadataValue = Metadata.Value'; - while isempty(find(MetadataValue == 0, 1)) - TestLength=TestLength*2; - setdatatype(Metadata, 'int8Ptr', TestLength) - MetadataValue = Metadata.Value'; - end - Metadatalength = find(MetadataValue == 0, 1); - MetadataJson = char(MetadataValue(1:Metadatalength - 1)); + MetadataJson = ND2ReadString(Metadata, 'int8Ptr'); MetadataStru = jsondecode(MetadataJson); end @@ -54,16 +37,7 @@ ExperimentStru=[]; NumInCoord=[]; else - TestLength=3000; - setdatatype(Experiment, 'int8Ptr', TestLength) - ExperimentValue = Experiment.Value'; - while isempty(find(ExperimentValue == 0, 1)) - TestLength=TestLength*2; - setdatatype(Experiment, 'int8Ptr', TestLength) - ExperimentValue = Experiment.Value'; - end - Experimentlength = find(ExperimentValue == 0, 1); - ExperimentJson = char(ExperimentValue(1:Experimentlength - 1)); + ExperimentJson = ND2ReadString(Experiment, 'int8Ptr'); ExperimentStru=jsondecode(ExperimentJson); diff --git a/ND2Open.m b/ND2Open.m index e8903be..bb2c96d 100644 --- a/ND2Open.m +++ b/ND2Open.m @@ -9,11 +9,10 @@ [FilePointer] = calllib('Nd2ReadSdk', 'Lim_FileOpenForReadUtf8', FileID); % CoordSize = calllib('Nd2ReadSdk', 'Lim_FileGetCoordSize', FilePointer); % numImages = calllib('Nd2ReadSdk', 'Lim_FileGetSeqCount', FilePointer); + % The attributes JSON is only a couple of hundred bytes, so the fixed + % 500-byte read ran off the end of the SDK allocation on every call. Attibutes = calllib('Nd2ReadSdk', 'Lim_FileGetAttributes', FilePointer); - setdatatype(Attibutes, 'uint8Ptr', 500) - AttibutesValue = Attibutes.Value'; - Attibuteslength = find(AttibutesValue == 0, 1); - AttibutesJson = char(AttibutesValue(1:Attibuteslength - 1)); + AttibutesJson = ND2ReadString(Attibutes, 'uint8Ptr'); AttibutesStru = jsondecode(AttibutesJson); % Metadata = calllib('Nd2ReadSdk','Lim_FileGetMetadata',FilePointer); diff --git a/ND2ReadString.m b/ND2ReadString.m new file mode 100644 index 0000000..7c439f5 --- /dev/null +++ b/ND2ReadString.m @@ -0,0 +1,87 @@ +function [String] = ND2ReadString(StringPointer, DataType) +% ND2ReadString reads a NUL-terminated LIMSTR returned by the ND2 SDK. +% +% String = ND2ReadString(StringPointer, DataType) copies the C string +% referenced by StringPointer into a MATLAB character vector. Pass the +% lib.pointer returned by any SDK function that Nd2ReadSdk.h documents as +% returning a LIMSTR, i.e. Lim_FileGetAttributes, Lim_FileGetTextinfo, +% Lim_FileGetMetadata, Lim_FileGetFrameMetadata and Lim_FileGetExperiment. +% A null pointer returns an empty character vector. +% +% DataType is the pointer type to read the bytes as, and defaults to +% 'uint8Ptr'. It only affects how bytes above 127 reach char(): 'uint8Ptr' +% maps them to their Latin-1 characters, while 'int8Ptr' makes them negative +% so that char() silently turns them into NUL. Each caller passes whichever +% type it used before this function existed, so text handling is unchanged. +% +% This function does not release the buffer. The SDK requires that the +% caller do so with Lim_FileFreeString; see ND2FreeString. On return the +% declared extent of StringPointer is left small enough to pass safely back +% across calllib. +% +% Why the terminator is found one byte at a time +% ---------------------------------------------- +% The SDK allocates these buffers to exactly strlen+1 bytes and offers no +% size query. setdatatype(p, type, n) only records the declared extent, but +% the p.Value that follows memcpys all n bytes out of the raw address. Any n +% larger than the real allocation therefore reads past the end of the heap +% block. Such a read usually lands on a mapped page and appears to work, +% which is why guessing a length seemed to work, but when the block happens +% to sit near the end of a committed region the read faults and kills the +% whole MATLAB process with an access violation. +% +% Reading byte k only after bytes 0..k-1 have all been shown to be non-NUL +% proves the allocation holds at least k+1 bytes, so no read below is ever +% out of bounds. + + if nargin < 2 + DataType = 'uint8Ptr'; + end + + String = ''; + + if ~isa(StringPointer, 'lib.pointer') || StringPointer.isNull + return + end + + % A terminator is expected long before this. It is only a runaway guard. + MaxLength = 4194304; + + % Declaring a large extent costs nothing because setdatatype issues no + % read. It exists so the pointer arithmetic below stays inside the declared + % bounds. Every actual read is a single byte. + setdatatype(StringPointer, DataType, MaxLength); + + Length = 0; + while Length < MaxLength + Probe = StringPointer+Length; + setdatatype(Probe, DataType, 1); + + if Probe.Value(1) == 0 + break + end + + Length = Length+1; + end + + % Never leave the pointer declared as MaxLength. calllib marshals the full + % declared extent of a lib.pointer argument, so a large extent would make + % MATLAB itself read past the buffer when the caller frees it. + if Length >= MaxLength + setdatatype(StringPointer, DataType, 1); + error('ND2ReadString:NoTerminator', ... + ['The ND2 SDK returned a string with no terminator in the first %d bytes. ', ... + 'Check that Nd2ReadSdk.dll matches the Nd2ReadSdk.h used by loadlibrary ', ... + 'and that the ND2 file is not truncated.'], MaxLength); + end + + % Length is now known to be in bounds, so the payload can be taken in a + % single read that stops exactly at the terminator. + if Length > 0 + setdatatype(StringPointer, DataType, Length); + String = char(StringPointer.Value(:)'); + else + setdatatype(StringPointer, DataType, 1); + end + +end diff --git a/SeqInfo.m b/SeqInfo.m index 7231997..2e9cbb3 100644 --- a/SeqInfo.m +++ b/SeqInfo.m @@ -8,20 +8,12 @@ [FilePointer] = calllib('Nd2ReadSdk', 'Lim_FileOpenForReadUtf8', FileID); FrameMetadata = calllib('Nd2ReadSdk', 'Lim_FileGetFrameMetadata', FilePointer,0); -TestLength=3000; -setdatatype(FrameMetadata, 'uint8Ptr', TestLength) -FrameMetadataValue = FrameMetadata.Value'; -while isempty(find(FrameMetadataValue == 0, 1)) - TestLength=TestLength*2; - setdatatype(FrameMetadata, 'uint8Ptr', TestLength) - FrameMetadataValue = FrameMetadata.Value'; -end -FrameMetadataLength = find(FrameMetadataValue == 0, 1); -FrameMetadataJson = char(FrameMetadataValue(1:FrameMetadataLength - 1)); +FrameMetadataJson = ND2ReadString(FrameMetadata, 'uint8Ptr'); FrameMetadataStru=jsondecode(FrameMetadataJson); -setdatatype(FrameMetadata, 'voidPtr', TestLength) -calllib('Nd2ReadSdk', 'Lim_FileFreeString', FrameMetadata); +% The free was itself an overrun: re-declaring the string as 'voidPtr' with the +% same element count made calllib marshal eight bytes per element. +ND2FreeString(FrameMetadata, 'uint8Ptr'); if FrameMetadataStru.contents.channelCount==1 SeqTime=zeros(size(Num,2),1); @@ -38,14 +30,13 @@ for i=1:size(Num,2) + % This loop also reused TestLength from the frame 0 probe above, so a frame + % whose metadata was longer produced an empty index and an error out of + % jsondecode. Finding the terminator per frame removes that too. FrameMetadata = calllib('Nd2ReadSdk', 'Lim_FileGetFrameMetadata', FilePointer,Num(i)-1); - setdatatype(FrameMetadata, 'uint8Ptr', TestLength) - FrameMetadataValue = FrameMetadata.Value'; - FrameMetadataLength = find(FrameMetadataValue == 0, 1); - FrameMetadataJson = char(FrameMetadataValue(1:FrameMetadataLength - 1)); + FrameMetadataJson = ND2ReadString(FrameMetadata, 'uint8Ptr'); FrameMetadataStru=jsondecode(FrameMetadataJson); - setdatatype(FrameMetadata, 'voidPtr', TestLength) - calllib('Nd2ReadSdk', 'Lim_FileFreeString', FrameMetadata); + ND2FreeString(FrameMetadata, 'uint8Ptr'); if FrameMetadataStru.contents.channelCount==1 SeqTime(i)=FrameMetadataStru.channels.time.relativeTimeMs;