From e408e411b8fe694da41448d9d4fd69dfe655df2f Mon Sep 17 00:00:00 2001 From: Jason Finch Date: Sun, 18 Oct 2026 09:21:21 +1000 Subject: [PATCH] Skip stray TEM markers and reserved markers before restart markers when decoding jpegs A TEM marker (FF 01) between header segments was read as a marker with a length field, so the next segment header was consumed as its length and decoding failed. TEM stands alone like RSTn, so skip it the same way. Some producers write extraneous bytes before restart markers. Those that form a TEM or reserved marker (FF 01 to FF BF) were taken as the end of the scan, which corrupted the rest of the image or made the marker parser fail. Skip them while looking for the marker that ends a restart interval and resync at the next marker, as libjpeg does. --- .../Components/Decoder/HuffmanScanDecoder.cs | 6 +- .../Jpeg/Components/Decoder/JpegBitReader.cs | 7 + src/ImageSharp/Formats/Jpeg/JpegConstants.cs | 9 + .../Formats/Jpeg/JpegDecoderCore.cs | 6 +- .../Formats/Jpg/JpegDecoderTests.Malformed.cs | 165 ++++++++++++++++++ 5 files changed, 190 insertions(+), 3 deletions(-) create mode 100644 tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Malformed.cs 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; + } +}