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

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Carl Eugen Hoyos:
>> Am Fr., 20. Sept. 2019 um 00:30 Uhr schrieb Andreas Rheinhardt
>> <andreas.rheinhardt at 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 at 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