-
Notifications
You must be signed in to change notification settings - Fork 1.8k
AVRO-4286: [csharp] Enforce a maximum decompressed block size #3857
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5b63084
15737e5
f8c8e1d
7a05ac1
f17a90e
51bf9f8
76e7459
55d0e4f
968ac0c
6bea53e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -335,6 +335,11 @@ public bool HasNext() | |
| { | ||
| _currentBlock = NextRawBlock(_currentBlock); | ||
| _currentBlock.Data = _codec.Decompress(_currentBlock.Data, (int)_blockSize); | ||
| // Guard against a block that decompresses to more than the | ||
| // allowed maximum (a decompression bomb). The built-in deflate | ||
| // codec is already bounded during decompression; this covers | ||
| // any codec that returns a fully decompressed buffer. | ||
| Codec.CheckDecompressLength(_currentBlock.Data.Length, Codec.GetMaxDecompressLength()); | ||
|
Comment on lines
337
to
+342
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed.
Comment on lines
337
to
+342
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. The inner-exception preservation is now applied to HasNext() (the intended target): it passes the original exception as InnerException and uses e.Message in the wrapper text. Pushed in 7a05ac1.
Comment on lines
+338
to
+342
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Added TestReaderRejectsOversizedBlock, which writes a Null-codec block larger than the limit and asserts the DataFileReader read path (HasNext -> CheckDecompressLength) rejects it, covering codecs that return a fully materialized buffer. Pushed in 7a05ac1. |
||
| _datumDecoder = new BinaryDecoder(_currentBlock.GetDataAsStream()); | ||
| } | ||
| } | ||
|
|
@@ -343,7 +348,7 @@ public bool HasNext() | |
| catch (Exception e) | ||
| { | ||
| throw new AvroRuntimeException(string.Format(CultureInfo.InvariantCulture, | ||
| "Error fetching next object from block: {0}", e)); | ||
| "Error fetching next object from block: {0}", e.Message), e); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one, on the other hand, is less likely to be changed in the future, so
constseems OK.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed — left
MaxDecompressLengthEnvVarasconst; it's a stable string identifier that referencing assemblies inlining is fine for.