Avoid inexact stream reads

dotnet · Dec 8, 2024 · 27378e2 · 27378e2
1 parent 2009e91
commit 27378e2
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 39 deletions.
diff --git a/src/NerdBank.GitVersioning/ManagedGit/GitObjectStream.cs b/src/NerdBank.GitVersioning/ManagedGit/GitObjectStream.cs
@@ -45,7 +45,7 @@ public GitObjectStream(Stream stream, string objectType)
     private void ReadObjectTypeAndLength(string objectType)
     {
         Span<byte> buffer = stackalloc byte[128];
-        this.Read(buffer.Slice(0, objectType.Length + 1));
+        this.ReadAll(buffer.Slice(0, objectType.Length + 1));
 
         string? actualObjectType = GitRepository.GetString(buffer.Slice(0, objectType.Length));
         this.ObjectType = actualObjectType;
@@ -55,7 +55,7 @@ private void ReadObjectTypeAndLength(string objectType)
 
         while (headerLength < buffer.Length)
         {
-            this.Read(buffer.Slice(headerLength, 1));
+            this.ReadAll(buffer.Slice(headerLength, 1));
 
             if (buffer[headerLength] == 0)
             {

diff --git a/src/NerdBank.GitVersioning/ManagedGit/GitPackReader.cs b/src/NerdBank.GitVersioning/ManagedGit/GitPackReader.cs
@@ -24,60 +24,67 @@ public static Stream GetObject(GitPack pack, Stream stream, long offset, string
             throw new ArgumentNullException(nameof(stream));
         }
 
-        // Read the signature
+        try
+        {
+            // Read the signature
 #if DEBUG
-        stream.Seek(0, SeekOrigin.Begin);
-        Span<byte> buffer = stackalloc byte[12];
-        stream.ReadAll(buffer);
+            stream.Seek(0, SeekOrigin.Begin);
+            Span<byte> buffer = stackalloc byte[12];
+            stream.ReadAll(buffer);
 
-        Debug.Assert(buffer.Slice(0, 4).SequenceEqual(Signature));
+            Debug.Assert(buffer.Slice(0, 4).SequenceEqual(Signature));
 
-        int versionNumber = BinaryPrimitives.ReadInt32BigEndian(buffer.Slice(4, 4));
-        Debug.Assert(versionNumber == 2);
+            int versionNumber = BinaryPrimitives.ReadInt32BigEndian(buffer.Slice(4, 4));
+            Debug.Assert(versionNumber == 2);
 
-        int numberOfObjects = BinaryPrimitives.ReadInt32BigEndian(buffer.Slice(8, 4));
+            int numberOfObjects = BinaryPrimitives.ReadInt32BigEndian(buffer.Slice(8, 4));
 #endif
 
-        stream.Seek(offset, SeekOrigin.Begin);
+            stream.Seek(offset, SeekOrigin.Begin);
 
-        (GitPackObjectType type, long decompressedSize) = ReadObjectHeader(stream);
+            (GitPackObjectType type, long decompressedSize) = ReadObjectHeader(stream);
 
-        if (type == GitPackObjectType.OBJ_OFS_DELTA)
-        {
-            long baseObjectRelativeOffset = ReadVariableLengthInteger(stream);
-            long baseObjectOffset = offset - baseObjectRelativeOffset;
+            if (type == GitPackObjectType.OBJ_OFS_DELTA)
+            {
+                long baseObjectRelativeOffset = ReadVariableLengthInteger(stream);
+                long baseObjectOffset = offset - baseObjectRelativeOffset;
 
-            var deltaStream = new ZLibStream(stream, decompressedSize);
-            Stream? baseObjectStream = pack.GetObject(baseObjectOffset, objectType);
+                var deltaStream = new ZLibStream(stream, decompressedSize);
+                Stream? baseObjectStream = pack.GetObject(baseObjectOffset, objectType);
 
-            return new GitPackDeltafiedStream(baseObjectStream, deltaStream);
-        }
-        else if (type == GitPackObjectType.OBJ_REF_DELTA)
-        {
-            Span<byte> baseObjectId = stackalloc byte[20];
-            stream.ReadAll(baseObjectId);
+                return new GitPackDeltafiedStream(baseObjectStream, deltaStream);
+            }
+            else if (type == GitPackObjectType.OBJ_REF_DELTA)
+            {
+                Span<byte> baseObjectId = stackalloc byte[20];
+                stream.ReadAll(baseObjectId);
 
-            Stream baseObject = pack.GetObjectFromRepository(GitObjectId.Parse(baseObjectId), objectType)!;
-            var seekableBaseObject = new GitPackMemoryCacheStream(baseObject);
+                Stream baseObject = pack.GetObjectFromRepository(GitObjectId.Parse(baseObjectId), objectType)!;
+                var seekableBaseObject = new GitPackMemoryCacheStream(baseObject);
 
-            var deltaStream = new ZLibStream(stream, decompressedSize);
+                var deltaStream = new ZLibStream(stream, decompressedSize);
 
-            return new GitPackDeltafiedStream(seekableBaseObject, deltaStream);
-        }
+                return new GitPackDeltafiedStream(seekableBaseObject, deltaStream);
+            }
+
+            // Tips for handling deltas: https://github.com/choffmeister/gitnet/blob/4d907623d5ce2d79a8875aee82e718c12a8aad0b/src/GitNet/GitPack.cs
+            if (type != packObjectType)
+            {
+                throw new GitException($"An object of type {objectType} could not be located at offset {offset}.") { ErrorCode = GitException.ErrorCodes.ObjectNotFound };
+            }
 
-        // Tips for handling deltas: https://github.com/choffmeister/gitnet/blob/4d907623d5ce2d79a8875aee82e718c12a8aad0b/src/GitNet/GitPack.cs
-        if (type != packObjectType)
+            return new ZLibStream(stream, decompressedSize);
+        }
+        catch (EndOfStreamException eof)
         {
-            throw new GitException($"An object of type {objectType} could not be located at offset {offset}.") { ErrorCode = GitException.ErrorCodes.ObjectNotFound };
+            throw new GitException($"An object of type {objectType} could not be located at offset {offset}.", eof) { ErrorCode = GitException.ErrorCodes.ObjectNotFound };
         }
-
-        return new ZLibStream(stream, decompressedSize);
     }
 
     private static (GitPackObjectType ObjectType, long Length) ReadObjectHeader(Stream stream)
     {
         Span<byte> value = stackalloc byte[1];
-        stream.Read(value);
+        stream.ReadAll(value);
 
         var type = (GitPackObjectType)((value[0] & 0b0111_0000) >> 4);
         long length = value[0] & 0b_1111;
@@ -91,7 +98,7 @@ private static (GitPackObjectType ObjectType, long Length) ReadObjectHeader(Stre
 
         do
         {
-            stream.Read(value);
+            stream.ReadAll(value);
             length = length | ((value[0] & 0b0111_1111L) << shift);
             shift += 7;
         }

diff --git a/test/Nerdbank.GitVersioning.Tests/ManagedGit/GitPackDeltafiedStreamTests.cs b/test/Nerdbank.GitVersioning.Tests/ManagedGit/GitPackDeltafiedStreamTests.cs
@@ -23,7 +23,7 @@ public void TestDeltaStream(string basePath, string deltaPath, string expectedPa
         using (Stream expectedStream = TestUtilities.GetEmbeddedResource(expectedPath))
         {
             expected = new byte[expectedStream.Length];
-            expectedStream.Read(expected);
+            expectedStream.ReadAll(expected);
         }
 
         byte[] actual = new byte[expected.Length];
@@ -34,7 +34,7 @@ public void TestDeltaStream(string basePath, string deltaPath, string expectedPa
         {
             ////Assert.Equal(expected.Length, deltafiedStream.Length);
 
-            deltafiedStream.Read(actual);
+            deltafiedStream.ReadAll(actual);
 
             Assert.Equal(expected, actual);
         }

diff --git a/test/Nerdbank.GitVersioning.Tests/ManagedGit/ZLibStreamTest.cs b/test/Nerdbank.GitVersioning.Tests/ManagedGit/ZLibStreamTest.cs
@@ -41,7 +41,7 @@ public void SeekTest()
                 // Seek past the commit 137 header, and make sure we can read the 'tree' word
                 Assert.Equal(11, stream.Seek(11, SeekOrigin.Begin));
                 byte[] tree = new byte[4];
-                stream.Read(tree);
+                stream.ReadAll(tree);
                 Assert.Equal("tree", Encoding.UTF8.GetString(tree));
 
                 // Valid no-ops