[FFmpeg-devel] [PATCH 1/2] avcodec/h264: keep SPS and PPS bitstream data

[wm4]

On Thu, 1 Oct 2015 19:56:41 +0200
Michael Niedermayer <michaelni at gmx.at> wrote:

> On Thu, Oct 01, 2015 at 07:26:47PM +0200, wm4 wrote:
> > On Thu, 1 Oct 2015 19:08:21 +0200
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> > 
> > > On Thu, Oct 01, 2015 at 06:13:20PM +0200, wm4 wrote:
> > > > Needed for the following VideotoolBox commit.
> > > > ---
> > > >  libavcodec/h264.h    |  4 ++++
> > > >  libavcodec/h264_ps.c | 20 ++++++++++++++++----
> > > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/libavcodec/h264.h b/libavcodec/h264.h
> > > > index 7356288..eeb2aaf 100644
> > > > --- a/libavcodec/h264.h
> > > > +++ b/libavcodec/h264.h
> > > > @@ -229,6 +229,8 @@ typedef struct SPS {
> > > >      int residual_color_transform_flag;    ///< residual_colour_transform_flag
> > > >      int constraint_set_flags;             ///< constraint_set[0-3]_flag
> > > >      int new;                              ///< flag to keep track if the decoder context needs re-init due to changed SPS
> > > > +    uint8_t data[1 << 16];
> > > > +    int data_size;
> > > >  } SPS;
> > > >  
> > > >  /**
> > > > @@ -254,6 +256,8 @@ typedef struct PPS {
> > > >      uint8_t scaling_matrix8[6][64];
> > > >      uint8_t chroma_qp_table[2][QP_MAX_NUM+1];  ///< pre-scaled (with chroma_qp_index_offset) version of qp_table
> > > >      int chroma_qp_diff;
> > > > +    uint8_t data[1 << 16];
> > > > +    int data_size;
> > > >  } PPS;
> > > >  
> > > >  /**
> > > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
> > > > index 52d235c..fd16a95 100644
> > > > --- a/libavcodec/h264_ps.c
> > > > +++ b/libavcodec/h264_ps.c
> > > > @@ -307,6 +307,15 @@ int ff_h264_decode_seq_parameter_set(H264Context *h, int ignore_truncation)
> > > >      int i, log2_max_frame_num_minus4;
> > > >      SPS *sps;
> > > >  
> > > > +    sps = av_mallocz(sizeof(SPS));
> > > > +    if (!sps)
> > > > +        return AVERROR(ENOMEM);
> > > > +
> > > > +    sps->data_size = h->gb.buffer_end - h->gb.buffer;
> > > 
> > > the subtraction could overflow the 32bit int range leading to a
> > > truncated or negative data_size
> > > 
> > > [...]
> > > 
> > 
> > This is impossible. The C type used for NALs is int.
> 
> yes as long as all callers of this global function use such int based
> sizes to initialize the pointers
> if a single one is changed or one is added to initialize them from
> a int64_t then this could overflow.

There could be much more wrong wtih this, like e.g. how the GetBits
context is assumed to not have any bits written yet. But ok, I'll fix
it on push by changing the data_size fields to size_t.