[vlc-devel] [vlc-commits] commit: Really fix ps_pkt_size() ( Rémi Denis-Courmont )

[Derk-Jan Hartman]

On 10 okt 2010, at 12:11, git at videolan.org wrote:

> vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net> | Sun Oct 10 12:43:38 2010 +0300| [9dc746496551eb854ab6bf0ebdeb8c8ab5cd1709] | committer: Rémi Denis-Courmont 
> 
> Really fix ps_pkt_size()
> 
> The original commit description is rather terse
>  "ps demux: fix an issue in ps_pkt_read()"
> so I don't really know what the "issue" is. But ps_pkt_size() really
> made no sense as is.
> 
> This reverts commit d761cf6f1d40a952548407b843ee0c3f0a3cd472.
> 
> Conflicts:
> 	modules/demux/ps.h

This was to fix an issue with playback of certain DVDs if I remember correctly, and either fenrir or Meuuh suggested this. Searching irc logs.....

< thedj> nefrir: ps_pkt_read() peeks 14 bytes, and then calls ps_pkt_size, without checking if it actually got 14 bytes
< nefrir> thedj: about ps, in fact ps_pkt_size need at least 6 bytes
< nefrir> thedj: and yes ps_pkt_read should check that 6 bytes at least has been read
< thedj> nefrir: ps_pkt_size won't crash over it, but ps_pkt_read will right ?
< thedj> i_size would be -1 and ps_pkt_read will try to use that ?
< nefrir> thedj: ps_pkt_read should ensure at least 6 bytes otherwise -> return NULL
< thedj> nefrir: that explains one crash we have on the bugreports list.
< nefrir> thedj: ps_pkt_size -> if -1 then I do not know
< nefrir> thedj: it means a packet is broken but we should not return NULL otherwise eof
< thedj> nefrir: but if i_peek in read is less then 14 (requested) isn't it always EOF ?>
< nefrir> thedj_: no you may have a last valid packet < 14 bytes
< thedj_> nefrir: but the smallest valid packet is always 6
< nefrir> yes
< nefrir> the start code (4) + size (2)
< thedj_> nefrir: then i'll add an assert to ps_pkt_size for i_peek < 6, and add a check to ps_pkt_read that returns NULL when i_peek < 6
< nefrir> thedj: yes

The if( i_size < 0 || ( i_size <= 6 && p_peek[3] > 0xba ) ) looks a bit iffy to me though.

DJ


>> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=9dc746496551eb854ab6bf0ebdeb8c8ab5cd1709
> ---
> 
> modules/demux/ps.c |   15 ++++++---------
> modules/demux/ps.h |   11 ++++++-----
> 2 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/modules/demux/ps.c b/modules/demux/ps.c
> index f21be2d..c05d49a 100644
> --- a/modules/demux/ps.c
> +++ b/modules/demux/ps.c
> @@ -581,16 +581,12 @@ static int ps_pkt_resynch( stream_t *s, uint32_t *pi_code )
> static block_t *ps_pkt_read( stream_t *s, uint32_t i_code )
> {
>     const uint8_t *p_peek;
> -    int      i_peek = stream_Peek( s, &p_peek, 14 );
> -    int      i_size;
> -    VLC_UNUSED(i_code);
> -
> -    /* Smallest valid packet */
> -    if( i_peek < 6 ) return NULL;
> +    int i_peek = stream_Peek( s, &p_peek, 14 );
> +    if( i_peek < 4 )
> +        return NULL;
> 
> -    i_size = ps_pkt_size( p_peek, i_peek );
> -
> -    if( i_size < 0 || ( i_size <= 6 && p_peek[3] > 0xba ) )
> +    int i_size = ps_pkt_size( p_peek, i_peek );
> +    if( i_size <= 6 && p_peek[3] > 0xba )
>     {
>         /* Special case, search the next start code */
>         i_size = 6;
> @@ -618,5 +614,6 @@ static block_t *ps_pkt_read( stream_t *s, uint32_t i_code )
>         return stream_Block( s, i_size );
>     }
> 
> +    VLC_UNUSED(i_code);
>     return NULL;
> }
> diff --git a/modules/demux/ps.h b/modules/demux/ps.h
> index c432faa..4d75b74 100644
> --- a/modules/demux/ps.h
> +++ b/modules/demux/ps.h
> @@ -299,13 +299,14 @@ static inline int ps_pkt_id( block_t *p_pkt )
>     return p_pkt->p_buffer[3];
> }
> 
> -/* return the size of the next packet
> - * You need to give him at least 14 bytes (and it need to start as a
> - * valid packet) It does not handle less than 6 bytes */
> +/* return the size of the next packet */
> static inline int ps_pkt_size( const uint8_t *p, int i_peek )
> {
> -    assert( i_peek >= 6 );
> -    if( p[3] == 0xb9 && i_peek >= 4 )
> +    if( unlikely(i_peek < 4) )
> +    {
> +        return -1;
> +    }
> +    else if( p[3] == 0xb9 )
>     {
>         return 4;
>     }
> 
> _______________________________________________
> vlc-commits mailing list
> vlc-commits at videolan.org
> http://mailman.videolan.org/listinfo/vlc-commits