Message ID | 20191206173519.27098-1-andriy.gelman@gmail.com |
---|---|
State | Accepted |
Headers | show |
Andriy Gelman: > 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_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_h265_syntax_template.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c > index 54570929ec7..57fb2f3aa6a 100644 > --- a/libavcodec/cbs_h265_syntax_template.c > +++ b/libavcodec/cbs_h265_syntax_template.c > @@ -2167,6 +2167,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, > uint32_t payload_size = 0; > uint32_t tmp; > > + current->payload_count++; > while (show_bits(rw, 8) == 0xff) { > fixed(8, ff_byte, 0xff); > payload_type += 255; > @@ -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; > Good catch. But you should only increment payload_count directly before CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k], prefix)) -- there are after all no allocations performed before it. And cbs_h264 suffers from the same problem; it should probably be in the same commit. - Andreas
On Fri, 06. Dec 18:01, Andreas Rheinhardt wrote: > Andriy Gelman: > > 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_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_h265_syntax_template.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c > > index 54570929ec7..57fb2f3aa6a 100644 > > --- a/libavcodec/cbs_h265_syntax_template.c > > +++ b/libavcodec/cbs_h265_syntax_template.c > > @@ -2167,6 +2167,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, > > uint32_t payload_size = 0; > > uint32_t tmp; > > > > + current->payload_count++; > > while (show_bits(rw, 8) == 0xff) { > > fixed(8, ff_byte, 0xff); > > payload_type += 255; > > @@ -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; > > > Good catch. But you should only increment payload_count directly > before CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k], prefix)) > -- there are after all no allocations performed before it. > And cbs_h264 suffers from the same problem; it should probably be in > the same commit. > Thanks, I've made the changes and resent.
diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c index 54570929ec7..57fb2f3aa6a 100644 --- a/libavcodec/cbs_h265_syntax_template.c +++ b/libavcodec/cbs_h265_syntax_template.c @@ -2167,6 +2167,7 @@ static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, uint32_t payload_size = 0; uint32_t tmp; + current->payload_count++; while (show_bits(rw, 8) == 0xff) { fixed(8, ff_byte, 0xff); payload_type += 255; @@ -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;