[FFmpeg-devel] avcodec/extract_extradata: zero initialize the padding bytes of the exported extradata

Submitted by James Almer on March 8, 2018, 4:02 p.m.

Details

Message ID 20180308160208.2688-1-jamrial@gmail.com
State Accepted
Headers show

Commit Message

James Almer March 8, 2018, 4:02 p.m.
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/extract_extradata_bsf.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Derek Buitenhuis March 9, 2018, 3:18 p.m.
On 3/8/2018 4:02 PM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/extract_extradata_bsf.c | 4 ++++
>  1 file changed, 4 insertions(+)

Can't most (or all) of these be fixed by using av_mallocz?

- Derek
James Almer March 9, 2018, 3:22 p.m.
On 3/9/2018 12:18 PM, Derek Buitenhuis wrote:
> On 3/8/2018 4:02 PM, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/extract_extradata_bsf.c | 4 ++++
>>  1 file changed, 4 insertions(+)
> 
> Can't most (or all) of these be fixed by using av_mallocz?

Yes, but it's slower, and the buffer is guaranteed to be written to with
actual data after being allocated.

This is a filter that may run once per processed packet, so the less
overhead the better.
Derek Buitenhuis March 9, 2018, 3:47 p.m.
On 3/9/2018 3:22 PM, James Almer wrote:
> Yes, but it's slower, and the buffer is guaranteed to be written to with
> actual data after being allocated.
> 
> This is a filter that may run once per processed packet, so the less
> overhead the better.

Not sure I buy the "speed" argument here, but OK.

- Derek
James Almer March 9, 2018, 4:31 p.m.
On 3/9/2018 12:47 PM, Derek Buitenhuis wrote:
> On 3/9/2018 3:22 PM, James Almer wrote:
>> Yes, but it's slower, and the buffer is guaranteed to be written to with
>> actual data after being allocated.
>>
>> This is a filter that may run once per processed packet, so the less
>> overhead the better.
> 
> Not sure I buy the "speed" argument here, but OK.
> 
> - Derek

There's really nothing to win zeroing then immediately writing actual
data on top of it.

Pushed, thanks.
wm4 March 9, 2018, 8:52 p.m.
On Fri, 9 Mar 2018 13:31:45 -0300
James Almer <jamrial@gmail.com> wrote:

> On 3/9/2018 12:47 PM, Derek Buitenhuis wrote:
> > On 3/9/2018 3:22 PM, James Almer wrote:  
> >> Yes, but it's slower, and the buffer is guaranteed to be written to with
> >> actual data after being allocated.
> >>
> >> This is a filter that may run once per processed packet, so the less
> >> overhead the better.  
> > 
> > Not sure I buy the "speed" argument here, but OK.
> > 
> > - Derek  
> 
> There's really nothing to win zeroing then immediately writing actual
> data on top of it.
> 

Except code clarity and robustness?
Paul B Mahol March 9, 2018, 8:53 p.m.
On 3/9/18, wm4 <nfxjfg@googlemail.com> wrote:
> On Fri, 9 Mar 2018 13:31:45 -0300
> James Almer <jamrial@gmail.com> wrote:
>
>> On 3/9/2018 12:47 PM, Derek Buitenhuis wrote:
>> > On 3/9/2018 3:22 PM, James Almer wrote:
>> >> Yes, but it's slower, and the buffer is guaranteed to be written to
>> >> with
>> >> actual data after being allocated.
>> >>
>> >> This is a filter that may run once per processed packet, so the less
>> >> overhead the better.
>> >
>> > Not sure I buy the "speed" argument here, but OK.
>> >
>> > - Derek
>>
>> There's really nothing to win zeroing then immediately writing actual
>> data on top of it.
>>
>
> Except code clarity and robustness?

Code already is clear and robust enough.
Hendrik Leppkes March 9, 2018, 8:57 p.m.
On Fri, Mar 9, 2018 at 9:52 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Fri, 9 Mar 2018 13:31:45 -0300
> James Almer <jamrial@gmail.com> wrote:
>
>> On 3/9/2018 12:47 PM, Derek Buitenhuis wrote:
>> > On 3/9/2018 3:22 PM, James Almer wrote:
>> >> Yes, but it's slower, and the buffer is guaranteed to be written to with
>> >> actual data after being allocated.
>> >>
>> >> This is a filter that may run once per processed packet, so the less
>> >> overhead the better.
>> >
>> > Not sure I buy the "speed" argument here, but OK.
>> >
>> > - Derek
>>
>> There's really nothing to win zeroing then immediately writing actual
>> data on top of it.
>>
>
> Except code clarity and robustness?

We use this sort of pattern in all sorts of places where buffers are
read and padded. Its clear enough.

- Hendrik
wm4 March 9, 2018, 9:40 p.m.
On Fri, 9 Mar 2018 21:57:38 +0100
Hendrik Leppkes <h.leppkes@gmail.com> wrote:

> On Fri, Mar 9, 2018 at 9:52 PM, wm4 <nfxjfg@googlemail.com> wrote:
> > On Fri, 9 Mar 2018 13:31:45 -0300
> > James Almer <jamrial@gmail.com> wrote:
> >  
> >> On 3/9/2018 12:47 PM, Derek Buitenhuis wrote:  
> >> > On 3/9/2018 3:22 PM, James Almer wrote:  
> >> >> Yes, but it's slower, and the buffer is guaranteed to be written to with
> >> >> actual data after being allocated.
> >> >>
> >> >> This is a filter that may run once per processed packet, so the less
> >> >> overhead the better.  
> >> >
> >> > Not sure I buy the "speed" argument here, but OK.
> >> >
> >> > - Derek  
> >>
> >> There's really nothing to win zeroing then immediately writing actual
> >> data on top of it.
> >>  
> >
> > Except code clarity and robustness?  
> 
> We use this sort of pattern in all sorts of places where buffers are
> read and padded. Its clear enough.

Well, the fact that there are patches every once in a while that fix
places where this was not correctly done could hint otherwise. Not that
I want to make a huge discussion about it (though I always disliked
this padding requirement).

Patch hide | download patch | download mbox

diff --git a/libavcodec/extract_extradata_bsf.c b/libavcodec/extract_extradata_bsf.c
index d40907a675..0bffe8f42c 100644
--- a/libavcodec/extract_extradata_bsf.c
+++ b/libavcodec/extract_extradata_bsf.c
@@ -114,6 +114,7 @@  static int extract_extradata_h2645(AVBSFContext *ctx, AVPacket *pkt,
             ret = AVERROR(ENOMEM);
             goto fail;
         }
+        memset(extradata + extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
 
         *data = extradata;
         *size = extradata_size;
@@ -169,6 +170,7 @@  static int extract_extradata_vc1(AVBSFContext *ctx, AVPacket *pkt,
             return AVERROR(ENOMEM);
 
         memcpy(*data, pkt->data, extradata_size);
+        memset(*data + extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
         *size = extradata_size;
 
         if (s->remove) {
@@ -199,6 +201,7 @@  static int extract_extradata_mpeg12(AVBSFContext *ctx, AVPacket *pkt,
                     return AVERROR(ENOMEM);
 
                 memcpy(*data, pkt->data, *size);
+                memset(*data + *size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
 
                 if (s->remove) {
                     pkt->data += *size;
@@ -228,6 +231,7 @@  static int extract_extradata_mpeg4(AVBSFContext *ctx, AVPacket *pkt,
                     return AVERROR(ENOMEM);
 
                 memcpy(*data, pkt->data, *size);
+                memset(*data + *size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
 
                 if (s->remove) {
                     pkt->data += *size;