Copilot commented on code in PR #3857:
URL: https://github.com/apache/avro/pull/3857#discussion_r3564677925
##########
lang/csharp/src/apache/main/File/DataFileReader.cs:
##########
@@ -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());
Review Comment:
`HasNext()` catches all exceptions and rethrows a new `AvroRuntimeException`
without preserving the original exception as `InnerException`. With the new
decompression-limit check, this discards the original stack trace and makes it
harder for callers to distinguish a decompression-bomb rejection from other
failures.
##########
lang/csharp/src/apache/main/File/DataFileReader.cs:
##########
@@ -233,7 +233,7 @@ public string GetMetaString(string key)
catch (Exception e)
{
throw new
AvroRuntimeException(string.Format(CultureInfo.InvariantCulture,
- "Error fetching meta data for key: {0}", key), e);
+ "Error fetching next object from block: {0}", e.Message),
e);
}
Review Comment:
`GetMetaString` wraps UTF-8 decoding errors with an exception message about
"next object from block", which is unrelated to metadata and also drops the
metadata key from the message. This will mislead callers debugging malformed
metadata.
##########
lang/csharp/src/apache/main/File/DataFileReader.cs:
##########
@@ -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());
Review Comment:
The new `Codec.CheckDecompressLength(...)` safeguard in `HasNext()` isn’t
exercised by tests. Current additions only cover calling
`DeflateCodec.Decompress` directly, but not the `DataFileReader` path that is
meant to protect *all* codecs that return a fully materialized buffer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
