[vlc-devel] [PATCH] demux: mp4: Add support for GoPro HiLight tags as chapters, so they can appear in the progress bar

[Francois Cartegnie]

Le 28/10/2015 19:56, Emeric Grange a écrit :
> +static int MP4_ReadBox_HMMT( stream_t *p_stream, MP4_Box_t *p_box )
> +{
> +    MP4_Box_data_HMMT_t *p_hmmt;
> +    char tag_nb[4] = {0};
> +
> +    MP4_READBOX_ENTER( MP4_Box_data_HMMT_t, MP4_FreeBox_hmmt );
> +
> +    p_hmmt = p_box->data.p_hmmt;

> +    MP4_GET4BYTES( p_hmmt->i_chapter );
> +

No size check has been done

> +    for( uint32_t i = 0; i < p_hmmt->i_chapter; i++ )
> +    {
> +        uint64_t i_start;
> +
> +        p_hmmt->chapter[i].psz_name = malloc( 18 );
> +        if( !p_hmmt->chapter[i].psz_name )
> +            MP4_READBOX_EXIT( 0 );
> +        strncpy(p_hmmt->chapter[i].psz_name, "HiLight tag #", 17);
> +        snprintf(tag_nb, 3, "%d", i + 1);
> +        strncat(p_hmmt->chapter[i].psz_name, tag_nb, 4);

asprintf

> +
> +        MP4_GET4BYTES( i_start );

4 bytes in 64bits ??

> +        p_hmmt->chapter[i].i_start = i_start;
> +    }

No read size checks done, no array size invalidation...

>  typedef struct
>  {
> +    uint8_t  i_version;
> +    uint32_t i_flags;

What for ?

> +
> +    uint32_t i_chapter;
> +    struct
> +    {
> +        char    *psz_name;
> +        int64_t  i_start;
> +    } chapter[400];

That's a no go. Bloat + heap write overflow now.

> +        // HiLights are expressed in ms, convert to µs

store on 32bits then

> +        s->i_time_offset = BOXDATA(p_chpl)->chapter[i].i_start * 1000;
> +        TAB_APPEND( p_sys->p_title->i_seekpoint, p_sys->p_title->seekpoint, s );

If you know final size before allocation, don't tab append.

>      }
> +    else if( ( p_chpl = MP4_BoxGet( p_sys->p_root, "/moov/udta/HMMT" ) ) &&
> +          BOXDATA(p_chpl) && BOXDATA(p_chpl)->i_chapter > 0 )

Wrong union member


Francois