[FFmpeg-devel,05/11] avformat/nutenc: Don't pass NULL to memcmp

Submitted by Andreas Rheinhardt on Sept. 19, 2019, 10:17 p.m.

Details

Message ID 20190919221706.16529-6-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt Sept. 19, 2019, 10:17 p.m.
Fixes lots of FATE tests, e.g. lavf-nut, as well as the nut part of
ticket #7980.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
This patch is made to match the previous behaviour; whether the previous
behaviour is correct at all if the header length is zero is unknown to
me.

 libavformat/nutenc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Carl Eugen Hoyos Sept. 19, 2019, 11:22 p.m.
Am Fr., 20. Sept. 2019 um 00:30 Uhr schrieb Andreas Rheinhardt
<andreas.rheinhardt@gmail.com>:
>
> Fixes lots of FATE tests, e.g. lavf-nut, as well as the nut part of
> ticket #7980.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> This patch is made to match the previous behaviour; whether the previous
> behaviour is correct at all if the header length is zero is unknown to
> me.
>
>  libavformat/nutenc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c
> index e9a3bb49db..dc714eb809 100644
> --- a/libavformat/nutenc.c
> +++ b/libavformat/nutenc.c
> @@ -791,8 +791,9 @@ static int get_needed_flags(NUTContext *nut, StreamContext *nus, FrameCode *fc,
>          flags |= FLAG_CHECKSUM;
>      if (pkt->size < nut->header_len[fc->header_idx] ||
>          (pkt->size > 4096 && fc->header_idx)        ||
> -        memcmp(pkt->data, nut->header[fc->header_idx],
> -               nut->header_len[fc->header_idx]))
> +        (nut->header_len[fc->header_idx] > 0 &&
> +         memcmp(pkt->data, nut->header[fc->header_idx],
> +                nut->header_len[fc->header_idx])))
>          flags |= FLAG_HEADER_IDX;

Is this different from the patch I sent?
(Why does it look different)
https://patchwork.ffmpeg.org/patch/13782/

Carl Eugen
Andreas Rheinhardt Oct. 21, 2019, 9:01 a.m.
Carl Eugen Hoyos:
> Am Fr., 20. Sept. 2019 um 00:30 Uhr schrieb Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com>:
>>
>> Fixes lots of FATE tests, e.g. lavf-nut, as well as the nut part of
>> ticket #7980.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> This patch is made to match the previous behaviour; whether the previous
>> behaviour is correct at all if the header length is zero is unknown to
>> me.
>>
>>  libavformat/nutenc.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c
>> index e9a3bb49db..dc714eb809 100644
>> --- a/libavformat/nutenc.c
>> +++ b/libavformat/nutenc.c
>> @@ -791,8 +791,9 @@ static int get_needed_flags(NUTContext *nut, StreamContext *nus, FrameCode *fc,
>>          flags |= FLAG_CHECKSUM;
>>      if (pkt->size < nut->header_len[fc->header_idx] ||
>>          (pkt->size > 4096 && fc->header_idx)        ||
>> -        memcmp(pkt->data, nut->header[fc->header_idx],
>> -               nut->header_len[fc->header_idx]))
>> +        (nut->header_len[fc->header_idx] > 0 &&
>> +         memcmp(pkt->data, nut->header[fc->header_idx],
>> +                nut->header_len[fc->header_idx])))
>>          flags |= FLAG_HEADER_IDX;
> 
> Is this different from the patch I sent?
> (Why does it look different)
> https://patchwork.ffmpeg.org/patch/13782/
> 
> Carl Eugen

Yes.
a) Your patch checks whether nut->header[fc->header_idx] is NULL.
b) Michael's proposal is to check for fc->header_idx instead. This
would work, because it is the index of the only used pointer in
nut->header that is NULL, and this proposal has the advantage of one
dereferencing less than a).
c) My* patch checks whether nut->header_len[fc->header_idx] is zero.
Given that nut->header_len[fc->header_idx] is already used in the very
first check, my solution should not entail a dereference at all.

Furthermore, b) is very specific to the nutenc situation here and a)
might hide a situation where nut->header_len[fc->header_idx] is > 0,
but nut->header[fc->header_idx] == NULL, which should of course not
happen. And it singles out one of the two pointer arguments.

- Andreas

