Repository navigation
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @karelz, @dotnet/area-system-io-compression |
…myux/dotnet-runtime into 133018-brotli-decomp-options
iremyux
changed the title
iremyux
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Decoder validation can bypass the configured window ceiling, and header handling and argument validation have correctness regressions.
3 open findings
What changed in this PR
Adds configurable Brotli decompression window limits to support opt-in Large Window Brotli in the existing compression library.
Changes:
- Adds
BrotliDecompressionOptionsand decoder/stream overloads. - Extends encoding windows to 30 bits and validates compatible quality settings.
- Adds native decoder configuration bindings and window-limit tests.
Validation was limited to source inspection; no builds or tests were run.
| File | Description |
|---|---|
| src/native/libs/System.IO.Compression.Native/System.IO.Compression.Native.def | Exports decoder parameter configuration on Windows. |
| src/native/libs/System.IO.Compression.Native/System.IO.Compression.Native_unixexports.src | Exports decoder parameter configuration on Unix. |
| src/native/libs/System.IO.Compression.Native/entrypoints.c | Registers the decoder configuration entry point. |
| src/libraries/System.IO.Compression.Brotli/tests/CompressionStreamUnitTests.Brotli.cs | Adds window-limit and large-window tests. |
| src/libraries/System.IO.Compression.Brotli/tests/BrotliEncoderDecoderTests.cs | Updates the invalid window boundary. |
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliStream.Compress.cs | Validates large-window quality combinations. |
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliEncoderParameter.cs | Adds the large-window encoder parameter. |
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliEncoder.cs | Enables larger encoding windows. |
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliCompressionOptions.cs | Expands the supported window range. |
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/dec/BrotliStream.Decompress.cs | Adds the decompression-options constructor. |
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/dec/BrotliDecompressionOptions.cs | Defines configurable window limits. |
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/dec/BrotliDecoderParameter.cs | Defines native decoder parameters. |
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/dec/BrotliDecoder.cs | Configures decoding and validates declared windows. |
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliUtils.cs | Adds header parsing and quality validation. |
| src/libraries/System.IO.Compression.Brotli/src/System.IO.Compression.Brotli.csproj | Includes the new source files. |
| src/libraries/System.IO.Compression.Brotli/src/Resources/Strings.resx | Adds configuration error messages. |
| src/libraries/System.IO.Compression.Brotli/ref/System.IO.Compression.Brotli.cs | Declares the new public API surface. |
| src/libraries/Common/src/Interop/Interop.Brotli.cs | Adds decoder parameter interop. |
🧠 Review effort: Balanced
This was referenced Oct 15, 2026
Closed
iremyux
marked this pull request as ready for review
October 19, 2026 11:35
rzikm
reviewed
Oct 19, 2026
| /// </summary> | ||
| public sealed class BrotliDecompressionOptions | ||
| { | ||
| private int _maxWindowLog2 = BrotliUtils.WindowBits_Max; |
Member
There was a problem hiding this comment.
I assume this Max is the "safe" Max that is currently the default?
Maybe we should update the Max to be the large window Max and have a separate Default constant.
Contributor
Author
There was a problem hiding this comment.
yes, WindowBits_Max is the safe ceiling.
I added a separate BrotliUtils.WindowBits_DefaultLarge constant, we alredy have BrotliUtils.WindowBits_MaxLarge.
I wouldn't change WindowBits_Max to be the large max, so that nothing changes on the encoder side.
| default: | ||
| // A malformed header is reported by the native decoder, so that the error matches what callers that did not | ||
| // specify any decompression options would observe. | ||
| _windowValidated = true; |
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.
Adds opt-in support for decompressing Brotli streams with window sizes beyond default range, via a new
BrotliDecompressionOptionstype.Resolves #133018