diff mbox

[FFmpeg-devel,v2] lavc/cbs_h2645_syntax_template: Fix memleak

Message ID 20191206192214.30514-1-andriy.gelman@gmail.com
State Accepted
Commit c07a77247363eb666a49536af505e7317225ee81
Headers show

Commit Message

Andriy Gelman Dec. 6, 2019, 7:22 p.m. UTC 
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(-)

Comments

Andriy Gelman Dec. 16, 2019, 4:18 a.m. UTC | #1 
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, &current->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, &current->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
Andreas Rheinhardt Dec. 16, 2019, 3 p.m. UTC | #2 
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, &current->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, &current->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
James Almer Dec. 16, 2019, 3:18 p.m. UTC | #3 
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, &current->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, &current->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 mbox

Patch

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, &current->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, &current->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;