[vlc-devel] [PATCH 02/11] demux: wav: refactor chunk skip

[Rémi Denis-Courmont]

Le perjantaina 13. maaliskuuta 2020, 15.54.11 EET Thomas Guillem a écrit :
> On Thu, Mar 12, 2020, at 17:34, Remi Denis-Courmont wrote:
> > Le 2020-03-12 16:05, Thomas Guillem a écrit :
> > > ---
> > > 
> > >  modules/demux/wav.c | 19 +++++++++++++------
> > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/modules/demux/wav.c b/modules/demux/wav.c
> > > index c68f5e916eb..4f483df10c7 100644
> > > --- a/modules/demux/wav.c
> > > +++ b/modules/demux/wav.c
> > > @@ -116,6 +116,17 @@ static int Control( demux_t *p_demux, int
> > > i_query, va_list args )
> > > 
> > >                                     i_query, args );
> > >  
> > >  }
> > > 
> > > +static int ChunkSkip( demux_t *p_demux, size_t i_size )
> > > +{
> > > +    if( vlc_stream_Read( p_demux->s, NULL, i_size ) != (ssize_t)i_size
> > > )
> > > +        return VLC_EGENERIC;
> > 
> > Can you really assume that i_size <= SSIZE_MAX here even on 32-bits
> > platforms?
> > 
> > Also i_size += i_size & 1; would simplify.
> 
> Thanks for your comment.
> 
> Is something like that OK ?
> 
> static int ChunkSkip( demux_t *p_demux, uint32_t i_size )
> {
>     ssize_t i_ret;
>     i_size += i_size & 1;
> 
> #if (UINT32_MAX > SSIZE_MAX)
>     if( unlikely( i_size > SSIZE_MAX ) )
>     {
>         i_ret = vlc_stream_Read( p_demux->s, NULL, SSIZE_MAX );
>         if( i_ret < 0 || i_ret != SSIZE_MAX )
>             return VLC_EGENERIC;
>         i_size -= SSIZE_MAX;
>         assert( i_size <= SSIZE_MAX );

In other words, assert(i_size <= SIZE_MAX - 1); at function entry.
Are you sure this is valid?

>     }
> #endif
> 
>     i_ret = vlc_stream_Read( p_demux->s, NULL, i_size );
>     return i_ret < 0 || i_ret != i_size ? VLC_EGENERIC : VLC_SUCCESS;
> }
> 
> uint32_t instead of size_t since value come directly from GetDWLE()

So I guess it is NOT a valid assertion.

> I won't handle uint64_t skip since the 64bit data will never be skipped

Using stream_Read to skip a variable and potentially large block of data is 
just a bad idea. Realistically, if the stream is not seekable, you don't want 
to read that data to skip it as it could take hours.

IMO, stream_Read(NULL) looks like a convenient hack, but it's just poor 
design. We should have a separate function for relative seeking.

-- 
レミ・デニ-クールモン
http://www.remlab.net/