Fix out-of-bounds reads when marshalling SDK strings

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

ND2Info.m

@@ -9,41 +9,24 @@
 [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);
 
 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);
 
 

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);

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

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;