Opened 2 years ago

Closed 2 years ago

#9622 closed defect (fixed)

Skip samples overflow and negative in fragmented MP4

Reported by: Matt Wolenetz Owned by:
Priority: normal Component: avformat
Version: git-master Keywords: mov, skip_samples, timestamp, seek
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description (last modified by Matt Wolenetz)

Summary of the bug:
Negative int64_t result of mov_get_skip_samples() can result if the timestamps for samples in moofs decrease (e.g. first moof->traf->tfdt having base decode media time of Z, second moof-...time of Y, third ... of X, where Z>>Y>>X).
Scenario: Seek to time X. Observe that the next packet read has time X but skip_samples (front discard) value in side data that is negative.

Two questions:
1) Should such a stream instead produce parse error of some kind before emitting a packet with a negative skip_samples?
How to reproduce: chromium fuzzer produced a case, but similar can be crafted manually. Reference https://crbug.com/1189939.

2) Return type of mov_get_skip_samples is larger than where it is used in caller, resulting in overflow/truncation. Is this only possible for malformed streams, which don't currently yield error but maybe should (see (1))? Or should the overflow/truncation be fixed or itself trigger some error?

Downstream Chromium will likely just clip negative front_skip_samples to be non-negative, but such case may imply other issues in ffmpeg. Hence this bug.

% ffplay -loglevel trace -ss <time X> <reproFile>

...messages like:
[mov,mp4,m4a,3gp,3g2,mj2 @ ...] demuxer injecting skip -925335552 / discard 0
...
[opus @ ...] skip -925335552 / discard 0 samples due to side data

ffplay version N-105440-gf23d3a5f8f Copyright (c) 2003-2022 the FFmpeg developers

Change History (2)

comment:1 by Matt Wolenetz, 2 years ago

Description: modified (diff)

comment:2 by James, 2 years ago

Resolution: fixed
Status: newclosed

Technically speaking, the first field in AV_PKT_DATA_SKIP_SAMPLES described as "number of samples to skip from start of this packet" is to be interpreted as an uint32, not an int. I have changed that for the aforementioned debug log message in 928e7c60cc5f220e931df51392b5577b253ffa23 to reflect this.

Either way, since the side data is ultimately going to be used to skip samples in an AVFrame where nb_samples is an int, I've clipped the value to the 0..INT_MAX range in 3b9bd63ad92967e06d5e2f67e0cfd9093bf7700d and 22d6d2b4818f3b1b451d1b4c4dd3d484e3040261.

Note: See TracTickets for help on using tickets.