Skip to content

Fix several overflows in box and track processing #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 18, 2023

Conversation

oftheforest
Copy link
Contributor

@oftheforest oftheforest commented Feb 2, 2023

Add checked arithmetic in several places where the code:

  • Reads a length from an open file, and tries to allocate that much memory
  • Subtracts from a length, overflows if it's unexpectedly small, and tries to allocate that much memory
  • Performs unchecked arithmetic on a value (e.g. to compute a sample_id), and panics (in debug mode)

(da83d3c isn't an overflow per se, but I think it's in the same spirit.)

This appears to be a bug unmasked by other changes. read_sample() calls
sample_offset() then sample_size(), and assumes that if the former returns Ok
then the latter does as well. However, if the sample_id is one past the end,
sample_offset() might succeed (it only checks samples _up to_ the given
sample_id but not _including_ it) while sample_size() fails (because the sample
doesn't exist). read_sample() will then panic.

Fix this by duplicating the error propagation (that is currently done for
sample_offset) for sample_size, instead of unwrapping. This is a cautious
change that fixes the bug; alternatively, having sample_offset() call
sample_size() on the given sample_id and propagate any error might also work.
@alfg
Copy link
Owner

alfg commented Feb 2, 2023

Hi @oftheforest. Thanks for the PR. Can you provide a description of the changes you are making here? Thank you.

Together with the entry_count checks, this eliminates several OOMs when reading
incorrect mp4 files.
@alfg
Copy link
Owner

alfg commented Feb 9, 2023

Hi @oftheforest,

Thanks for the PR. Running through some of my local test vectors on the examples and seeing a few issues:

mp4copy example:

cargo run --example mp4copy tears-of-steel-2s-audio.mp4 tears-of-steel-2s-audio-copy.mp4
    Finished dev [unoptimized + debuginfo] target(s) in 0.10s
     Running `target\debug\examples\mp4copy.exe tears-of-steel-2s-audio.mp4 tears-of-steel-2s-audio-copy.mp4`
thread 'main' panicked at 'attempt to subtract with overflow', src\track.rs:322:52
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `target\debug\examples\mp4copy.exe tears-of-steel-2s-audio.mp4 tears-of-steel-2s-audio-copy.mp4` (exit code: 101)

mp4sample example:

cargo run --example mp4sample tears-of-steel-2s-audio.mp4
    Finished dev [unoptimized + debuginfo] target(s) in 0.10s
     Running `target\debug\examples\mp4sample.exe tears-of-steel-2s-audio.mp4`
thread 'main' panicked at 'attempt to subtract with overflow', src\track.rs:322:52
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `target\debug\examples\mp4sample.exe tears-of-steel-2s-audio.mp4` (exit code: 101)

It seems to only affect an MP4 with a single track. I provided both an audio and video only mp4 below. Latest master branch seems to work OK.

tears-of-steel-2s-audio.mp4
tears-of-steel-video.mp4

This was due to an incorrect transcription when switching to checked
arithmetic, and fixes a bug that could cause attempted lookups of the wrong
chunk_id.
@alfg
Copy link
Owner

alfg commented Feb 14, 2023

Thanks for the fix. I will test again soon.

@alfg alfg merged commit 7cfdffb into alfg:master Feb 18, 2023
@alfg
Copy link
Owner

alfg commented Feb 18, 2023

Thank you, @oftheforest!

jprochazk pushed a commit to jprochazk/mp4 that referenced this pull request Sep 18, 2024
* Fix several overflows in box and track processing

* Use size_of::<Type>() instead of magic numbers

* Fix a panic in Mp4Track::read_sample() for one-past-the-end

This appears to be a bug unmasked by other changes. read_sample() calls
sample_offset() then sample_size(), and assumes that if the former returns Ok
then the latter does as well. However, if the sample_id is one past the end,
sample_offset() might succeed (it only checks samples _up to_ the given
sample_id but not _including_ it) while sample_size() fails (because the sample
doesn't exist). read_sample() will then panic.

Fix this by duplicating the error propagation (that is currently done for
sample_offset) for sample_size, instead of unwrapping. This is a cautious
change that fixes the bug; alternatively, having sample_offset() call
sample_size() on the given sample_id and propagate any error might also work.

* Account for header size in box processing overflow fixes

* Ensure that boxes aren't bigger than their containers

Together with the entry_count checks, this eliminates several OOMs when reading
incorrect mp4 files.

* Fix order of arithmetic operations

This was due to an incorrect transcription when switching to checked
arithmetic, and fixes a bug that could cause attempted lookups of the wrong
chunk_id.
CandleCandle pushed a commit to CandleCandle/mp4-rust that referenced this pull request May 7, 2025
* Fix several overflows in box and track processing

* Use size_of::<Type>() instead of magic numbers

* Fix a panic in Mp4Track::read_sample() for one-past-the-end

This appears to be a bug unmasked by other changes. read_sample() calls
sample_offset() then sample_size(), and assumes that if the former returns Ok
then the latter does as well. However, if the sample_id is one past the end,
sample_offset() might succeed (it only checks samples _up to_ the given
sample_id but not _including_ it) while sample_size() fails (because the sample
doesn't exist). read_sample() will then panic.

Fix this by duplicating the error propagation (that is currently done for
sample_offset) for sample_size, instead of unwrapping. This is a cautious
change that fixes the bug; alternatively, having sample_offset() call
sample_size() on the given sample_id and propagate any error might also work.

* Account for header size in box processing overflow fixes

* Ensure that boxes aren't bigger than their containers

Together with the entry_count checks, this eliminates several OOMs when reading
incorrect mp4 files.

* Fix order of arithmetic operations

This was due to an incorrect transcription when switching to checked
arithmetic, and fixes a bug that could cause attempted lookups of the wrong
chunk_id.
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