Skip to content

Conversation

@chieltbest
Copy link
Contributor

Some tools used for the sims 2 modding will let users create packages that have some resources mark as compressed, but have the actual stored data be uncompressed (or vice versa).

This means that all resources have to be tested by attempting to decompress them, and if decompression fails for all header formats it should be assumed to be uncompressed. (example)

The consequence is that the decompression must not panic under any circumstance, even with adversarial inputs.

So far I did three things:

  1. Make a test asserting that decompression works for all inputs, this test is a little limited but it works for now.
  2. Fix an infinite loop found while running the above test.
  3. Fix a panic when overrunning the decompression buffer, part of which was already fixed in Fix: Maxis large file actual fix #17

There should probably also be a way for users to specify the level of strictness on buffer size checks, as some inputs are currently accepted that probably should be rejected with an error in some applications.
I'm thinking of having three strictness levels:

  1. The current situation, ignore the decompressed size (except for initial buffer size), good for reliability.
  2. Don't allow decompression buffer overruns, good for e.g. reading untrusted inputs and performance.
  3. Same as 2, but also check the final size after decompressing, good for e.g. checking tools.

It would also be nice to be able to specify a maximum initial buffer size, so that garbage data can't allocate multiple gigabytes of memory for some inputs.

When doing a major version for the options change it would probably also be nice to change the compression options struct to use a builder pattern, so that future added options wouldn't be a breaking change.

This fixes a problem where files that have a 0 decompressed length will try to double the buffer size indefinitely.
The panic in rle_decode_fixed was previously solved in actioninja#17, this moves that code to also cover the copy_from_reader sections.
@actioninja
Copy link
Owner

Oh nice the new tests I added don't work at all in CI, something else to look into. Clippy fixes are pretty trivial and coverage is a non-blocker.

Also gotta love when assumptions about data formats cannot hold at all due to a big pile of janky user tools and sometimes janky official tools.

@actioninja
Copy link
Owner

actioninja commented Dec 9, 2025

Looks like I just accidentally commit some files that should have been ignored, rebase on to master and tests should run again.
EDIT: I lied, still failing in CI, going to look into this.
EDIT2: Ok no longer lying, seems to actually be working now.

@actioninja
Copy link
Owner

Wow, the full symmetrical integration tests take 3 hours to run in CI, going to have to make those manually invoked.

@chieltbest chieltbest marked this pull request as ready for review December 11, 2025 11:20
@chieltbest
Copy link
Contributor Author

chieltbest commented Dec 11, 2025

This is good enough for now for a patch version, I'll make the api changes in a separate pull request later.

@actioninja actioninja merged commit 4c2b162 into actioninja:main Dec 17, 2025
7 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants