Reject inactive partitionings in physical_to_symbolic decode

[sahvx655-wq]

Out-of-bounds partition lookup when decoding foreign blocks in self-decompress mode

While decoding data with a context created using ASTCENC_FLG_SELF_DECOMPRESS_ONLY, I followed a block whose decoded partition index pointed at a partitioning the context had dropped, and watched get_partition_info() run off the end of the partitioning table. The header documents partitioning_packed_index as holding BLOCK_BAD_PARTITIONING for inactive entries, and block_mode_packed_index is duly cleared to its bad value before the tables are built, but the partitioning array is never initialised; omitted entries, and the whole 3/4-partition tables skipped by the count cutoff, keep stale offsets that physical_to_symbolic() reads back without any check.

The consequence is a read past the partitioning table and then an out-of-bounds write into the per-block colour arrays through the garbage texel indices, with only a debug-only assert standing in the way, so release builds either decode rubbish or crash on a hostile texture. The documented contract is that foreign input comes back as a magenta/NaN error block, so I mirrored the existing block-mode handling: clear the index to BLOCK_BAD_PARTITIONING up front and bail out with SYM_BTYPE_ERROR when a block selects an inactive partitioning. Self-produced data is untouched, since the compressor only ever emits partitionings it kept.

[solidpixel]

This is expected behavior - as the name of the flag suggests applications can only use SELF_DECOMPRESS to decompress textures that they compressed with the same configuration (and compressor version).

It is never valid for an application to do this for arbitrary textures and it is an API usage error to do so. The library API is not a security boundary, and is not expected to handle arbitrary inputs.

[sahvx655-wq]

Fair enough, that lines up with the intent of the flag. I'd been treating the decode path as if it might see hostile blocks, but you're right that SELF_DECOMPRESS_ONLY is contractually only for data the application compressed itself with the same config and compressor version, so arbitrary or crafted input is out of scope and the library isn't a security boundary here.

Happy to close this out. The only thing I'll flag in passing is the header comment documenting BLOCK_BAD_PARTITIONING for inactive partitioning_packed_index entries while the array is never actually cleared, where block_mode_packed_index is, but that's a doc-versus-code tidy-up rather than anything that warrants touching the decode path.

[solidpixel]

Yeah, that's fair. I strongly suspect some of the comments predate the index repacking. At some point in the history going back a few major versions we didn't repack and iterated the whole array where scattered indices were marked as inactive.

[solidpixel]

Having dug around the code a bit and reviewed where this ends up, I'm happy to accept this change. The changes do help protect users making API usage errors and the changes are not on a performance critical path, so there is really no downside to them.

[solidpixel]

Test failures in the new test are due to a longstanding bug in the AVX2 header (compare condition for != needs to be unordered type so it returns 1 for NaN inputs). I'll queue a different PR for that one and land that first.

EDIT: PR for this one #638

[solidpixel]

@sahvx655-wq Thanks for the PR - one bug found, and a useful usability improvement.