diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticScanDecoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticScanDecoder.cs index 75f2c63713..0d5d0ad439 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticScanDecoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticScanDecoder.cs @@ -202,6 +202,9 @@ public ArithmeticScanDecoder(BufferedReadStream stream, SpectralConverter conver this.ct = -16; // Force reading 2 initial bytes to fill C. } + /// + public SegmentIntegrityHandling SegmentIntegrityHandling { get; set; } + /// public int ResetInterval { @@ -1100,9 +1103,10 @@ private bool HandleRestart() { if (this.restartInterval > 0 && (--this.todo) == 0) { - if (this.scanBuffer.Marker == JpegConstants.Markers.XFF) + // Use the same marker recovery and integrity policy as Huffman-coded scans. + if (this.scanBuffer.Marker == JpegConstants.Markers.XFF || this.scanBuffer.HasReservedMarker()) { - if (!this.scanBuffer.FindNextMarker()) + if (!this.scanBuffer.FindNextMarker(this.SegmentIntegrityHandling)) { return false; } @@ -1119,8 +1123,7 @@ private bool HandleRestart() component.AcStatistics?.Reset(); } - this.Reset(); - + // Reset clears the detected marker. Classify it first so a real segment marker can be read again. if (this.scanBuffer.HasRestartMarker()) { this.Reset(); diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs index 9ee43a2c83..2aa485ceab 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs @@ -85,6 +85,9 @@ public HuffmanScanDecoder( this.acHuffmanTables = new HuffmanTable[maxTables]; } + /// + public SegmentIntegrityHandling SegmentIntegrityHandling { get; set; } + /// /// Sets reset interval determined by RST markers. /// @@ -745,9 +748,10 @@ private bool HandleRestart() { if (this.restartInterval > 0 && (--this.todo) == 0) { - if (this.scanBuffer.Marker == JpegConstants.Markers.XFF) + // A reserved marker cannot complete a restart interval. The shared reader applies the recovery policy. + if (this.scanBuffer.Marker == JpegConstants.Markers.XFF || this.scanBuffer.HasReservedMarker()) { - if (!this.scanBuffer.FindNextMarker()) + if (!this.scanBuffer.FindNextMarker(this.SegmentIntegrityHandling)) { return false; } diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/IJpegScanDecoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/IJpegScanDecoder.cs index 588090deb6..1540885d97 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/IJpegScanDecoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/IJpegScanDecoder.cs @@ -10,6 +10,11 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder; /// internal interface IJpegScanDecoder { + /// + /// Sets the recovery policy for errors in compressed scan data. + /// + public SegmentIntegrityHandling SegmentIntegrityHandling { set; } + /// /// Sets the reset interval. /// diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index f888a8fb7b..c77e2e0762 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -84,6 +84,13 @@ public void Reset() [MethodImpl(InliningOptions.ShortMethod)] public readonly bool HasBadMarker() => this.Marker != JpegConstants.Markers.XFF && !this.HasRestartMarker(); + /// + /// Determines whether the detected marker is TEM or a reserved code before SOF0. + /// + /// True for TEM and reserved marker codes. + [MethodImpl(InliningOptions.ShortMethod)] + public readonly bool HasReservedMarker() => this.Marker < JpegConstants.Markers.SOF0; + [MethodImpl(InliningOptions.AlwaysInline)] public void FillBuffer() { @@ -177,39 +184,56 @@ private ulong GetBytes() return temp; } + /// + /// Finds the next marker at a restart boundary and applies the selected image-data recovery policy. + /// + /// The recovery policy for compressed image data. + /// True if a marker was found. False if the stream ended first. [MethodImpl(InliningOptions.ShortMethod)] - public bool FindNextMarker() + public bool FindNextMarker(SegmentIntegrityHandling integrityHandling) { - while (true) + do { - int b = this.stream.ReadByte(); - if (b == -1) + // The bit reader can already hold a marker from its entropy-data lookahead. + // Libjpeg treats codes before SOF0 as damaged image data and scans forward to another marker. + // Only IgnoreImageData permits that recovery. Real segment markers stay with the segment parser. + if (this.HasReservedMarker() && integrityHandling != SegmentIntegrityHandling.IgnoreImageData) { - return false; + JpegThrowHelper.ThrowInvalidImageContentException("Unexpected marker before a JPEG restart marker."); } - // Found a marker. - if (b == JpegConstants.Markers.XFF) + while (true) { - while (b == JpegConstants.Markers.XFF) + int b = this.stream.ReadByte(); + if (b == -1) { - // Loop here to discard any padding FF bytes on terminating marker. - b = this.stream.ReadByte(); - if (b == -1) - { - return false; - } + return false; } - // Found a valid marker. Exit loop - if (b != 0) + if (b == JpegConstants.Markers.XFF) { - this.Marker = (byte)b; - this.MarkerPosition = this.stream.Position - 2; - return true; + // Multiple FF bytes are marker padding. FF 00 is a stuffed data byte, not a marker. + while (b == JpegConstants.Markers.XFF) + { + b = this.stream.ReadByte(); + if (b == -1) + { + return false; + } + } + + if (b != 0) + { + this.Marker = (byte)b; + this.MarkerPosition = this.stream.Position - 2; + break; + } } } } + while (this.HasReservedMarker()); + + return true; } [MethodImpl(InliningOptions.AlwaysInline)] diff --git a/src/ImageSharp/Formats/Jpeg/JpegConstants.cs b/src/ImageSharp/Formats/Jpeg/JpegConstants.cs index 50b7de68e4..6a8eb07643 100644 --- a/src/ImageSharp/Formats/Jpeg/JpegConstants.cs +++ b/src/ImageSharp/Formats/Jpeg/JpegConstants.cs @@ -271,6 +271,14 @@ internal static class Markers /// /// public const byte RST7 = 0xD7; + + /// + /// Temporary private marker for arithmetic coding. + /// + /// + /// This marker stands alone. It has no length field or payload. + /// + public const byte TEM = 0x01; } /// diff --git a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs index 65b0f4d5d3..a5cc7d2ca5 100644 --- a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs @@ -188,8 +188,9 @@ public static JpegFileMarker FindNextFileMarker(BufferedReadStream stream) } } - // Found a valid marker. Exit loop - if (b is not 0 and (< JpegConstants.Markers.RST0 or > JpegConstants.Markers.RST7)) + // FF 00 is a stuffed data byte. RSTn and TEM have no length field (ITU T.81, Table B.1). + // Skip these standalone markers so the next marker's bytes are not read as a length. + if (b is not 0 and not JpegConstants.Markers.TEM and (< JpegConstants.Markers.RST0 or > JpegConstants.Markers.RST7)) { return new JpegFileMarker((byte)(uint)b, stream.Position - 2); } @@ -324,6 +325,9 @@ internal void ParseStream(BufferedReadStream stream, SpectralConverter spectralC this.scanDecoder ??= new HuffmanScanDecoder(stream, spectralConverter, cancellationToken); + // Apply the image-data policy to the active decoder, including one supplied by TIFF. + this.scanDecoder.SegmentIntegrityHandling = this.Options.SegmentIntegrityHandling; + this.Metadata ??= new ImageMetadata(); Span markerBuffer = stackalloc byte[2]; @@ -386,7 +390,11 @@ internal void ParseStream(BufferedReadStream stream, SpectralConverter spectralC case JpegConstants.Markers.SOF10: case JpegConstants.Markers.SOF13: case JpegConstants.Markers.SOF14: - this.scanDecoder = new ArithmeticScanDecoder(stream, spectralConverter, cancellationToken); + this.scanDecoder = new ArithmeticScanDecoder(stream, spectralConverter, cancellationToken) + { + SegmentIntegrityHandling = this.Options.SegmentIntegrityHandling + }; + if (this.resetInterval.HasValue) { this.scanDecoder.ResetInterval = this.resetInterval.Value; diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs index a1f5f74be9..ee5cec5036 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs @@ -9,6 +9,7 @@ using SixLabors.ImageSharp.Tests.Formats.Jpg.Utils; using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison; using SixLabors.ImageSharp.Tests.TestUtilities.ReferenceCodecs; +using static SixLabors.ImageSharp.Tests.TestImages.Jpeg; // ReSharper disable InconsistentNaming namespace SixLabors.ImageSharp.Tests.Formats.Jpg; @@ -299,6 +300,99 @@ public void Issue2136_DecodeWorks(TestImageProvider provider) image.CompareToOriginal(provider); } + /// + /// Checks restart recovery against clean stored files whose pixels also match in libjpeg. + /// + /// The clean JPEG input. + /// The input with extra markers before restart markers. + [Theory] + [InlineData(MarkerDirectory + "baseline.jpg", MarkerDirectory + "baseline-reserved-12.jpg")] + [InlineData(MarkerDirectory + "baseline.jpg", MarkerDirectory + "baseline-tem.jpg")] + [InlineData(MarkerDirectory + "baseline.jpg", MarkerDirectory + "baseline-reserved-bf.jpg")] + [InlineData(MarkerDirectory + "baseline.jpg", MarkerDirectory + "baseline-reserved-chain.jpg")] + [InlineData(MarkerDirectory + "noninterleaved.jpg", MarkerDirectory + "noninterleaved-reserved-12.jpg")] + [InlineData(MarkerDirectory + "progressive.jpg", MarkerDirectory + "progressive-reserved-12.jpg")] + [InlineData(MarkerDirectory + "arithmetic.jpg", MarkerDirectory + "arithmetic-reserved-12.jpg")] + [InlineData(MarkerDirectory + "arithmetic-progressive.jpg", MarkerDirectory + "arithmetic-progressive-reserved-12.jpg")] + public void Decode_ReservedMarkerBeforeRestartMarkers_RecoversWithIgnoreImageData(string originalFile, string malformedFile) + { + // These stored files have complete restart intervals. Libjpeg warns about the extra markers, + // then finds the restart marker and produces the same pixels as the clean file. + // The arithmetic inputs declare their conditioning tables to isolate restart recovery. + DecoderOptions options = new() { SegmentIntegrityHandling = SegmentIntegrityHandling.IgnoreImageData }; + using Image expected = Image.Load(TestFile.Create(originalFile).Bytes); + using Image actual = Image.Load(options, TestFile.Create(malformedFile).Bytes); + + ImageComparer.Exact.VerifySimilarity(expected, actual); + } + + /// + /// Rejects image-data errors when the selected policy permits only ancillary recovery or no recovery. + /// + /// The input with reserved markers before restart markers. + [Theory] + [InlineData(MarkerDirectory + "baseline-reserved-12.jpg")] + [InlineData(MarkerDirectory + "noninterleaved-reserved-12.jpg")] + [InlineData(MarkerDirectory + "progressive-reserved-12.jpg")] + [InlineData(MarkerDirectory + "arithmetic-reserved-12.jpg")] + [InlineData(MarkerDirectory + "arithmetic-progressive-reserved-12.jpg")] + public void Decode_ReservedMarkerBeforeRestartMarkers_RespectsIntegrity(string fileName) + { + byte[] data = TestFile.Create(fileName).Bytes; + DecoderOptions strict = new() { SegmentIntegrityHandling = SegmentIntegrityHandling.Strict }; + DecoderOptions ancillary = new() { SegmentIntegrityHandling = SegmentIntegrityHandling.IgnoreAncillary }; + + Assert.Throws(() => + { + using Image image = Image.Load(strict, data); + }); + + Assert.Throws(() => + { + using Image image = Image.Load(ancillary, data); + }); + } + + /// + /// Accepts a standalone TEM marker between header segments under every integrity policy. + /// + [Fact] + public void Decode_TemMarkerBetweenHeaderSegments_IsSkipped() + { + byte[] data = TestFile.Create(MarkerDirectory + "header-tem.jpg").Bytes; + using Image expected = Image.Load(TestFile.Create(MarkerDirectory + "baseline.jpg").Bytes); + + // TEM has no length field. The following bytes belong to the next marker, not to TEM. + foreach (SegmentIntegrityHandling integrity in Enum.GetValues()) + { + DecoderOptions options = new() { SegmentIntegrityHandling = integrity }; + Assert.Equal(new Size(32, 24), Image.Identify(options, data).Size); + + using Image actual = Image.Load(options, data); + ImageComparer.Exact.VerifySimilarity(expected, actual); + } + } + + /// + /// Keeps required header lengths valid after a standalone TEM marker. + /// + [Fact] + public void Decode_InvalidQuantizationSegmentAfterTem_RemainsFatal() + { + // TEM precedes DQT in this stored file. DQT declares more bytes than the file contains. + // No image data has been read, so there is no safe point for recovery. + byte[] data = TestFile.Create(MarkerDirectory + "header-tem-invalid-dqt-length.jpg").Bytes; + + foreach (SegmentIntegrityHandling integrity in Enum.GetValues()) + { + DecoderOptions options = new() { SegmentIntegrityHandling = integrity }; + Assert.Throws(() => + { + using Image image = Image.Load(options, data); + }); + } + } + // https://github.com/SixLabors/ImageSharp/issues/2315 // https://github.com/SixLabors/ImageSharp/issues/2334 [Theory] diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 13e31e7079..6966a32a78 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -264,6 +264,11 @@ public static class Bad public static class Jpeg { + /// + /// Stored JPEG marker and restart-recovery inputs. + /// + public const string MarkerDirectory = "Jpg/markers/"; + public static class ICC { public const string SRgb = "Jpg/icc-profiles/Momiji-sRGB-yes.jpg"; diff --git a/tests/Images/Input/Jpg/markers/arithmetic-progressive-reserved-12.jpg b/tests/Images/Input/Jpg/markers/arithmetic-progressive-reserved-12.jpg new file mode 100644 index 0000000000..49f770afd6 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/arithmetic-progressive-reserved-12.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:66841d2705f2bd9ccdb0e77a7f71965507313556452d4e3fb26e32853e85e4ee +size 778 diff --git a/tests/Images/Input/Jpg/markers/arithmetic-progressive.jpg b/tests/Images/Input/Jpg/markers/arithmetic-progressive.jpg new file mode 100644 index 0000000000..54820d9fd8 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/arithmetic-progressive.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:fe8367c26bce4840a14d41c3a0f64c562722e5caf751bba4082d0c2839482d11 +size 654 diff --git a/tests/Images/Input/Jpg/markers/arithmetic-reserved-12.jpg b/tests/Images/Input/Jpg/markers/arithmetic-reserved-12.jpg new file mode 100644 index 0000000000..02ff57b0eb --- /dev/null +++ b/tests/Images/Input/Jpg/markers/arithmetic-reserved-12.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:0f40aa4188c0c094bda8c2d02fb243700239eb492886fc03616eb37b27dd87af +size 344 diff --git a/tests/Images/Input/Jpg/markers/arithmetic.jpg b/tests/Images/Input/Jpg/markers/arithmetic.jpg new file mode 100644 index 0000000000..437ebe7608 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/arithmetic.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:412a2af2c4005d4526eae74d1270b935fdb816623414c2cec9009b2a1fe16b31 +size 338 diff --git a/tests/Images/Input/Jpg/markers/baseline-reserved-12.jpg b/tests/Images/Input/Jpg/markers/baseline-reserved-12.jpg new file mode 100644 index 0000000000..8ceeefe4ff --- /dev/null +++ b/tests/Images/Input/Jpg/markers/baseline-reserved-12.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:71da350023e239ccfb89e800c856db8880469c7fedae8e35b82327a9d27ffbd8 +size 775 diff --git a/tests/Images/Input/Jpg/markers/baseline-reserved-bf.jpg b/tests/Images/Input/Jpg/markers/baseline-reserved-bf.jpg new file mode 100644 index 0000000000..d099466872 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/baseline-reserved-bf.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:68949614b2d283f6d086d5559e775abb2c409da605bffaac0cedc7ac81479dcb +size 775 diff --git a/tests/Images/Input/Jpg/markers/baseline-reserved-chain.jpg b/tests/Images/Input/Jpg/markers/baseline-reserved-chain.jpg new file mode 100644 index 0000000000..619695d781 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/baseline-reserved-chain.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:60ed63792bcf73bfcc2c87330fd178bd5a39d7b675d8bbc895f53d186859b3a6 +size 781 diff --git a/tests/Images/Input/Jpg/markers/baseline-tem.jpg b/tests/Images/Input/Jpg/markers/baseline-tem.jpg new file mode 100644 index 0000000000..9b55cbd663 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/baseline-tem.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:6499a4154e3a93523dd77af5e719b3d144e92485fa37ba02e95bad86f402b81b +size 775 diff --git a/tests/Images/Input/Jpg/markers/baseline.jpg b/tests/Images/Input/Jpg/markers/baseline.jpg new file mode 100644 index 0000000000..792cd12205 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/baseline.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:576c8e79709961c68f49ff3cde01d8b1ba5c1326bdd36fc1b487cdb5df4533ca +size 769 diff --git a/tests/Images/Input/Jpg/markers/header-tem-invalid-dqt-length.jpg b/tests/Images/Input/Jpg/markers/header-tem-invalid-dqt-length.jpg new file mode 100644 index 0000000000..124ee59ddc --- /dev/null +++ b/tests/Images/Input/Jpg/markers/header-tem-invalid-dqt-length.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:ceada1c48585463c4293dd33c35476d970df41673a9ea9ba5aa11026fd64282d +size 771 diff --git a/tests/Images/Input/Jpg/markers/header-tem.jpg b/tests/Images/Input/Jpg/markers/header-tem.jpg new file mode 100644 index 0000000000..e00564d2d5 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/header-tem.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:d9f4fdb4b11c9556c3a531a64e7d4e8db88b2a2c169cf877561b9c060ee69279 +size 771 diff --git a/tests/Images/Input/Jpg/markers/noninterleaved-reserved-12.jpg b/tests/Images/Input/Jpg/markers/noninterleaved-reserved-12.jpg new file mode 100644 index 0000000000..9959107fb4 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/noninterleaved-reserved-12.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:1e426b63bbb8d96511877087a367296c0cb867b7578cc896c1c508e6feb0242c +size 850 diff --git a/tests/Images/Input/Jpg/markers/noninterleaved.jpg b/tests/Images/Input/Jpg/markers/noninterleaved.jpg new file mode 100644 index 0000000000..a9598b2281 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/noninterleaved.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:0881301e40bd562f501a6296f7b9843779b0df6f59d2f2b2a789d634c169f835 +size 816 diff --git a/tests/Images/Input/Jpg/markers/progressive-reserved-12.jpg b/tests/Images/Input/Jpg/markers/progressive-reserved-12.jpg new file mode 100644 index 0000000000..2ffbc03290 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/progressive-reserved-12.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:fb95b8d19e7e3f51b446b6b801e2f21902dcf4d442a7002b16169c93a051b19e +size 903 diff --git a/tests/Images/Input/Jpg/markers/progressive.jpg b/tests/Images/Input/Jpg/markers/progressive.jpg new file mode 100644 index 0000000000..1eefde2d33 --- /dev/null +++ b/tests/Images/Input/Jpg/markers/progressive.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:c4433ae4df863cf8cc0d5751e34cd64cf33e180551830e5fd1b1b1918064ed11 +size 779