diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs index 9ee43a2c83..41b4552c1b 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs @@ -745,7 +745,11 @@ private bool HandleRestart() { if (this.restartInterval > 0 && (--this.todo) == 0) { - if (this.scanBuffer.Marker == JpegConstants.Markers.XFF) + // Find the marker that ends this interval. FindNextMarker skips the extraneous data bytes some + // producers write before it. Those bytes can also form a TEM or reserved marker (FF 01 to FF BF) + // which cannot end an interval, so skip those too and resync at the next marker, as libjpeg does. + // Treating such a marker as the end of the scan would lose the rest of the scan data. + while (this.scanBuffer.Marker == JpegConstants.Markers.XFF || this.scanBuffer.HasReservedMarker()) { if (!this.scanBuffer.FindNextMarker()) { diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index f888a8fb7b..c3fbdda22c 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(); + /// + /// Whether a TEM or reserved marker has been detected, I.E. One below SOF0. + /// Such a marker can neither end a restart interval nor the scan. + /// + [MethodImpl(InliningOptions.ShortMethod)] + public readonly bool HasReservedMarker() => this.Marker < JpegConstants.Markers.SOF0; + [MethodImpl(InliningOptions.AlwaysInline)] public void FillBuffer() { diff --git a/src/ImageSharp/Formats/Jpeg/JpegConstants.cs b/src/ImageSharp/Formats/Jpeg/JpegConstants.cs index 50b7de68e4..9e34f1130c 100644 --- a/src/ImageSharp/Formats/Jpeg/JpegConstants.cs +++ b/src/ImageSharp/Formats/Jpeg/JpegConstants.cs @@ -271,6 +271,15 @@ internal static class Markers /// /// public const byte RST7 = 0xD7; + + /// + /// Temporary private use in arithmetic coding + /// + /// Like SOI, EOI and RSTn, this marker stands alone and is not followed by a length field. + /// It is the lowest marker code, every code between it and is reserved. + /// + /// + public const byte TEM = 0x01; } /// diff --git a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs index 65b0f4d5d3..af1d9e6e9d 100644 --- a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs @@ -188,8 +188,10 @@ 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)) + // Found a valid marker. Exit loop. + // FF 00 is a stuffed data byte. RSTn and TEM stand alone without a length field (ITU T.81 B.1.1.4), + // so stray ones between marker segments carry no data and are skipped. + 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); } diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Malformed.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Malformed.cs new file mode 100644 index 0000000000..e0871ea164 --- /dev/null +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Malformed.cs @@ -0,0 +1,165 @@ +// Copyright (c) Six Labors. +// Licensed under the Six Labors Split License. + +using SixLabors.ImageSharp.Formats.Jpeg; +using SixLabors.ImageSharp.PixelFormats; +using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison; + +// ReSharper disable InconsistentNaming +namespace SixLabors.ImageSharp.Tests.Formats.Jpg; + +/// +/// Tests for streams written by imperfect producers which libjpeg based decoders still decode. +/// +[Trait("Format", "Jpg")] +public partial class JpegDecoderTests +{ + /// + /// Gets the encoder layouts paired with the junk inserted before every restart marker. + /// The junk forms a TEM or reserved marker (FF 01 to FF BF) which cannot end a restart interval. + /// + public static TheoryData ReservedMarkerBeforeRestartData { get; } = new() + { + // Baseline interleaved. + { false, true, [0xFF, 0x12] }, + { false, true, [0xFF, 0x01] }, + { false, true, [0xFF, 0xBF] }, + { false, true, [0xFF, 0x12, 0xFF, 0x34] }, + + // Baseline non-interleaved. + { false, false, [0xFF, 0x12] }, + + // Progressive. + { true, false, [0xFF, 0x12] }, + }; + + [Theory] + [MemberData(nameof(ReservedMarkerBeforeRestartData))] + public void Decode_ReservedMarkerBeforeRestartMarkers_IsSkipped(bool progressive, bool interleaved, byte[] junk) + { + byte[] original = EncodeGradientJpeg(new JpegEncoder + { + ColorType = JpegColorType.YCbCrRatio420, + Progressive = progressive, + Interleaved = interleaved, + RestartInterval = 1 + }); + + byte[] corrupt = InsertBeforeRestartMarkers(original, junk); + + using Image expected = Image.Load(original); + using Image actual = Image.Load(corrupt); + ImageComparer.Exact.VerifySimilarity(expected, actual); + } + + [Fact] + public void Decode_TemMarkerBetweenHeaderSegments_IsSkipped() + { + byte[] original = EncodeGradientJpeg(new JpegEncoder { ColorType = JpegColorType.YCbCrRatio420 }); + + // TEM has no length field. Insert one straight after the APP0 segment which directly follows SOI. + byte[] corrupt = Insert(original, SegmentEnd(original, 2), JpegConstants.Markers.XFF, JpegConstants.Markers.TEM); + + ImageInfo info = Image.Identify(corrupt); + Assert.Equal(new Size(32, 24), info.Size); + + using Image expected = Image.Load(original); + using Image actual = Image.Load(corrupt); + ImageComparer.Exact.VerifySimilarity(expected, actual); + } + + /// + /// Encodes a 32x24 gradient, which spans several MCUs in every layout, with the given encoder. + /// + /// The encoder. + /// The encoded jpeg. + private static byte[] EncodeGradientJpeg(JpegEncoder encoder) + { + using Image image = new(32, 24); + for (int y = 0; y < image.Height; y++) + { + for (int x = 0; x < image.Width; x++) + { + image[x, y] = new Rgba32((byte)(x * 8), (byte)(y * 10), (byte)((x + y) * 4)); + } + } + + using MemoryStream ms = new(); + image.SaveAsJpeg(ms, encoder); + return ms.ToArray(); + } + + /// + /// Inserts the junk bytes before every RSTn marker in the entropy-coded data following the first SOS. + /// + /// The jpeg. + /// The bytes to insert. + /// The corrupted jpeg. + private static byte[] InsertBeforeRestartMarkers(byte[] data, byte[] junk) + { + int start = SegmentEnd(data, FindSegment(data, JpegConstants.Markers.SOS)); + using MemoryStream ms = new(); + ms.Write(data, 0, start); + + int count = 0; + for (int i = start; i < data.Length; i++) + { + if (data[i] == JpegConstants.Markers.XFF && i + 1 < data.Length && data[i + 1] is >= JpegConstants.Markers.RST0 and <= JpegConstants.Markers.RST7) + { + ms.Write(junk, 0, junk.Length); + count++; + } + + ms.WriteByte(data[i]); + } + + Assert.True(count > 0, "The test image has no restart markers."); + return ms.ToArray(); + } + + /// + /// Walks the marker segments following SOI up to the first SOS. + /// + /// The jpeg. + /// The marker code to find. + /// The offset of the first segment with the given marker code. + private static int FindSegment(byte[] data, byte marker) + { + int offset = 2; + while (data[offset + 1] != marker) + { + Assert.NotEqual(JpegConstants.Markers.SOS, data[offset + 1]); + offset = SegmentEnd(data, offset); + } + + return offset; + } + + /// + /// Gets the offset just past the marker segment (FF xx and its length field) starting at the given offset. + /// + /// The jpeg. + /// The offset of the segment. + /// The offset of the following segment. + private static int SegmentEnd(byte[] data, int offset) + { + Assert.Equal(JpegConstants.Markers.XFF, data[offset]); + return offset + 2 + ((data[offset + 2] << 8) | data[offset + 3]); + } + + /// + /// Inserts bytes at the given offset. + /// + /// The source data. + /// The offset to insert at. + /// The bytes to insert. + /// The new data. + private static byte[] Insert(byte[] data, int offset, params byte[] bytes) + { + byte[] result = new byte[data.Length + bytes.Length]; + Buffer.BlockCopy(data, 0, result, 0, offset); + Buffer.BlockCopy(bytes, 0, result, offset, bytes.Length); + Buffer.BlockCopy(data, offset, result, offset + bytes.Length, data.Length - offset); + return result; + } +}