| Message ID | 20170704203352.13136-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | c8cfbc6629c1fe5755b59a3bcfd95ad08b843a07 |
| Headers | show |
On 7/4/2017 9:33 PM, Michael Niedermayer wrote: > Fixes: out of array access > Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/hevcdec.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) Is it possible this will cause any regressions with mid-stream parameter change handling? - Derek
On Tue, Jul 04, 2017 at 09:53:00PM +0100, Derek Buitenhuis wrote: > On 7/4/2017 9:33 PM, Michael Niedermayer wrote: > > Fixes: out of array access > > Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/hevcdec.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > Is it possible this will cause any regressions with mid-stream parameter change > handling? do you have some testcases that you are concerned about ? can you add them to fate ? thx [...]
On 7/4/2017 11:09 PM, Michael Niedermayer wrote: > do you have some testcases that you are concerned about ? > can you add them to fate ? I can make some and test tomorrow morning if you're fine with waiting half a day. - Derek
On Tue, Jul 4, 2017 at 10:53 PM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > On 7/4/2017 9:33 PM, Michael Niedermayer wrote: >> Fixes: out of array access >> Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/hevcdec.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) > > Is it possible this will cause any regressions with mid-stream parameter change > handling? > The patch seems fine to me, mid-stream extradata should be populated to avctx when its used by a frame, not when its parsed. The export during parsing should only be done on codec open so initial values are populated to the context right away. - Hendrik
On 7/5/2017 12:15 AM, Hendrik Leppkes wrote: > The patch seems fine to me, mid-stream extradata should be populated > to avctx when its used by a frame, not when its parsed. > The export during parsing should only be done on codec open so initial > values are populated to the context right away. Makes sense. I won't bother with samples tomorrow. - Derek
On Wed, Jul 05, 2017 at 01:15:39AM +0200, Hendrik Leppkes wrote: > On Tue, Jul 4, 2017 at 10:53 PM, Derek Buitenhuis > <derek.buitenhuis@gmail.com> wrote: > > On 7/4/2017 9:33 PM, Michael Niedermayer wrote: > >> Fixes: out of array access > >> Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 > >> > >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> --- > >> libavcodec/hevcdec.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > > > > Is it possible this will cause any regressions with mid-stream parameter change > > handling? > > > > The patch seems fine to me, mid-stream extradata should be populated > to avctx when its used by a frame, not when its parsed. > The export during parsing should only be done on codec open so initial > values are populated to the context right away. applied thx [...]
On Tue, 4 Jul 2017 22:33:52 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: out of array access > Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/hevcdec.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > index cc8ac82164..55f51211c3 100644 > --- a/libavcodec/hevcdec.c > +++ b/libavcodec/hevcdec.c > @@ -3057,7 +3057,7 @@ static int verify_md5(HEVCContext *s, AVFrame *frame) > return 0; > } > > -static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > +static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length, int first) > { > int ret, i; > > @@ -3069,7 +3069,7 @@ static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > /* export stream parameters from the first SPS */ > for (i = 0; i < FF_ARRAY_ELEMS(s->ps.sps_list); i++) { > - if (s->ps.sps_list[i]) { > + if (first && s->ps.sps_list[i]) { > const HEVCSPS *sps = (const HEVCSPS*)s->ps.sps_list[i]->data; > export_stream_params(s->avctx, &s->ps, sps); > break; > @@ -3099,7 +3099,7 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, > &new_extradata_size); > if (new_extradata && new_extradata_size > 0) { > - ret = hevc_decode_extradata(s, new_extradata, new_extradata_size); > + ret = hevc_decode_extradata(s, new_extradata, new_extradata_size, 0); > if (ret < 0) > return ret; > } > @@ -3387,7 +3387,7 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) > s->threads_number = 1; > > if (avctx->extradata_size > 0 && avctx->extradata) { > - ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size); > + ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1); > if (ret < 0) { > hevc_decode_free(avctx); > return ret; Couldn't that have done in a less confusing way? What the heck does "first" even mean? (Also you have to look up what that means on the caller site.) Can you explain this?
On Wed, Jul 05, 2017 at 09:56:10AM +0200, wm4 wrote: > On Tue, 4 Jul 2017 22:33:52 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > Fixes: out of array access > > Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/hevcdec.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > index cc8ac82164..55f51211c3 100644 > > --- a/libavcodec/hevcdec.c > > +++ b/libavcodec/hevcdec.c > > @@ -3057,7 +3057,7 @@ static int verify_md5(HEVCContext *s, AVFrame *frame) > > return 0; > > } > > > > -static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > +static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length, int first) > > { > > int ret, i; > > > > @@ -3069,7 +3069,7 @@ static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > > /* export stream parameters from the first SPS */ > > for (i = 0; i < FF_ARRAY_ELEMS(s->ps.sps_list); i++) { > > - if (s->ps.sps_list[i]) { > > + if (first && s->ps.sps_list[i]) { > > const HEVCSPS *sps = (const HEVCSPS*)s->ps.sps_list[i]->data; > > export_stream_params(s->avctx, &s->ps, sps); > > break; > > @@ -3099,7 +3099,7 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > > new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, > > &new_extradata_size); > > if (new_extradata && new_extradata_size > 0) { > > - ret = hevc_decode_extradata(s, new_extradata, new_extradata_size); > > + ret = hevc_decode_extradata(s, new_extradata, new_extradata_size, 0); > > if (ret < 0) > > return ret; > > } > > @@ -3387,7 +3387,7 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) > > s->threads_number = 1; > > > > if (avctx->extradata_size > 0 && avctx->extradata) { > > - ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size); > > + ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1); > > if (ret < 0) { > > hevc_decode_free(avctx); > > return ret; > > Couldn't that have done in a less confusing way? What the heck does > "first" even mean? (Also you have to look up what that means on the > caller site.) > > Can you explain this? first means "Preceding all others of a series or kind;" https://en.wiktionary.org/wiki/first in the context of hevc_decode_extradata(), a function decoding extradata that signifies the first of a series of extradata. No question it can be done differently, everything can be done differently. [...]
On Wed, 5 Jul 2017 16:08:38 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Jul 05, 2017 at 09:56:10AM +0200, wm4 wrote: > > On Tue, 4 Jul 2017 22:33:52 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > Fixes: out of array access > > > Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/hevcdec.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > > index cc8ac82164..55f51211c3 100644 > > > --- a/libavcodec/hevcdec.c > > > +++ b/libavcodec/hevcdec.c > > > @@ -3057,7 +3057,7 @@ static int verify_md5(HEVCContext *s, AVFrame *frame) > > > return 0; > > > } > > > > > > -static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > +static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length, int first) > > > { > > > int ret, i; > > > > > > @@ -3069,7 +3069,7 @@ static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > > > > /* export stream parameters from the first SPS */ > > > for (i = 0; i < FF_ARRAY_ELEMS(s->ps.sps_list); i++) { > > > - if (s->ps.sps_list[i]) { > > > + if (first && s->ps.sps_list[i]) { > > > const HEVCSPS *sps = (const HEVCSPS*)s->ps.sps_list[i]->data; > > > export_stream_params(s->avctx, &s->ps, sps); > > > break; > > > @@ -3099,7 +3099,7 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > > > new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, > > > &new_extradata_size); > > > if (new_extradata && new_extradata_size > 0) { > > > - ret = hevc_decode_extradata(s, new_extradata, new_extradata_size); > > > + ret = hevc_decode_extradata(s, new_extradata, new_extradata_size, 0); > > > if (ret < 0) > > > return ret; > > > } > > > @@ -3387,7 +3387,7 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) > > > s->threads_number = 1; > > > > > > if (avctx->extradata_size > 0 && avctx->extradata) { > > > - ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size); > > > + ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1); > > > if (ret < 0) { > > > hevc_decode_free(avctx); > > > return ret; > > > > Couldn't that have done in a less confusing way? What the heck does > > "first" even mean? (Also you have to look up what that means on the > > caller site.) > > > > Can you explain this? > > first means "Preceding all others of a series or kind;" https://en.wiktionary.org/wiki/first Thanks for the English lesson. By the way, you're the only person on the internet who uses "iam". > in the context of hevc_decode_extradata(), a function decoding extradata > that signifies the first of a series of extradata. > > No question it can be done differently, everything can be done > differently. So can you explain why "first" is checked every loop, instead of putting the loop into an if, or (what should have been done) moving the entire loop out of the function and putting it inline into the only place where it's effectively used?
On Wed, Jul 05, 2017 at 04:39:05PM +0200, wm4 wrote: > On Wed, 5 Jul 2017 16:08:38 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Wed, Jul 05, 2017 at 09:56:10AM +0200, wm4 wrote: > > > On Tue, 4 Jul 2017 22:33:52 +0200 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > Fixes: out of array access > > > > Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/hevcdec.c | 8 ++++---- > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > > > index cc8ac82164..55f51211c3 100644 > > > > --- a/libavcodec/hevcdec.c > > > > +++ b/libavcodec/hevcdec.c > > > > @@ -3057,7 +3057,7 @@ static int verify_md5(HEVCContext *s, AVFrame *frame) > > > > return 0; > > > > } > > > > > > > > -static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > > +static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length, int first) > > > > { > > > > int ret, i; > > > > > > > > @@ -3069,7 +3069,7 @@ static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > > > > > > /* export stream parameters from the first SPS */ > > > > for (i = 0; i < FF_ARRAY_ELEMS(s->ps.sps_list); i++) { > > > > - if (s->ps.sps_list[i]) { > > > > + if (first && s->ps.sps_list[i]) { > > > > const HEVCSPS *sps = (const HEVCSPS*)s->ps.sps_list[i]->data; > > > > export_stream_params(s->avctx, &s->ps, sps); > > > > break; > > > > @@ -3099,7 +3099,7 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > > > > new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, > > > > &new_extradata_size); > > > > if (new_extradata && new_extradata_size > 0) { > > > > - ret = hevc_decode_extradata(s, new_extradata, new_extradata_size); > > > > + ret = hevc_decode_extradata(s, new_extradata, new_extradata_size, 0); > > > > if (ret < 0) > > > > return ret; > > > > } > > > > @@ -3387,7 +3387,7 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) > > > > s->threads_number = 1; > > > > > > > > if (avctx->extradata_size > 0 && avctx->extradata) { > > > > - ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size); > > > > + ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1); > > > > if (ret < 0) { > > > > hevc_decode_free(avctx); > > > > return ret; > > > > > > Couldn't that have done in a less confusing way? What the heck does > > > "first" even mean? (Also you have to look up what that means on the > > > caller site.) > > > > > > Can you explain this? > > > > first means "Preceding all others of a series or kind;" https://en.wiktionary.org/wiki/first > > Thanks for the English lesson. By the way, you're the only person on > the internet who uses "iam". > > > in the context of hevc_decode_extradata(), a function decoding extradata > > that signifies the first of a series of extradata. > > > > No question it can be done differently, everything can be done > > differently. > > So can you explain why "first" is checked every loop, instead of > putting the loop into an if, or Every piece of code can be implemented im many ways. Theres many equivalent and near equivalent ways to do it. add a if(), add a condition to a existing if(), ... some of these are a bit simpler, some are a bit faster, speedwise theres nothing one can gain here as this executes too rarely > (what should have been done) moving the > entire loop out of the function and putting it inline into the only > place where it's effectively used? For a security fix i like to make sure it can be robustly backported. If i add a "int first" the compiler makes sure that every call has the parameter set. If i move code around the compiler wouldnt notice if there was another call. For git master, i agree moving the code is slightly nicer, ill post a patch to make that change thx [...]
On Thu, 6 Jul 2017 11:58:47 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Jul 05, 2017 at 04:39:05PM +0200, wm4 wrote: > > On Wed, 5 Jul 2017 16:08:38 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Wed, Jul 05, 2017 at 09:56:10AM +0200, wm4 wrote: > > > > On Tue, 4 Jul 2017 22:33:52 +0200 > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > Fixes: out of array access > > > > > Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > libavcodec/hevcdec.c | 8 ++++---- > > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > > > > index cc8ac82164..55f51211c3 100644 > > > > > --- a/libavcodec/hevcdec.c > > > > > +++ b/libavcodec/hevcdec.c > > > > > @@ -3057,7 +3057,7 @@ static int verify_md5(HEVCContext *s, AVFrame *frame) > > > > > return 0; > > > > > } > > > > > > > > > > -static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > > > +static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length, int first) > > > > > { > > > > > int ret, i; > > > > > > > > > > @@ -3069,7 +3069,7 @@ static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > > > > > > > > /* export stream parameters from the first SPS */ > > > > > for (i = 0; i < FF_ARRAY_ELEMS(s->ps.sps_list); i++) { > > > > > - if (s->ps.sps_list[i]) { > > > > > + if (first && s->ps.sps_list[i]) { > > > > > const HEVCSPS *sps = (const HEVCSPS*)s->ps.sps_list[i]->data; > > > > > export_stream_params(s->avctx, &s->ps, sps); > > > > > break; > > > > > @@ -3099,7 +3099,7 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > > > > > new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, > > > > > &new_extradata_size); > > > > > if (new_extradata && new_extradata_size > 0) { > > > > > - ret = hevc_decode_extradata(s, new_extradata, new_extradata_size); > > > > > + ret = hevc_decode_extradata(s, new_extradata, new_extradata_size, 0); > > > > > if (ret < 0) > > > > > return ret; > > > > > } > > > > > @@ -3387,7 +3387,7 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) > > > > > s->threads_number = 1; > > > > > > > > > > if (avctx->extradata_size > 0 && avctx->extradata) { > > > > > - ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size); > > > > > + ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1); > > > > > if (ret < 0) { > > > > > hevc_decode_free(avctx); > > > > > return ret; > > > > > > > > Couldn't that have done in a less confusing way? What the heck does > > > > "first" even mean? (Also you have to look up what that means on the > > > > caller site.) > > > > > > > > Can you explain this? > > > > > > first means "Preceding all others of a series or kind;" https://en.wiktionary.org/wiki/first > > > > Thanks for the English lesson. By the way, you're the only person on > > the internet who uses "iam". > > > > > > in the context of hevc_decode_extradata(), a function decoding extradata > > > that signifies the first of a series of extradata. > > > > > > No question it can be done differently, everything can be done > > > differently. > > > > So can you explain why "first" is checked every loop, instead of > > putting the loop into an if, or > > Every piece of code can be implemented im many ways. > Theres many equivalent and near equivalent ways to do it. > add a if(), add a condition to a existing if(), ... > some of these are a bit simpler, some are a bit faster, speedwise > theres nothing one can gain here as this executes too rarely Well this one has literally no advantage, and it's like it was written in this way just to be tricky. > > > > (what should have been done) moving the > > entire loop out of the function and putting it inline into the only > > place where it's effectively used? > > For a security fix i like to make sure it can be robustly backported. > If i add a "int first" the compiler makes sure that every call has > the parameter set. > If i move code around the compiler wouldnt notice if there was another > call. > > For git master, i agree moving the code is slightly nicer, ill post a > patch to make that change > > thx > > [...] I don't think the backport argument works either. If it's a subtle security issue, you should probably do something more careful than cherry-pick + see if it compiles.
On Thu, Jul 06, 2017 at 12:35:28PM +0200, wm4 wrote: > On Thu, 6 Jul 2017 11:58:47 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Wed, Jul 05, 2017 at 04:39:05PM +0200, wm4 wrote: > > > On Wed, 5 Jul 2017 16:08:38 +0200 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > On Wed, Jul 05, 2017 at 09:56:10AM +0200, wm4 wrote: > > > > > On Tue, 4 Jul 2017 22:33:52 +0200 > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > Fixes: out of array access > > > > > > Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 > > > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > --- > > > > > > libavcodec/hevcdec.c | 8 ++++---- > > > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > > > > > index cc8ac82164..55f51211c3 100644 > > > > > > --- a/libavcodec/hevcdec.c > > > > > > +++ b/libavcodec/hevcdec.c > > > > > > @@ -3057,7 +3057,7 @@ static int verify_md5(HEVCContext *s, AVFrame *frame) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > > > > +static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length, int first) > > > > > > { > > > > > > int ret, i; > > > > > > > > > > > > @@ -3069,7 +3069,7 @@ static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > > > > > > > > > > /* export stream parameters from the first SPS */ > > > > > > for (i = 0; i < FF_ARRAY_ELEMS(s->ps.sps_list); i++) { > > > > > > - if (s->ps.sps_list[i]) { > > > > > > + if (first && s->ps.sps_list[i]) { > > > > > > const HEVCSPS *sps = (const HEVCSPS*)s->ps.sps_list[i]->data; > > > > > > export_stream_params(s->avctx, &s->ps, sps); > > > > > > break; > > > > > > @@ -3099,7 +3099,7 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > > > > > > new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, > > > > > > &new_extradata_size); > > > > > > if (new_extradata && new_extradata_size > 0) { > > > > > > - ret = hevc_decode_extradata(s, new_extradata, new_extradata_size); > > > > > > + ret = hevc_decode_extradata(s, new_extradata, new_extradata_size, 0); > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > } > > > > > > @@ -3387,7 +3387,7 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) > > > > > > s->threads_number = 1; > > > > > > > > > > > > if (avctx->extradata_size > 0 && avctx->extradata) { > > > > > > - ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size); > > > > > > + ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1); > > > > > > if (ret < 0) { > > > > > > hevc_decode_free(avctx); > > > > > > return ret; > > > > > > > > > > Couldn't that have done in a less confusing way? What the heck does > > > > > "first" even mean? (Also you have to look up what that means on the > > > > > caller site.) > > > > > > > > > > Can you explain this? > > > > > > > > first means "Preceding all others of a series or kind;" https://en.wiktionary.org/wiki/first > > > > > > Thanks for the English lesson. By the way, you're the only person on > > > the internet who uses "iam". > > > > > > > > > in the context of hevc_decode_extradata(), a function decoding extradata > > > > that signifies the first of a series of extradata. > > > > > > > > No question it can be done differently, everything can be done > > > > differently. > > > > > > So can you explain why "first" is checked every loop, instead of > > > putting the loop into an if, or > > > > Every piece of code can be implemented im many ways. > > Theres many equivalent and near equivalent ways to do it. > > add a if(), add a condition to a existing if(), ... > > some of these are a bit simpler, some are a bit faster, speedwise > > theres nothing one can gain here as this executes too rarely > > Well this one has literally no advantage, and it's like it was written > in this way just to be tricky. > > > > > > > > (what should have been done) moving the > > > entire loop out of the function and putting it inline into the only > > > place where it's effectively used? > > > > For a security fix i like to make sure it can be robustly backported. > > If i add a "int first" the compiler makes sure that every call has > > the parameter set. > > If i move code around the compiler wouldnt notice if there was another > > call. > > > > For git master, i agree moving the code is slightly nicer, ill post a > > patch to make that change > > > > thx > > > > [...] > > I don't think the backport argument works either. > If it's a subtle > security issue, you should probably do something more careful than > cherry-pick + see if it compiles. yes absolutely but Is it a subtle issue, noone has claimed it is nor that it isnt. And where did i say that one would just do "cherry-pick + see if it compiles." I do like to use all tools available that help produce better code with less effort. If i can use the compiler to check against some issues, thats a good thing The fewer things the compiler can check the more things will be buggy on average when the human time spend on it stays constant. No matter how much or little time the human spends on it That said, if you want to help backport fixes, that surely is welcome. With whatever code style you prefer. [...]
On Thu, 6 Jul 2017 16:29:21 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Jul 06, 2017 at 12:35:28PM +0200, wm4 wrote: > > On Thu, 6 Jul 2017 11:58:47 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Wed, Jul 05, 2017 at 04:39:05PM +0200, wm4 wrote: > > > > On Wed, 5 Jul 2017 16:08:38 +0200 > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > On Wed, Jul 05, 2017 at 09:56:10AM +0200, wm4 wrote: > > > > > > On Tue, 4 Jul 2017 22:33:52 +0200 > > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > > > Fixes: out of array access > > > > > > > Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 > > > > > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > > --- > > > > > > > libavcodec/hevcdec.c | 8 ++++---- > > > > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > > > > > > index cc8ac82164..55f51211c3 100644 > > > > > > > --- a/libavcodec/hevcdec.c > > > > > > > +++ b/libavcodec/hevcdec.c > > > > > > > @@ -3057,7 +3057,7 @@ static int verify_md5(HEVCContext *s, AVFrame *frame) > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > -static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > > > > > +static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length, int first) > > > > > > > { > > > > > > > int ret, i; > > > > > > > > > > > > > > @@ -3069,7 +3069,7 @@ static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) > > > > > > > > > > > > > > /* export stream parameters from the first SPS */ > > > > > > > for (i = 0; i < FF_ARRAY_ELEMS(s->ps.sps_list); i++) { > > > > > > > - if (s->ps.sps_list[i]) { > > > > > > > + if (first && s->ps.sps_list[i]) { > > > > > > > const HEVCSPS *sps = (const HEVCSPS*)s->ps.sps_list[i]->data; > > > > > > > export_stream_params(s->avctx, &s->ps, sps); > > > > > > > break; > > > > > > > @@ -3099,7 +3099,7 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, > > > > > > > new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, > > > > > > > &new_extradata_size); > > > > > > > if (new_extradata && new_extradata_size > 0) { > > > > > > > - ret = hevc_decode_extradata(s, new_extradata, new_extradata_size); > > > > > > > + ret = hevc_decode_extradata(s, new_extradata, new_extradata_size, 0); > > > > > > > if (ret < 0) > > > > > > > return ret; > > > > > > > } > > > > > > > @@ -3387,7 +3387,7 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) > > > > > > > s->threads_number = 1; > > > > > > > > > > > > > > if (avctx->extradata_size > 0 && avctx->extradata) { > > > > > > > - ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size); > > > > > > > + ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1); > > > > > > > if (ret < 0) { > > > > > > > hevc_decode_free(avctx); > > > > > > > return ret; > > > > > > > > > > > > Couldn't that have done in a less confusing way? What the heck does > > > > > > "first" even mean? (Also you have to look up what that means on the > > > > > > caller site.) > > > > > > > > > > > > Can you explain this? > > > > > > > > > > first means "Preceding all others of a series or kind;" https://en.wiktionary.org/wiki/first > > > > > > > > Thanks for the English lesson. By the way, you're the only person on > > > > the internet who uses "iam". > > > > > > > > > > > > in the context of hevc_decode_extradata(), a function decoding extradata > > > > > that signifies the first of a series of extradata. > > > > > > > > > > No question it can be done differently, everything can be done > > > > > differently. > > > > > > > > So can you explain why "first" is checked every loop, instead of > > > > putting the loop into an if, or > > > > > > Every piece of code can be implemented im many ways. > > > Theres many equivalent and near equivalent ways to do it. > > > add a if(), add a condition to a existing if(), ... > > > some of these are a bit simpler, some are a bit faster, speedwise > > > theres nothing one can gain here as this executes too rarely > > > > Well this one has literally no advantage, and it's like it was written > > in this way just to be tricky. > > > > > > > > > > > > (what should have been done) moving the > > > > entire loop out of the function and putting it inline into the only > > > > place where it's effectively used? > > > > > > For a security fix i like to make sure it can be robustly backported. > > > If i add a "int first" the compiler makes sure that every call has > > > the parameter set. > > > If i move code around the compiler wouldnt notice if there was another > > > call. > > > > > > For git master, i agree moving the code is slightly nicer, ill post a > > > patch to make that change > > > > > > thx > > > > > > [...] > > > > I don't think the backport argument works either. > > > If it's a subtle > > security issue, you should probably do something more careful than > > cherry-pick + see if it compiles. > > yes absolutely but > Is it a subtle issue, noone has claimed it is nor that it isnt. And > where did i say that one would just do "cherry-pick + see if it compiles." > > I do like to use all tools available that help produce better code > with less effort. If i can use the compiler to check against some > issues, thats a good thing Sorry, that's a really really bad argument. If you were really concerned that there could have been a third caller, you could just have renamed the function to check this. Would have been just as much effort as adding an argument that nobody knows what it's supposed to mean, even if the argument name appears in an English dictionary. What I'm afraid is that such roundabout code is added in much more complex settings, where it isn't so obvious. Maybe that's why we have the mpegvideo mess today.
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c index cc8ac82164..55f51211c3 100644 --- a/libavcodec/hevcdec.c +++ b/libavcodec/hevcdec.c @@ -3057,7 +3057,7 @@ static int verify_md5(HEVCContext *s, AVFrame *frame) return 0; } -static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) +static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length, int first) { int ret, i; @@ -3069,7 +3069,7 @@ static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length) /* export stream parameters from the first SPS */ for (i = 0; i < FF_ARRAY_ELEMS(s->ps.sps_list); i++) { - if (s->ps.sps_list[i]) { + if (first && s->ps.sps_list[i]) { const HEVCSPS *sps = (const HEVCSPS*)s->ps.sps_list[i]->data; export_stream_params(s->avctx, &s->ps, sps); break; @@ -3099,7 +3099,7 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output, new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, &new_extradata_size); if (new_extradata && new_extradata_size > 0) { - ret = hevc_decode_extradata(s, new_extradata, new_extradata_size); + ret = hevc_decode_extradata(s, new_extradata, new_extradata_size, 0); if (ret < 0) return ret; } @@ -3387,7 +3387,7 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) s->threads_number = 1; if (avctx->extradata_size > 0 && avctx->extradata) { - ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size); + ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1); if (ret < 0) { hevc_decode_free(avctx); return ret;
Fixes: out of array access Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/hevcdec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)