*: It is also what Reimar proposed (I was completely unaware of the
earlier patch when I submitted mine).
Andreas Rheinhardt Oct. 31, 2019, 7:38 a.m.
Andreas Rheinhardt:
> Carl Eugen Hoyos:
>> Am Fr., 20. Sept. 2019 um 00:30 Uhr schrieb Andreas Rheinhardt
>> <andreas.rheinhardt@gmail.com>:
>>>
>>> Fixes lots of FATE tests, e.g. lavf-nut, as well as the nut part of
>>> ticket #7980.
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>> ---
>>> This patch is made to match the previous behaviour; whether the previous
>>> behaviour is correct at all if the header length is zero is unknown to
>>> me.
>>>
>>>  libavformat/nutenc.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c
>>> index e9a3bb49db..dc714eb809 100644
>>> --- a/libavformat/nutenc.c
>>> +++ b/libavformat/nutenc.c
>>> @@ -791,8 +791,9 @@ static int get_needed_flags(NUTContext *nut, StreamContext *nus, FrameCode *fc,
>>>          flags |= FLAG_CHECKSUM;
>>>      if (pkt->size < nut->header_len[fc->header_idx] ||
>>>          (pkt->size > 4096 && fc->header_idx)        ||
>>> -        memcmp(pkt->data, nut->header[fc->header_idx],
>>> -               nut->header_len[fc->header_idx]))
>>> +        (nut->header_len[fc->header_idx] > 0 &&
>>> +         memcmp(pkt->data, nut->header[fc->header_idx],
>>> +                nut->header_len[fc->header_idx])))
>>>          flags |= FLAG_HEADER_IDX;
>>
>> Is this different from the patch I sent?
>> (Why does it look different)
>> https://patchwork.ffmpeg.org/patch/13782/
>>
>> Carl Eugen
> 
> Yes.
> a) Your patch checks whether nut->header[fc->header_idx] is NULL.
> b) Michael's proposal is to check for fc->header_idx instead. This
> would work, because it is the index of the only used pointer in
> nut->header that is NULL, and this proposal has the advantage of one
> dereferencing less than a).
> c) My* patch checks whether nut->header_len[fc->header_idx] is zero.
> Given that nut->header_len[fc->header_idx] is already used in the very
> first check, my solution should not entail a dereference at all.
> 
> Furthermore, b) is very specific to the nutenc situation here and a)
> might hide a situation where nut->header_len[fc->header_idx] is > 0,
> but nut->header[fc->header_idx] == NULL, which should of course not
> happen. And it singles out one of the two pointer arguments.
> 
> - Andreas
> 
> *: It is also what Reimar proposed (I was completely unaware of the
> earlier patch when I submitted mine).
> 
Ping.

- Andreas
Michael Niedermayer Nov. 1, 2019, 9:16 a.m.
On Thu, Oct 31, 2019 at 07:38:00AM +0000, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
> > Carl Eugen Hoyos:
> >> Am Fr., 20. Sept. 2019 um 00:30 Uhr schrieb Andreas Rheinhardt
> >> <andreas.rheinhardt@gmail.com>:
> >>>
> >>> Fixes lots of FATE tests, e.g. lavf-nut, as well as the nut part of
> >>> ticket #7980.
> >>>
> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >>> ---
> >>> This patch is made to match the previous behaviour; whether the previous
> >>> behaviour is correct at all if the header length is zero is unknown to
> >>> me.
> >>>
> >>>  libavformat/nutenc.c | 5 +++--
> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c
> >>> index e9a3bb49db..dc714eb809 100644
> >>> --- a/libavformat/nutenc.c
> >>> +++ b/libavformat/nutenc.c
> >>> @@ -791,8 +791,9 @@ static int get_needed_flags(NUTContext *nut, StreamContext *nus, FrameCode *fc,
> >>>          flags |= FLAG_CHECKSUM;
> >>>      if (pkt->size < nut->header_len[fc->header_idx] ||
> >>>          (pkt->size > 4096 && fc->header_idx)        ||
> >>> -        memcmp(pkt->data, nut->header[fc->header_idx],
> >>> -               nut->header_len[fc->header_idx]))
> >>> +        (nut->header_len[fc->header_idx] > 0 &&
> >>> +         memcmp(pkt->data, nut->header[fc->header_idx],
> >>> +                nut->header_len[fc->header_idx])))
> >>>          flags |= FLAG_HEADER_IDX;
> >>
> >> Is this different from the patch I sent?
> >> (Why does it look different)
> >> https://patchwork.ffmpeg.org/patch/13782/
> >>
> >> Carl Eugen
> > 
> > Yes.
> > a) Your patch checks whether nut->header[fc->header_idx] is NULL.
> > b) Michael's proposal is to check for fc->header_idx instead. This
> > would work, because it is the index of the only used pointer in
> > nut->header that is NULL, and this proposal has the advantage of one
> > dereferencing less than a).
> > c) My* patch checks whether nut->header_len[fc->header_idx] is zero.
> > Given that nut->header_len[fc->header_idx] is already used in the very
> > first check, my solution should not entail a dereference at all.
> > 
> > Furthermore, b) is very specific to the nutenc situation here and a)
> > might hide a situation where nut->header_len[fc->header_idx] is > 0,
> > but nut->header[fc->header_idx] == NULL, which should of course not
> > happen. And it singles out one of the two pointer arguments.
> > 
> > - Andreas
> > 
> > *: It is also what Reimar proposed (I was completely unaware of the
> > earlier patch when I submitted mine).
> > 
> Ping.

I dont think the resulting code is particularly readable with the
increasingly complex condition in the if()

ill post a alternative suggestion for consideration that has a simpler
condition after testing it

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c
index e9a3bb49db..dc714eb809 100644
--- a/libavformat/nutenc.c
+++ b/libavformat/nutenc.c
@@ -791,8 +791,9 @@  static int get_needed_flags(NUTContext *nut, StreamContext *nus, FrameCode *fc,
         flags |= FLAG_CHECKSUM;
     if (pkt->size < nut->header_len[fc->header_idx] ||
         (pkt->size > 4096 && fc->header_idx)        ||
-        memcmp(pkt->data, nut->header[fc->header_idx],
-               nut->header_len[fc->header_idx]))
+        (nut->header_len[fc->header_idx] > 0 &&
+         memcmp(pkt->data, nut->header[fc->header_idx],
+                nut->header_len[fc->header_idx])))
         flags |= FLAG_HEADER_IDX;
 
     return flags | (fc->flags & FLAG_CODED);