Use zlib decode to properly use window size and checksum in flate filter

[rhuijben]

Use the zlib data to verify that the compressed data is not corrupted. This avoids quite a few cases of processing known corrupted data.

For .Net 6+ we can use the ZlibStream to do this in a single step, but we need a bit more work to be Framework compatible.
As we can't use the zlib framing now, we need to calculate the length of the uncompressed data ourselves. Fix a few related bugs to make this possible.

[rhuijben]

Looks like it changes some behavior on strange/bad PDFs. It now detecteds corrupted zlib data via the adler32 checksum, and by that avoids that this bad data is later interpreted.
I think I have some managed adler32 code somewhere in another github project. Perhaps we can also fix the not NET6+ cases with that.

[BobLd]

@rhuijben thanks a lot for the PR, let me know if I can help

[rhuijben]

@BobLd I fixed the issues at the flatefilter level, but I think there is an issue in where the streams are detected. I think we should drop the final newline ("\r\n", "\r" or "\n") from the Memory object we hand down.
Otherwise every filter type has to handle these exceptions.

[BobLd]

@rhuijben do you mind giving more details about "drop the final newline ("\r\n", "\r" or "\n") from the Memory object we hand down."?

Are you refering to the following in FlateFilter?

       if (length > 0 && length < input.Length)
       {
           // Truncates final "\r\n" or "\n" from source data if any. Fixes detecting where the adler checksum is. (Zlib uses framing for this)
           input = input.Slice(0, length);
       }

Also, do you have document samples that were failing before this change and now succeed?

[rhuijben]

There are quite a few cases in the testsuite. Do you want some references for the individual failures?

I just found that length may also be available before the first filter. (Sometimes it is per filter). Perhaps that already trims the info enough so we don't need it in the flate filter.

The code relied quite a bit on the deflate implementation doing the right thing. (It usually does... but it is hard to integrate as it always reads more data from the stream below it than it should... But luckily it doesn't touch the data)

[rhuijben]

fseprd1102849.pdf from the WordCount test is one such example. (I think there are at least a hundred in the tests. Some with length parameter info, some without)..
Some having '\r\n' (Windows/network style), some '\n' (Linux) and some '\r' (Legacy Mac style).

Zlib has an internal end-of-stream marker that now just hides the problem. But other stream/file processors may have other issues with this trailing data.

[This one is fixed by either the fix in Decode() or the low level detection of "\r\n". But the same issue would have been undetected if it goes to other filters. Or they must have their own handling for trailing data]

[BobLd]

@rhuijben thanks for the details. I was looking for documents (not necessarily in the current tests), that used to fail to open before adding input.Slice(0, length); and now succeed.

But anyway, let me know when good with the changes (no rush), I'll review them

[rhuijben]

Last few changes were already cleaning up the PR.

Most things 'worked' because the zlib magic was triggered. But if zlib reports the data is corrupted, it almost always is. And I think at that point it is safer to not process it. The decompression can make a bit of junk data work out to be gigabytes of junk in bad ways. This changes behavior for some of the tests that needed other ways to work around bad data. But you'll find this in the review.

In my opinion using the length when available is a good thing. I'm still trying to find out why the top level parser would introduce this data while it is clearly not part of the actual stream. The checks inside the adler handling fix the tests to all pass but I find this an ugly hack. The length filters in the higher layers fix most (but not all!) of these cases too.

The reason I started looking was that I hoped to find the root cause of #1183. But it looks like this additional data is really encoded the way it should... (Could be used to hide information. But I think there are so many other ways to hide data in a pdf, that this is not that relevant)

[rhuijben]

If you want things separated. Feel free to ask (or do yourself if that is easier).
I tried to minimize warnings and test failures on my system.

[BobLd]

Thanks for offering to split things. I usually prefer that but I believe it'll be fine.

I've done a first pass for the review, have a look when you have time.

I'll have a 2nd look later on.

Also, do you mind squashing your commits into a single one

[EliotJones]

So I'm just looking into this because it looks like it causes page content to be skipped here #1235

In PDFBox the default is to use a more tolerant parser for these corrupted streams: https://github.com/apache/pdfbox/blob/trunk/pdfbox/src/main/java/org/apache/pdfbox/filter/FlateFilterDecoderStream.java#L47 which references this issue https://issues.apache.org/jira/browse/PDFBOX-1232.

I've run the first 2500 files of the test corpus but didn't detect any additional files affected by this. If we revert to DeflateStream what would the consequences be? Perhaps we can use checksum validation for lenient mode off but keep the support for corrupted streams where lenient parsing is on (default)?

[BobLd]

@EliotJones I'd say the priority is to be able to extract text. I'd need to have a look, but I'm not against reverting on my end if it breaks stuff...

EDIT: Would need to double check but might be related to #1243