Merge pull request #588 from JBildstein/icc-improvements

ICC Improvements

Author: JimBobSquarePants

src/ImageSharp/Formats/Jpeg/GolangPort/GolangJpegDecoderCore.cs

@@ -450,6 +450,11 @@ private void InitDerivedMetaDataProperties()
                     this.MetaData.VerticalResolution = verticalValue;
                 }
             }
+
+            if (this.MetaData.IccProfile?.CheckIsValid() == false)
+            {
+                this.MetaData.IccProfile = null;
+            }
         }
 
         /// <summary>

src/ImageSharp/Formats/Jpeg/PdfJsPort/PdfJsJpegDecoderCore.cs

@@ -196,7 +196,7 @@ public Image<TPixel> Decode<TPixel>(Stream stream)
             where TPixel : struct, IPixel<TPixel>
         {
             this.ParseStream(stream);
-            this.AssignResolution();
+            this.InitDerivedMetaDataProperties();
             return this.PostProcessIntoImage<TPixel>();
         }
 
@@ -207,7 +207,7 @@ public Image<TPixel> Decode<TPixel>(Stream stream)
         public IImageInfo Identify(Stream stream)
         {
             this.ParseStream(stream, true);
-            this.AssignResolution();
+            this.InitDerivedMetaDataProperties();
             return new ImageInfo(new PixelTypeInfo(this.BitsPerPixel), this.ImageWidth, this.ImageHeight, this.MetaData);
         }
 
@@ -395,9 +395,9 @@ private JpegColorSpace DeduceJpegColorSpace()
         }
 
         /// <summary>
-        /// Assigns the horizontal and vertical resolution to the image if it has a JFIF header or EXIF metadata.
+        /// Assigns derived metadata properties to <see cref="MetaData"/>, eg. horizontal and vertical resolution if it has a JFIF header.
         /// </summary>
-        private void AssignResolution()
+        private void InitDerivedMetaDataProperties()
         {
             if (this.jFif.XDensity > 0 && this.jFif.YDensity > 0)
             {
@@ -420,6 +420,11 @@ private void AssignResolution()
                     this.MetaData.VerticalResolution = verticalValue;
                 }
             }
+
+            if (this.MetaData.IccProfile?.CheckIsValid() == false)
+            {
+                this.MetaData.IccProfile = null;
+            }
         }
 
         /// <summary>

src/ImageSharp/MetaData/Profiles/ICC/DataReader/IccDataReader.cs

@@ -1,7 +1,6 @@
 // Copyright (c) Six Labors and contributors.
 // Licensed under the Apache License, Version 2.0.
 
-using System;
 using System.Text;
 
 namespace SixLabors.ImageSharp.MetaData.Profiles.Icc
@@ -11,7 +10,6 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Icc
     /// </summary>
     internal sealed partial class IccDataReader
     {
-        private static readonly bool IsLittleEndian = BitConverter.IsLittleEndian;
         private static readonly Encoding AsciiEncoding = Encoding.GetEncoding("ASCII");
 
         /// <summary>
@@ -34,6 +32,14 @@ public IccDataReader(byte[] data)
             this.data = data;
         }
 
+        /// <summary>
+        /// Gets the length in bytes of the raw data
+        /// </summary>
+        public int DataLength
+        {
+            get { return this.data.Length; }
+        }
+
         /// <summary>
         /// Sets the reading position to the given value
         /// </summary>

src/ImageSharp/MetaData/Profiles/ICC/IccProfile.cs

@@ -52,17 +52,12 @@ public IccProfile(byte[] data)
         /// by making a copy from another ICC profile.
         /// </summary>
         /// <param name="other">The other ICC profile, where the clone should be made from.</param>
-        /// <exception cref="System.ArgumentNullException"><paramref name="other"/> is null.</exception>>
+        /// <exception cref="ArgumentNullException"><paramref name="other"/> is null.</exception>>
         public IccProfile(IccProfile other)
         {
             Guard.NotNull(other, nameof(other));
 
-            // TODO: Do we need to copy anything else?
-            if (other.data != null)
-            {
-                this.data = new byte[other.data.Length];
-                Buffer.BlockCopy(other.data, 0, this.data, 0, other.data.Length);
-            }
+            this.data = other.ToByteArray();
         }
 
         /// <summary>
