[FFmpeg-devel] [PATCH] avformat/utils: Fix undefined NULL + 0

[Andreas Rheinhardt]

James Almer:
> On 2/14/2021 6:28 PM, Andreas Rheinhardt wrote:
>> This is undefined behaviour in C, so use data = len ? data + len : data
>> instead if data += len. GCC optimizes the branch away in this case;
>> Clang unfortunately doesn't.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>> Checking for len != 0 instead of > 0 allows the compiler to optimize the
>> branch away; maybe future versions of Clang (I used 11) will do so, too.
>>
>>   libavformat/utils.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>> index 3e955b85bc..cea6d4ca92 100644
>> --- a/libavformat/utils.c
>> +++ b/libavformat/utils.c
>> @@ -1426,7 +1426,7 @@ static int parse_packet(AVFormatContext *s,
>> AVPacket *pkt,
>>           pkt->pts = pkt->dts = AV_NOPTS_VALUE;
>>           pkt->pos = -1;
>>           /* increment read pointer */
>> -        data += len;
>> +        data  = len ? data + len : data;
> 
> Isn't adding any offset to a NULL pointer UB? That was afaik the entire
> point behind the change in c40d36076a.
> 
Yes, it is. And if it happened here, it would indicate a real bug (i.e.
a real logic error) that would need to be fixed; but as long as it
doesn't happen, we should apply cheap solutions that the compiler can
optimize away (you probably notice that I regard NULL + 0 as not a real
bug, but rather as an annoyance of the C standard that disallows this
(even the sanitizer authors seem to agree with this point (that's why
there is an error message specifically for NULL + 0); it is allowed in C++).

- Andreas