Maybe a bug in isal_inflate_stateless()

[axelriet]

The loop at 2179 in isal_inflate_stateless() exits when decode_literal_block() returns 1 (end of input), which causes the CRC check to be skipped and the function to return 1 despite the decoding going well.

I think that loop runs one too many times. My compressed buffer is 515 bytes, and the uncompressed data is 1024 bytes.

The initial call to decode_huffman_code_block_stateless() consumes the entire input and emits the entire output and returns 0.

avail_in = 0 and total_out = 1024 after decode_huffman_code_block_stateless() returns, but block_state is not set to ISAL_BLOCK_INPUT_DONE (it's left to ISAL_BLOCK_NEW_HDR) therefore the loop loops and calls read_hdr() which succeeds and ends up calling decode_literal_block() as block_state is now ISAL_BLOCK_TYPE0.

The loop must exit after decode_huffman_code_block_stateless() has consumed all the input bytes. Perhaps that function should set the block state to ISAL_BLOCK_INPUT_DONE?

As a quickfix, I changed lines 2192-93 from

            if (ret)
                    break;

to

            if (ret || !state->avail_in)
                    break;

where the reasoning is "if stateless (implicit in this function) and no more input" -> done.

[pablodelara]

Is this a GZIP file after being compressed? Did you use any auxiliary functions like isal_read_gzip_header, before calling isal_inflate_stateless? Thanks!

[axelriet]

Hello,

There is no Gzip and no file.

The bug is that decode_huffman_block_stateless() should set block_state to ISAL_BLOCK_INPUT_DONE unconditionally before returning since it is stateless, therefore done by definition.

My quickfix is symptomatic but ultimately decode_huffman_block_stateless() should probably be fixed to report the block_state as done.

[pablodelara]

I was asking it because you mentioned CRC, which is only checked if this is a GZIP file. Anyway, the compressed file can have multiple DEFLATE blocks, so decode_huffman_code_block_stateless shouldn't set block_state to ISAL_BLOCK_INPUT_DONE unless it is the last block (which is set by the BFINAL bit in the header). So it's likely you are encountering a situation where the only block in that message doesn't have that bit set...

[pablodelara]

Which, by the way, would be wrong from the compressor. But it could still be decompressed, adding a check like the one you are proposing.

[pablodelara]

I have forced that situation setting bfinal = 0 and I can see that issue, but your fix wouldn't be enough, as I had some extra bytes in next_in (avail_in > 0 and read_in_length > 0), since there were some gzip trailer at the end. Therefore the safest thing to do is to leave it as is, I think, since this is a malformed DEFLATE block (unless this is not your situation with BFINAL).

[axelriet]

The block is not malformed. It comes straight from deflate stateless and I inflate it the line after.

What I read is there is a deeper issue where some assumption is made about GZIP data but again there is no GZIP header or trailer anywhere. I'm not using the GZIP format.

I believe the issue must be fixed. Now the code attempts to read past the end while it already has processed all the bytes.

The GZIP (and possibly other) format(s) that include additional data must be treated separately but the base case without header must work and as it stands it is not correct.

My quick fix should not interfere in case there are unconsumed bytes and the loop should continue and read the GZIP trailer when there is one, but there is no point in trying when there is none.

I think it works by accident and is actually a bug since it skips the CRC check in the current code.

That is definitely a bug.

[axelriet]

Oh and it returns an error on success, too. This is how I found it.

[pablodelara]

Thanks for the extra details. In fairness, having GZIP header/trailer or not is not that relevant. What I don't understand is how decode_huffman_block_stateless sets block_state to ISAL_BLOCK_NEW_HDR if the bfinal bit is set to 1 (a correct DEFLATE block must have that bit set). Could you add some steps to reproduce this? Thanks!

[axelriet]

The steps are litterally:

1: isal_deflate_stateless(Stream)
2: isal_inflate_stateless(State)

I'm not using GZIP, there is no GZIP header or trailer involved at all.

The only flag I use is FULL_FLUSH which is required for stateless operations.

The compression mode is IGZIP_DEFLATE (0, the default) not IGZIP_GZIP or IGZIP_ZLIB.

When using IGZIP_DEFLATE there is no GZIP or ZLIB header or trailer, the loop must exit after the decompression stages, not try to read a trailer that isn't there and err because it's not found.

[pablodelara]

Thanks for the further explanation. Sounds like a very common case of compressing/decompressing a raw DEFLATE block, which should always return ISAL_BLOCK_INPUT_DONE if that block is final. Could you share which buffer/file are you compressing? In the isal_inflate_stateless function, state->bfinal should be set to 1, after reading the header in line 2181. Can you check that's true for you? Thanks!

[pablodelara]

Is it possible you are not setting "stream->end_of_stream = 1" when compressing? This would explain what you are seeing (no BFINAL would be set).