@@ -108,7 +103,7 @@ public List<IccTagDataEntry> Entries
 #if !NETSTANDARD1_1
 
         /// <summary>
-        /// Calculates the MD5 hash value of an ICC profile header
+        /// Calculates the MD5 hash value of an ICC profile
         /// </summary>
         /// <param name="data">The data of which to calculate the hash value</param>
         /// <returns>The calculated hash</returns>
@@ -117,22 +112,38 @@ public static IccProfileId CalculateHash(byte[] data)
             Guard.NotNull(data, nameof(data));
             Guard.IsTrue(data.Length >= 128, nameof(data), "Data length must be at least 128 to be a valid profile header");
 
-            byte[] header = new byte[128];
-            Buffer.BlockCopy(data, 0, header, 0, 128);
+            const int profileFlagPos = 44;
+            const int renderingIntentPos = 64;
+            const int profileIdPos = 84;
+
+            // need to copy some values because they need to be zero for the hashing
+            byte[] temp = new byte[24];
+            Buffer.BlockCopy(data, profileFlagPos, temp, 0, 4);
+            Buffer.BlockCopy(data, renderingIntentPos, temp, 4, 4);
+            Buffer.BlockCopy(data, profileIdPos, temp, 8, 16);
 
             using (var md5 = MD5.Create())
             {
-                // Zero out some values
-                Array.Clear(header, 44, 4);     // Profile flags
-                Array.Clear(header, 64, 4);     // Rendering Intent
-                Array.Clear(header, 84, 16);    // Profile ID
-
-                // Calculate hash
-                byte[] hash = md5.ComputeHash(data);
-
-                // Read values from hash
-                var reader = new IccDataReader(hash);
-                return reader.ReadProfileId();
+                try
+                {
+                    // Zero out some values
+                    Array.Clear(data, profileFlagPos, 4);
+                    Array.Clear(data, renderingIntentPos, 4);
+                    Array.Clear(data, profileIdPos, 16);
+
+                    // Calculate hash
+                    byte[] hash = md5.ComputeHash(data);
+
+                    // Read values from hash
+                    var reader = new IccDataReader(hash);
+                    return reader.ReadProfileId();
+                }
+                finally
+                {
+                    Buffer.BlockCopy(temp, 0, data, profileFlagPos, 4);
+                    Buffer.BlockCopy(temp, 4, data, renderingIntentPos, 4);
+                    Buffer.BlockCopy(temp, 8, data, profileIdPos, 16);
+                }
             }
         }
 
@@ -149,14 +160,37 @@ public void Extend(byte[] bytes)
             Buffer.BlockCopy(bytes, 0, this.data, currentLength, bytes.Length);
         }
 
+        /// <summary>
+        /// Checks for signs of a corrupt profile.
+        /// </summary>
+        /// <remarks>This is not an absolute proof of validity but should weed out most corrupt data.</remarks>
+        /// <returns>True if the profile is valid; False otherwise</returns>
+        public bool CheckIsValid()
+        {
+            return Enum.IsDefined(typeof(IccColorSpaceType), this.Header.DataColorSpace) &&
+                   Enum.IsDefined(typeof(IccColorSpaceType), this.Header.ProfileConnectionSpace) &&
+                   Enum.IsDefined(typeof(IccRenderingIntent), this.Header.RenderingIntent) &&
+                   this.Header.Size >= 128 &&
+                   this.Header.Size < 50_000_000; // it's unlikely there is a profile bigger than 50MB
+        }
+
         /// <summary>
         /// Converts this instance to a byte array.
         /// </summary>
         /// <returns>The <see cref="T:byte[]"/></returns>
         public byte[] ToByteArray()
         {
-            var writer = new IccWriter();
-            return writer.Write(this);
+            if (this.data != null)
+            {
+                byte[] copy = new byte[this.data.Length];
+                Buffer.BlockCopy(this.data, 0, copy, 0, copy.Length);
+                return copy;
+            }
+            else
+            {
+                var writer = new IccWriter();
+                return writer.Write(this);
+            }
         }
 
         private void InitializeHeader()

src/ImageSharp/MetaData/Profiles/ICC/IccReader.cs

@@ -84,45 +84,66 @@ private IccProfileHeader ReadHeader(IccDataReader reader)
         private IccTagDataEntry[] ReadTagData(IccDataReader reader)
         {
             IccTagTableEntry[] tagTable = this.ReadTagTable(reader);
-            var entries = new IccTagDataEntry[tagTable.Length];
+            var entries = new List<IccTagDataEntry>(tagTable.Length);
             var store = new Dictionary<uint, IccTagDataEntry>();
-            for (int i = 0; i < tagTable.Length; i++)
+
+            foreach (IccTagTableEntry tag in tagTable)
             {
                 IccTagDataEntry entry;
-                uint offset = tagTable[i].Offset;
-                if (store.ContainsKey(offset))
+                if (store.ContainsKey(tag.Offset))
                 {
-                    entry = store[offset];
+                    entry = store[tag.Offset];
                 }
                 else
                 {
-                    entry = reader.ReadTagDataEntry(tagTable[i]);
-                    store.Add(offset, entry);
+                    try
+                    {
+                        entry = reader.ReadTagDataEntry(tag);
+                    }
+                    catch
+                    {
+                        // Ignore tags that could not be read
+                        continue;
+                    }
+
+                    store.Add(tag.Offset, entry);
                 }
 
-                entry.TagSignature = tagTable[i].Signature;
-                entries[i] = entry;
+                entry.TagSignature = tag.Signature;
+                entries.Add(entry);
             }
 
-            return entries;
+            return entries.ToArray();
         }
 
         private IccTagTableEntry[] ReadTagTable(IccDataReader reader)
         {
             reader.SetIndex(128);   // An ICC header is 128 bytes long
 
             uint tagCount = reader.ReadUInt32();
-            var table = new IccTagTableEntry[tagCount];
 
+            // Prevent creating huge arrays because of corrupt profiles.
+            // A normal profile usually has 5-15 entries
+            if (tagCount > 100)
+            {
+                return new IccTagTableEntry[0];
+            }
+
+            var table = new List<IccTagTableEntry>((int)tagCount);
             for (int i = 0; i < tagCount; i++)
             {
                 uint tagSignature = reader.ReadUInt32();
                 uint tagOffset = reader.ReadUInt32();
                 uint tagSize = reader.ReadUInt32();
-                table[i] = new IccTagTableEntry((IccProfileTag)tagSignature, tagOffset, tagSize);
+
+                // Exclude entries that have nonsense values and could cause exceptions further on
+                if (tagOffset < reader.DataLength && tagSize < reader.DataLength - 128)
+                {
+                    table.Add(new IccTagTableEntry((IccProfileTag)tagSignature, tagOffset, tagSize));
+                }
             }
 
-            return table;
+            return table.ToArray();
         }
     }
 }

tests/ImageSharp.Tests/MetaData/Profiles/ICC/IccProfileTests.cs

@@ -0,0 +1,49 @@
+// Copyright (c) Six Labors and contributors.
+// Licensed under the Apache License, Version 2.0.
+
+using System;
+using SixLabors.ImageSharp.MetaData.Profiles.Icc;
+using Xunit;
+
+namespace SixLabors.ImageSharp.Tests.Icc
+{
+    public class IccProfileTests
+    {
+
+#if !NETSTANDARD1_1
+
+        [Theory]
+        [MemberData(nameof(IccTestDataProfiles.ProfileIdTestData), MemberType = typeof(IccTestDataProfiles))]
+        public void CalculateHash_WithByteArray_CalculatesProfileHash(byte[] data, IccProfileId expected)
+        {
+            IccProfileId result = IccProfile.CalculateHash(data);
+
+            Assert.Equal(expected, result);
+        }
+
+        [Fact]
+        public void CalculateHash_WithByteArray_DoesNotModifyData()
+        {
+            byte[] data = IccTestDataProfiles.Profile_Random_Array;
+            byte[] copy = new byte[data.Length];
+            Buffer.BlockCopy(data, 0, copy, 0, data.Length);
+
+            IccProfileId result = IccProfile.CalculateHash(data);
+
+            Assert.Equal(data, copy);
+        }
+
+#endif
+
+        [Theory]
+        [MemberData(nameof(IccTestDataProfiles.ProfileValidityTestData), MemberType = typeof(IccTestDataProfiles))]
+        public void CheckIsValid_WithProfiles_ReturnsValidity(byte[] data, bool expected)
+        {
+            var profile = new IccProfile(data);
+
+            bool result = profile.CheckIsValid();
+
+            Assert.Equal(expected, result);
+        }
+    }
+}