Repository navigation
Make the WebP decoder tolerate reserved VP8X bits, trailing data and odd-length metadata chunks - #3233
Open
jafin wants to merge 1 commit into
Open
Make the WebP decoder tolerate reserved VP8X bits, trailing data and odd-length metadata chunks#3233jafin wants to merge 1 commit into
jafin wants to merge 1 commit into
Conversation
…odd-length metadata chunks Readers must ignore the reserved VP8X flag bits, so they are no longer validated. Chunks are now read only up to the end of the RIFF payload, so bytes after it are ignored rather than parsed as more chunks, and unknown chunks larger than the payload end the parsing instead of becoming a negative skip. A stream that ends inside a chunk header, a 24 bit field or the VP8L header now throws InvalidImageContentException. Metadata chunks with an odd payload length are read without the RIFF padding byte, which is skipped before the next chunk.
4 tasks done
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prerequisites
Description
This makes the WebP decoder accept files that libwebp and browsers open, and makes it read odd-length metadata chunks exactly as they were written.
Reserved VP8X flag bits were rejected
The decoder threw
ImageFormatException("first two bits of the VP8X header are expected to be zero") when either of the top two reserved bits of the VP8X flags byte was set. The container specification says readers must ignore reserved fields. The reserved bytes after the flags byte were already ignored. I removed the check, so a file with non-zero reserved bits now decodes to the same image as the unmodified file.Bytes after the RIFF payload broke decoding
The chunk loops in
ReadVp8InfoandParseOptionalChunksran to the end of the stream instead of the end of the RIFF payload. For an extended (VP8X) file, 1 to 3 trailing bytes threw "could not read chunk type". Longer trailing data was parsed as more chunks: a valid image chunk there would replace the image, and anImageFormatExceptionthrown this way also left the image bit reader's buffers undisposed. One of the existing test files,Issue2925.webp, has 80 KB of zero bytes after its RIFF payload.The decoder now records where the RIFF payload ends and reads chunks only up to that point, as libwebp does. If the declared size is larger than the stream (a truncated file), it reads up to the end of the stream instead. The loops also stop when fewer bytes than a chunk header remain. Unknown chunks were skipped with
stream.Skip((int)chunkSize), so a size aboveint.MaxValuebecame a negative skip and the decoder then read the chunk's payload as chunk headers. These skips are now clamped to the end of the RIFF payload.Truncated container fields threw
ImageFormatExceptionWhen the stream ended inside a chunk type, a chunk size or a 24-bit VP8X/ANMF field, the decoder threw a plain
ImageFormatException. It now throwsInvalidImageContentException, which derives fromImageFormatException. A VP8L chunk cut off before its 5-byte header was read from zero-filled memory and reported as "Invalid VP8L signature". It is now reported as missing data.Truncation inside the compressed image data itself is unchanged: the bitstream decoders still report it as they do today. The existing
WebpDecoder_ThrowImageFormatException_OnInvalidImagestest pins that behaviour for a truncated lossless file.Odd-length metadata chunks kept their padding byte
RIFF follows an odd-sized payload with one padding byte. The metadata readers used the padded size as the payload length, so an odd-length ICCP payload came back with an extra trailing
0x00, and saving the image wrote that byte back as part of the profile. EXIF and XMP chunks had the same defect. It was hidden in practice because the EXIF profile is re-serialised and XMP trailing NULs are trimmed.ReadIccProfile,ReadExifProfileandReadXmpProfilenow read the stored payload size, andReadMetadataChunkskips the padding byte after the payload. This keeps the next chunk aligned and does not change even-length chunks orSkipMetadata.Test fixture correction
Decode_WithMetadataChunkLargerThanRemainingData_ThrowsInStrictModeused a fixture with a RIFF size of0. With the RIFF bound, that fixture would fail early with "Missing WebP image data" and never reach the metadata path the test is for. I set the fixture's RIFF size to its real payload length (34), so the test exercises the oversized-metadata path again. Its assertions are unchanged.Already handled on
mainEXIF and XMP chunks that declare sizes up to
0xFFFFFFFFare already bounded by the remaining stream before any allocation. They are skipped, or rejected in strict mode, and an oversized ICCP chunk throwsInvalidImageContentException. The existingDecode_TruncatedTrailingMetadata_RespectsOptionsandDecode_WithOversizedMetadataChunk_*tests cover this, so I added no tests for it.Tests
All new tests build their inputs in memory by encoding a small image and changing the bytes. No new image files are added.
Decode_Vp8XWithNonZeroReservedFields_MatchesOriginal: reserved flag bits and reserved bytes.Decode_WithBytesAfterRiffPayload_MatchesOriginal: 1 to 64 random trailing bytes on simple lossy, simple lossless and VP8X files with alpha, EXIF, EXIF + XMP and ICC.Decode_WithImageChunkAfterRiffPayload_IgnoresIt: a complete VP8L chunk after the payload.Decode_UnknownChunkLargerThanRiffPayload_MatchesOriginalDecode_TruncatedContainer_DecodesOrThrowsInvalidImageContentException: every truncation length, for both Load and Identify.Decode_OddLengthMetadataChunks_ReadsPayloadsWithoutPaddingByte: odd ICCP, EXIF and XMP chunks, with and withoutSkipMetadata.All of these fail on
main, except the variants that already worked there. They pass with this change. The decoder tests run under[ValidateDisposedMemoryAllocations]. All WebP tests pass in Release on net10.0.Fixes #3245