[FFmpeg-devel] qt-faststart bug near 4GB
Michael Niedermayer
michael at niedermayer.cc
Sat Jun 9 21:16:43 EEST 2018
On Fri, Jun 01, 2018 at 07:00:20AM +0000, Eran Kornblau wrote:
> > On Thu, May 31, 2018 at 10:11:38AM +0000, Eran Kornblau wrote:
> > > >
> > > > -----Original Message-----
> > > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> > > > Behalf Of Eran Kornblau
> > > > Sent: Friday, May 25, 2018 4:40 PM
> > > > To: FFmpeg development discussions and patches
> > > > <ffmpeg-devel at ffmpeg.org>
> > > > Subject: [FFmpeg-devel] qt-faststart bug near 4GB
> > > >
> > > > Hi all,
> > > >
> > > > We encountered a rather extreme edge case with qt-faststart - we transcoded some video with ffmpeg, and the offset of the last video frame in the resulting mp4 was slightly less than 4GB.
> > > > Since it was less than 4GB, ffmpeg used an 'stco' atom and not a 'co64' atom.
> > > > When we ran qt-faststart on this file, it added the moov atom size to all offsets in the 'stco' atom, causing an overflow in the offsets of the frames close to the end of the file. The end of the video was therefore corrupt and could not be played.
> > > > I think the solution here is to 'upgrade' the 'stco' atom to 'co64' if such an edge case happens. However, looking at the code of qt-faststart, I see that it doesn't actually parse the atom tree, but rather looks for the strings 'stco' / 'co64'. Changing 'stco' to 'co64' requires updating the size of all the atom in which it's contained (moov, trak, mdia etc.) Therefore, such a change would probably be more of a rewrite of this utility than a patch, so wanted to check whether anyone has any thoughts on this before I start writing...
> > > >
> > > Attaching the patch for this issue.
> > > As expected, it required significant changes... hope you will like it
> > > :)
> > >
> > > Thanks!
> > >
> > > Eran
> >
> > about the AV_WB* macros, i like them alot :) but this seems not to apply cleanly:
> >
> > Applying: qt-faststart - stco offset bug fix Using index info to reconstruct a base tree...
> > M tools/qt-faststart.c
> > Falling back to patching base and 3-way merge...
> > Auto-merging tools/qt-faststart.c
> > CONFLICT (content): Merge conflict in tools/qt-faststart.c
> > error: Failed to merge in the changes.
> > Patch failed at 0001 qt-faststart - stco offset bug fix Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
> >
> Sorry Michael, I forgot to revert the previous patch before committing.
> Attaching a fixed patch.
>
> Thanks
>
> Eran
>
> > [...]
> >
> > --
> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Into a blind darkness they enter who follow after the Ignorance, they as if into a greater darkness enter who devote themselves to the Knowledge alone. -- Isha Upanishad
> >
> qt-faststart.c | 400 +++++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 345 insertions(+), 55 deletions(-)
> e4e6ca876f190a354031d269b53655effa0eaa20 0001-qt-faststart-stco-offset-bug-fix.patch
> From be7f3b43f5039297809dd88ead218e2523769064 Mon Sep 17 00:00:00 2001
> From: erankor <eran.kornblau at kaltura.com>
> Date: Fri, 1 Jun 2018 09:55:45 +0300
> Subject: [PATCH] qt-faststart - stco offset bug fix
>
> when the last offsets in the stco atom are close to 4GB, the addition of
> the moov atom size can overflow, causing corruption near the end of the
> mp4 file.
> this patch upgrades all stco atoms to co64 when such an edge case is
> detected. in order to accomplish this, the implementation was changed to
> walk the atom tree, instead of searching for the strings 'stco'/'co64'.
> this was required since when an stco atom is changed to co64, its size
> changes, and the sizes of all containing atoms (moov, trak, etc.) have
> to be updated as well.
> ---
> tools/qt-faststart.c | 400 ++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 345 insertions(+), 55 deletions(-)
>
> diff --git a/tools/qt-faststart.c b/tools/qt-faststart.c
> index d0ae7245f3..40eb3fcfc8 100644
> --- a/tools/qt-faststart.c
> +++ b/tools/qt-faststart.c
> @@ -28,6 +28,7 @@
> #include <stdlib.h>
> #include <inttypes.h>
> #include <string.h>
> +#include <limits.h>
>
> #ifdef __MINGW32__
> #undef fseeko
> @@ -43,8 +44,6 @@
>
> #define MIN(a,b) ((a) > (b) ? (b) : (a))
>
> -#define BE_16(x) ((((uint8_t*)(x))[0] << 8) | ((uint8_t*)(x))[1])
> -
> #define BE_32(x) (((uint32_t)(((uint8_t*)(x))[0]) << 24) | \
> (((uint8_t*)(x))[1] << 16) | \
> (((uint8_t*)(x))[2] << 8) | \
> @@ -59,6 +58,18 @@
> ((uint64_t)(((uint8_t*)(x))[6]) << 8) | \
> ((uint64_t)( (uint8_t*)(x))[7]))
>
> +#define AV_WB32(p, val) { \
> + ((uint8_t*)(p))[0] = ((val) >> 24) & 0xff; \
> + ((uint8_t*)(p))[1] = ((val) >> 16) & 0xff; \
> + ((uint8_t*)(p))[2] = ((val) >> 8) & 0xff; \
> + ((uint8_t*)(p))[3] = (val) & 0xff; \
> + }
> +
> +#define AV_WB64(p, val) { \
> + AV_WB32(p, (val) >> 32) \
> + AV_WB32(p + 4, val) \
> + }
> +
> #define BE_FOURCC(ch0, ch1, ch2, ch3) \
> ( (uint32_t)(unsigned char)(ch3) | \
> ((uint32_t)(unsigned char)(ch2) << 8) | \
> @@ -79,12 +90,342 @@
> #define UUID_ATOM QT_ATOM('u', 'u', 'i', 'd')
>
> #define CMOV_ATOM QT_ATOM('c', 'm', 'o', 'v')
> +#define TRAK_ATOM QT_ATOM('t', 'r', 'a', 'k')
> +#define MDIA_ATOM QT_ATOM('m', 'd', 'i', 'a')
> +#define MINF_ATOM QT_ATOM('m', 'i', 'n', 'f')
> +#define STBL_ATOM QT_ATOM('s', 't', 'b', 'l')
> #define STCO_ATOM QT_ATOM('s', 't', 'c', 'o')
> #define CO64_ATOM QT_ATOM('c', 'o', '6', '4')
>
> #define ATOM_PREAMBLE_SIZE 8
> #define COPY_BUFFER_SIZE 33554432
>
> +typedef struct {
> + uint32_t type;
> + uint32_t header_size;
> + uint64_t size;
> + unsigned char *data;
> +} atom_t;
> +
> +typedef struct {
> + uint64_t moov_atom_size;
> + uint64_t stco_offset_count;
> + uint64_t stco_data_size;
> + int stco_overflow;
> + uint32_t depth;
> +} update_chunk_offsets_context_t;
> +
> +typedef struct {
> + unsigned char *dest;
> + uint64_t original_moov_size;
> + uint64_t new_moov_size;
> +} upgrade_stco_context_t;
> +
> +typedef int (*parse_atoms_callback_t)(void *context, atom_t *atom);
> +
> +static int parse_atoms(
> + unsigned char *buf,
> + uint64_t size,
> + parse_atoms_callback_t callback,
> + void *context)
> +{
> + unsigned char *pos = buf;
> + unsigned char *end = pos + size;
> + atom_t atom;
> + int ret;
> +
> + while (end - pos >= ATOM_PREAMBLE_SIZE) {
> + atom.size = BE_32(pos);
> + atom.type = BE_32(pos + 4);
> + pos += ATOM_PREAMBLE_SIZE;
> + atom.header_size = ATOM_PREAMBLE_SIZE;
> +
> + switch (atom.size) {
> + case 1:
> + if (end - pos < 8) {
> + printf("not enough room for 64 bit atom size\n");
> + return -1;
> + }
> +
> + atom.size = BE_64(pos);
> + pos += 8;
> + atom.header_size = ATOM_PREAMBLE_SIZE + 8;
> + break;
> +
> + case 0:
> + atom.size = ATOM_PREAMBLE_SIZE + end - pos;
> + break;
> + }
> +
> + if (atom.size < atom.header_size) {
> + printf("atom size %"PRIu64" too small\n", atom.size);
errors should go to stderr if av_log() is not used
i see this is not introduced by the patch but was that way before so its
more a comment about the code in git than the patch ...
but ideally this should be fixed in a seperate patch either before or after
this one
some self test for the newly added feature would also be a good idea
also, was the new code tested with a fuzzer ?
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180609/17c2a112/attachment.sig>
More information about the ffmpeg-devel
mailing list