PNG: Account for memory limit when decompressing text

[RunDevelopment]

Resolves #2993

I kept the old limit set by the png as a maxium. So the limit is 2 MB or the memory allocation limit, whichever is smaller.

This may not be ideal. There are legitimate use cases for very large text chunks. Maybe we could raise the maximum depending on the memory limit. Maybe like this: min(mem_limit, max(2 MB, mem_limit * 5%). The idea is that we don't want to blow the entire memory budget on decompressing text, but using a small percentage should be okay. This would support huge text chunks with the default memory limit, while still preventing decompression bombs. Alternatively, we could also just raise the hard-coded limit to something like 10 to 16 MB and call it a day. Thoughts?

[mstoeckl]

As per an old issue from a different project (opencv/opencv#22551), people do sometimes create images which may embed large files in text chunks, so I do not think this hard limit will suffice in the long term.

This might be OK as a short term patch, in which case I think you should add a TODO comment. The cleanest solution might be to introduce some way to share the memory limit of the underlying image-png decoder; you've suggested a custom allocator in #2966.

(Edit: oops, I don't think anyone will embed a file into XMP or IPTC, so this will probably be OK long term.)

min(mem_limit, max(2 MB, mem_limit * 5%)

For the memory limit to remain accurate (assuming negligible allocation overhead), you'd need to reduce the memory limit passed to the PNG decoder accordingly.

[RunDevelopment]

min(mem_limit, max(2 MB, mem_limit * 5%)

For the memory limit to remain accurate (assuming negligible allocation overhead), you'd need to reduce the memory limit passed to the PNG decoder accordingly.

We are passing the entire memory budget to the PNG decoder, so I think we'd need to reduce it by how much memory the PNG decoder is currently using. The PNG decoder should be tracking that internally (like we do), but I don't think there's any way to get that.

[197g]

The current implementation is fine using it as the maximum allocation size for most purposes; there are two PRs open sketching changes to the limit system to be able to do this but we'd need every decoder to agree on a scheme here. Since allocators are being stabilized soon (tm) I'd say that mechanism best wait on this. In the meantime, there is no consistent mechanism for splitting the budget and that's still better than not adhering to a limit. In particular, lowering the limit still has the effect of reducing the maximum resident size even if not precise.

[RunDevelopment]

Hmmm, true. In that case this PR will be unnecessary soon (tm). Should I close it?

Btw, is there already a crate that implements an allocator like we'd need it? I did a quick search but found nothing that fits the bill.

[197g]

I'll take it with allocation duplication, we'll figure out the allocator independently. One final note that applies here in particular: The resulting allocation(Vec<u8>) is returned to the caller with the default global allocator which will remain the interface. But then there is no resource to be tracked by the allocator, the decoder can never dealloc it (to be clear, I'm not saying this is a needs-to-be-changed problem with the interface). Arguably, it should not count towards the budget use at that point and only intermittently check that budget would have been available.