Message ID | 20191206192214.30514-1-andriy.gelman@gmail.com |
---|---|
State | Accepted |
Commit | c07a77247363eb666a49536af505e7317225ee81 |
Headers | show |
On Fri, 06. Dec 14:22, Andriy Gelman wrote: > From: Andriy Gelman <andriy.gelman@gmail.com> > > payload_count is used to track the number of SEI payloads. It is also > used to free the SEIs in cbs_h264_free_sei()/cbs_h265_free_sei(). > > Currently, payload_count is set after for loop is completed. Hence if > there is an error and the function exits, the payload remains zero > causing a memleak. > > This commit keeps track of payload_count inside the for loop to fix the > issue. Note that that the contents of current are initialized with > av_mallocz() so there is no need to zero initialize payload_count. > > Found-by: libFuzzer > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > --- > libavcodec/cbs_h264_syntax_template.c | 2 +- > libavcodec/cbs_h265_syntax_template.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c > index 1671a15d330..878d348b948 100644 > --- a/libavcodec/cbs_h264_syntax_template.c > +++ b/libavcodec/cbs_h264_syntax_template.c > @@ -954,6 +954,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, > current->payload[k].payload_type = payload_type; > current->payload[k].payload_size = payload_size; > > + current->payload_count++; > CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); > > if (!cbs_h2645_read_more_rbsp_data(rw)) > @@ -964,7 +965,6 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, > "SEI message: found %d.\n", k); > return AVERROR_INVALIDDATA; > } > - current->payload_count = k + 1; > #else > for (k = 0; k < current->payload_count; k++) { > PutBitContext start_state; > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c > index 54570929ec7..15114548c60 100644 > --- a/libavcodec/cbs_h265_syntax_template.c > +++ b/libavcodec/cbs_h265_syntax_template.c > @@ -2184,6 +2184,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, > current->payload[k].payload_type = payload_type; > current->payload[k].payload_size = payload_size; > > + current->payload_count++; > CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k], prefix)); > > if (!cbs_h2645_read_more_rbsp_data(rw)) > @@ -2194,7 +2195,6 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, > "SEI message: found %d.\n", k); > return AVERROR_INVALIDDATA; > } > - current->payload_count = k + 1; > #else > for (k = 0; k < current->payload_count; k++) { > PutBitContext start_state; > -- > 2.24.0 > ping
On Fri, Dec 6, 2019 at 8:22 PM Andriy Gelman <andriy.gelman@gmail.com> wrote: > From: Andriy Gelman <andriy.gelman@gmail.com> > > payload_count is used to track the number of SEI payloads. It is also > used to free the SEIs in cbs_h264_free_sei()/cbs_h265_free_sei(). > > Currently, payload_count is set after for loop is completed. Hence if > there is an error and the function exits, the payload remains zero > causing a memleak. > > This commit keeps track of payload_count inside the for loop to fix the > issue. Note that that the contents of current are initialized with > av_mallocz() so there is no need to zero initialize payload_count. > > Found-by: libFuzzer > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > --- > libavcodec/cbs_h264_syntax_template.c | 2 +- > libavcodec/cbs_h265_syntax_template.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/cbs_h264_syntax_template.c > b/libavcodec/cbs_h264_syntax_template.c > index 1671a15d330..878d348b948 100644 > --- a/libavcodec/cbs_h264_syntax_template.c > +++ b/libavcodec/cbs_h264_syntax_template.c > @@ -954,6 +954,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, > RWContext *rw, > current->payload[k].payload_type = payload_type; > current->payload[k].payload_size = payload_size; > > + current->payload_count++; > CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); > > if (!cbs_h2645_read_more_rbsp_data(rw)) > @@ -964,7 +965,6 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, > RWContext *rw, > "SEI message: found %d.\n", k); > return AVERROR_INVALIDDATA; > } > - current->payload_count = k + 1; > #else > for (k = 0; k < current->payload_count; k++) { > PutBitContext start_state; > diff --git a/libavcodec/cbs_h265_syntax_template.c > b/libavcodec/cbs_h265_syntax_template.c > index 54570929ec7..15114548c60 100644 > --- a/libavcodec/cbs_h265_syntax_template.c > +++ b/libavcodec/cbs_h265_syntax_template.c > @@ -2184,6 +2184,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, > RWContext *rw, > current->payload[k].payload_type = payload_type; > current->payload[k].payload_size = payload_size; > > + current->payload_count++; > CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k], prefix)); > > if (!cbs_h2645_read_more_rbsp_data(rw)) > @@ -2194,7 +2195,6 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, > RWContext *rw, > "SEI message: found %d.\n", k); > return AVERROR_INVALIDDATA; > } > - current->payload_count = k + 1; > #else > for (k = 0; k < current->payload_count; k++) { > PutBitContext start_state; > -- > > LGTM. - Andreas
On 12/16/2019 12:00 PM, Andreas Rheinhardt wrote: > On Fri, Dec 6, 2019 at 8:22 PM Andriy Gelman <andriy.gelman@gmail.com> > wrote: > >> From: Andriy Gelman <andriy.gelman@gmail.com> >> >> payload_count is used to track the number of SEI payloads. It is also >> used to free the SEIs in cbs_h264_free_sei()/cbs_h265_free_sei(). >> >> Currently, payload_count is set after for loop is completed. Hence if >> there is an error and the function exits, the payload remains zero >> causing a memleak. >> >> This commit keeps track of payload_count inside the for loop to fix the >> issue. Note that that the contents of current are initialized with >> av_mallocz() so there is no need to zero initialize payload_count. >> >> Found-by: libFuzzer >> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> >> --- >> libavcodec/cbs_h264_syntax_template.c | 2 +- >> libavcodec/cbs_h265_syntax_template.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/cbs_h264_syntax_template.c >> b/libavcodec/cbs_h264_syntax_template.c >> index 1671a15d330..878d348b948 100644 >> --- a/libavcodec/cbs_h264_syntax_template.c >> +++ b/libavcodec/cbs_h264_syntax_template.c >> @@ -954,6 +954,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, >> RWContext *rw, >> current->payload[k].payload_type = payload_type; >> current->payload[k].payload_size = payload_size; >> >> + current->payload_count++; >> CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); >> >> if (!cbs_h2645_read_more_rbsp_data(rw)) >> @@ -964,7 +965,6 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, >> RWContext *rw, >> "SEI message: found %d.\n", k); >> return AVERROR_INVALIDDATA; >> } >> - current->payload_count = k + 1; >> #else >> for (k = 0; k < current->payload_count; k++) { >> PutBitContext start_state; >> diff --git a/libavcodec/cbs_h265_syntax_template.c >> b/libavcodec/cbs_h265_syntax_template.c >> index 54570929ec7..15114548c60 100644 >> --- a/libavcodec/cbs_h265_syntax_template.c >> +++ b/libavcodec/cbs_h265_syntax_template.c >> @@ -2184,6 +2184,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, >> RWContext *rw, >> current->payload[k].payload_type = payload_type; >> current->payload[k].payload_size = payload_size; >> >> + current->payload_count++; >> CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k], prefix)); >> >> if (!cbs_h2645_read_more_rbsp_data(rw)) >> @@ -2194,7 +2195,6 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, >> RWContext *rw, >> "SEI message: found %d.\n", k); >> return AVERROR_INVALIDDATA; >> } >> - current->payload_count = k + 1; >> #else >> for (k = 0; k < current->payload_count; k++) { >> PutBitContext start_state; >> -- >> >> > LGTM. > > - Andreas Applied.
diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c index 1671a15d330..878d348b948 100644 --- a/libavcodec/cbs_h264_syntax_template.c +++ b/libavcodec/cbs_h264_syntax_template.c @@ -954,6 +954,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, current->payload[k].payload_type = payload_type; current->payload[k].payload_size = payload_size; + current->payload_count++; CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); if (!cbs_h2645_read_more_rbsp_data(rw)) @@ -964,7 +965,6 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, "SEI message: found %d.\n", k); return AVERROR_INVALIDDATA; } - current->payload_count = k + 1; #else for (k = 0; k < current->payload_count; k++) { PutBitContext start_state; diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c index 54570929ec7..15114548c60 100644 --- a/libavcodec/cbs_h265_syntax_template.c +++ b/libavcodec/cbs_h265_syntax_template.c @@ -2184,6 +2184,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, current->payload[k].payload_type = payload_type; current->payload[k].payload_size = payload_size; + current->payload_count++; CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k], prefix)); if (!cbs_h2645_read_more_rbsp_data(rw)) @@ -2194,7 +2195,6 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, "SEI message: found %d.\n", k); return AVERROR_INVALIDDATA; } - current->payload_count = k + 1; #else for (k = 0; k < current->payload_count; k++) { PutBitContext start_state;