API changes for flate2 integration - #81
Conversation
| // (gzip footer, concatenated stream, or caller-owned trailing data). Partial bits are | ||
| // deflate/zlib padding and stay counted as consumed, but whole buffered bytes are | ||
| // reported back as unread so the caller can process them. | ||
| consumed = consumed.saturating_sub(self.bits.nbits as usize / 8); |
There was a problem hiding this comment.
The buffer management is quite subtle, but I don't think this is actually sufficient for the caller. The previous call to read may have returned early because it ran out of output space. If so, it might have filled the buffer (and consumed) up-to 7 bytes of input data that it didn't actually need. The saturating_sub will prevent an underflow here, but still won't un-consume the previous bytes.
But the alternative of decrementing consumed after every call to read also wouldn't work: some places in the decoder need to buffer multiple bytes of data to guarantee forward progress.
There was a problem hiding this comment.
This is pretty subtle and slipped under my radar.
Seeing how well GPT-5.5 reasoned through memory safety issues, I had it analyze this issue and write tests for it, then walk me through possible solutions. 9f9b0c3 is what we came up with.
I'm glad I got the tests first because they also exposed a checksum calculation issue, fixed in cc4436f
There was a problem hiding this comment.
We'll probably want to backport the checksum fix.
For 9f9b0c3, I think it breaks our forward progress guarantees when trying to write into an exactly large enough output buffer:
#[test]
fn forward_progress() {
let input = b"teststring";
let compressed = crate::compress_to_vec(input);
let mut decompressor = crate::Decompressor::new();
let mut output = vec![0u8; input.len()];
let mut input_index = 0;
let mut output_index = 0;
// Feed input one byte at a time and ensure the decompressor doesn't get stuck.
while !decompressor.is_done() {
let (input_consumed, output_written) = decompressor
.read(&compressed[input_index..][..1], &mut output, output_index)
.unwrap();
// Ensure that we made forward progress. (Only writing output would seem like progress,
// but actually signals a bug because it would mean that the previous iteration didn't
// return maximal output.)
assert!(input_consumed > 0);
input_index += input_consumed;
output_index += output_written;
}
}| self.format = format; | ||
| self.last_block = false; | ||
| self.ignore_adler32 = false; | ||
| self.fixed_table = false; |
There was a problem hiding this comment.
In a followup, we could probably avoid resetting self.fixed_table for a small performance boost when decoding lots of small deflate streams.
(This flag tracks whether the decoding tables and EOF codes are set to the values for a fixed Huffman block, which don't depend on the input)
fintelia
left a comment
There was a problem hiding this comment.
Might be worth spinning off the rewind functionality into a separate PR so we can land the rest and then think through all the edge cases in rewind without also having the other changes.
| && bit_buffer.nbits >= 15 | ||
| && bit_buffer.peek_bits(15) as u16 & self.eof_mask == self.eof_code |
There was a problem hiding this comment.
For Format::Raw we can no longer assume that there'll be 15 bits left in the input anytime we're looking for an EOF symbol. (In the zlib format there's 32-bits of checksum following any EOF so that isn't a concern.)
I think we need probably need to track the actual length of the EOF code and then replace 15 with self.eof_code_length
| } | ||
|
|
||
| fn update_checksum(&mut self, output: &[u8], start: usize, end: usize) { | ||
| if self.format == Format::Zlib && !self.ignore_adler32 && self.state != State::Done { |
There was a problem hiding this comment.
The self.format == Format::Zlib should now be unnecessary
This PR makes the necessary changes for integrating fdeflate as a
flate2backend: