Copilot commented on code in PR #3851: URL: https://github.com/apache/avro/pull/3851#discussion_r3564615849
########## lang/ruby/test/test_codec_limits.rb: ########## @@ -0,0 +1,90 @@ +# frozen_string_literal: true +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +require 'test_help' + +# A block with a very high compression ratio can expand to far more memory than +# its compressed size. These tests ensure that decompressing such a block raises +# an error instead of allocating without bound. +class TestCodecLimits < Test::Unit::TestCase + LIMIT = 1024 + + def with_limit(bytes) + previous = ENV['AVRO_MAX_DECOMPRESS_LENGTH'] + ENV['AVRO_MAX_DECOMPRESS_LENGTH'] = bytes.to_s + yield + ensure + if previous.nil? + ENV.delete('AVRO_MAX_DECOMPRESS_LENGTH') + else + ENV['AVRO_MAX_DECOMPRESS_LENGTH'] = previous + end + end + + def oversized_payload + ("\x00".b * (LIMIT * 8)) + end + + def test_deflate_rejects_oversized_block + codec = Avro::DataFile::DeflateCodec.new + compressed = codec.compress(oversized_payload) + with_limit(LIMIT) do + assert_raise(Avro::DataFile::DecompressionSizeError) do + codec.decompress(compressed) + end + end + end + + def test_deflate_allows_block_within_limit + codec = Avro::DataFile::DeflateCodec.new + payload = ('the quick brown fox ' * 10).b + compressed = codec.compress(payload) + with_limit(LIMIT) do + assert_equal payload, codec.decompress(compressed) + end + end + + def test_snappy_rejects_declared_oversized_length + begin + require 'snappy' + rescue LoadError + omit('snappy gem not available') + end + codec = Avro::DataFile::SnappyCodec.new + compressed = codec.compress(oversized_payload) + with_limit(LIMIT) do + assert_raise(Avro::DataFile::DecompressionSizeError) do + codec.decompress(compressed) + end + end + end + + def test_zstandard_rejects_oversized_block + begin + require 'zstd-ruby' + rescue LoadError + omit('zstd-ruby gem not available') + end + codec = Avro::DataFile::ZstandardCodec.new + compressed = codec.compress(oversized_payload) + with_limit(LIMIT) do + assert_raise(Avro::DataFile::DecompressionSizeError) do + codec.decompress(compressed) + end + end + end Review Comment: `ZstandardCodec#decompress` now uses streaming decompression, but the test only asserts over-limit rejection; it doesn't verify a successful round-trip within the limit. Adding a within-limit decode assertion will catch regressions where streaming decompression returns incomplete or incorrect output. ########## lang/ruby/lib/avro/data_file.rb: ########## @@ -374,11 +429,29 @@ def load_snappy! end class ZstandardCodec + # Size of the compressed input fed to the streaming decompressor per step, + # so the decompressed output can be bounded incrementally. + ZSTD_DECOMPRESS_CHUNK_SIZE = 16 * 1024 + def codec_name; 'zstandard'; end def decompress(data) load_zstandard! - Zstd.decompress(data) + limit = DataFile.max_decompress_length + stream = Zstd::StreamingDecompress.new + uncompressed = +''.b + offset = 0 + size = data.bytesize + # Decompress the block in chunks so an over-large (or malicious) block is + # rejected before its full decompressed form is materialized in memory. + while offset < size + uncompressed << stream.decompress(data.byteslice(offset, ZSTD_DECOMPRESS_CHUNK_SIZE)) + offset += ZSTD_DECOMPRESS_CHUNK_SIZE + if uncompressed.bytesize > limit + raise DecompressionSizeError, "Decompressed block size exceeds the maximum allowed of #{limit} bytes" + end Review Comment: The limit check is performed after appending the decompressed chunk to `uncompressed`, which can grow the buffer beyond the configured limit before raising. Checking the size before appending avoids expanding the output string past the limit and reduces peak memory usage during rejection. ########## lang/ruby/lib/avro/data_file.rb: ########## @@ -319,10 +340,22 @@ def decompress(compressed) # (without the RFC1950 header & checksum). See the docs for # inflateInit2 in https://www.zlib.net/manual.html zstream = Zlib::Inflate.new(-Zlib::MAX_WBITS) - data = zstream.inflate(compressed) - data << zstream.finish + limit = DataFile.max_decompress_length + data = "".b + # Inflate in chunks so an over-large (or malicious) block is rejected + # before its full decompressed form is materialized in memory. + append = lambda do |chunk| + data << chunk + if data.bytesize > limit + raise DecompressionSizeError, "Decompressed block size exceeds the maximum allowed of #{limit} bytes" + end + end Review Comment: The size check happens after appending the inflated chunk to `data`, which can temporarily materialize output beyond the configured limit (and allocate more memory than necessary) before raising. Check `data.bytesize + chunk.bytesize` before appending so over-limit blocks are rejected without growing the output buffer past the limit. -- 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]
