flvdec: Initialize the padding in new extradata side data

Message ID 20180201104427.79756-1-martin@martin.st
State Superseded
Headers show
Series
  • flvdec: Initialize the padding in new extradata side data
Related show

Commit Message

Martin Storsjö Feb. 1, 2018, 10:44 a.m.
We already allocate the internal extradata buffer with initialized
padding in flv_queue_extradata, but when copied into side data
(which is allocated by av_packet_new_side_data, which does allocate
the padding but doesn't initialize it), we forgot to initialize
the padding there.
---
 libavformat/flvdec.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Luca Barbato Feb. 1, 2018, 11:05 a.m. | #1
On 01/02/2018 11:44, Martin Storsjö wrote:
> We already allocate the internal extradata buffer with initialized
> padding in flv_queue_extradata, but when copied into side data
> (which is allocated by av_packet_new_side_data, which does allocate
> the padding but doesn't initialize it), we forgot to initialize
> the padding there.
> ---
>   libavformat/flvdec.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> index 81a71d39f4..23d320900b 100644
> --- a/libavformat/flvdec.c
> +++ b/libavformat/flvdec.c
> @@ -989,6 +989,10 @@ skip:
>           if (side) {
>               memcpy(side, flv->new_extradata[is_audio],
>                      flv->new_extradata_size[is_audio]);
> +            // av_packet_new_side_data allocates space for padding but doesn't
> +            // initialize it.
> +            memset(side + flv->new_extradata_size[is_audio], 0,
> +                   AV_INPUT_BUFFER_PADDING_SIZE);
>               av_freep(&flv->new_extradata[is_audio]);
>               flv->new_extradata_size[is_audio] = 0;
>           }
> 

Ok.
Martin Storsjö Feb. 1, 2018, 11:42 a.m. | #2
On Thu, 1 Feb 2018, Luca Barbato wrote:

> On 01/02/2018 11:44, Martin Storsjö wrote:
>> We already allocate the internal extradata buffer with initialized
>> padding in flv_queue_extradata, but when copied into side data
>> (which is allocated by av_packet_new_side_data, which does allocate
>> the padding but doesn't initialize it), we forgot to initialize
>> the padding there.
>> ---
>>   libavformat/flvdec.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>> 
>> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
>> index 81a71d39f4..23d320900b 100644
>> --- a/libavformat/flvdec.c
>> +++ b/libavformat/flvdec.c
>> @@ -989,6 +989,10 @@ skip:
>>           if (side) {
>>               memcpy(side, flv->new_extradata[is_audio],
>>                      flv->new_extradata_size[is_audio]);
>> +            // av_packet_new_side_data allocates space for padding but 
> doesn't
>> +            // initialize it.
>> +            memset(side + flv->new_extradata_size[is_audio], 0,
>> +                   AV_INPUT_BUFFER_PADDING_SIZE);
>>               av_freep(&flv->new_extradata[is_audio]);
>>               flv->new_extradata_size[is_audio] = 0;
>>           }
>> 
>
> Ok.

The other alternative would be to make av_packet_new_side_data 
zero-initialize the side data that it allocates (or at least the 
padding?), to avoid having to keep assumptions like these over here. (That 
would also match what ffmpeg does.) Would that perhaps be an even nicer 
solution, or does someone prefer keeping the allocation uninitialized like 
now?

// Martin
Luca Barbato Feb. 1, 2018, 12:47 p.m. | #3
On 01/02/2018 12:42, Martin Storsjö wrote:
> The other alternative would be to make av_packet_new_side_data 
> zero-initialize the side data that it allocates (or at least the 
> padding?), to avoid having to keep assumptions like these over here. 
> (That would also match what ffmpeg does.) Would that perhaps be an even 
> nicer solution, or does someone prefer keeping the allocation 
> uninitialized like now?

Probably shouldn't hurt and avoid some issues in the future.

lu

Patch

diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
index 81a71d39f4..23d320900b 100644
--- a/libavformat/flvdec.c
+++ b/libavformat/flvdec.c
@@ -989,6 +989,10 @@  skip:
         if (side) {
             memcpy(side, flv->new_extradata[is_audio],
                    flv->new_extradata_size[is_audio]);
+            // av_packet_new_side_data allocates space for padding but doesn't
+            // initialize it.
+            memset(side + flv->new_extradata_size[is_audio], 0,
+                   AV_INPUT_BUFFER_PADDING_SIZE);
             av_freep(&flv->new_extradata[is_audio]);
             flv->new_extradata_size[is_audio] = 0;
         }