[FFmpeg-devel] [PATCH] avutil: add av_memcpy() to avoid undefined behavior with NULL, NULL, 0

[Reimar Döffinger]

On 03.07.2019, at 08:29, Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Tue, Jul 02, 2019 at 08:42:43PM -0300, James Almer wrote:
>> On 7/2/2019 5:56 PM, Michael Niedermayer wrote:
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>> doc/APIchanges      |  3 +++
>>> libavutil/mem.h     | 13 +++++++++++++
>>> libavutil/version.h |  2 +-
>>> 3 files changed, 17 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index b5fadc2a48..65b8ed74ad 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>> 
>>> API changes, most recent first:
>>> 
>>> +2019-07-XX - XXXXXXXXXX - lavu 56.31.100 - mem.h
>>> +  Add av_memcpy()
>>> +
>>> 2019-06-21 - XXXXXXXXXX - lavu 56.30.100 - frame.h
>>>   Add FF_DECODE_ERROR_DECODE_SLICES
>>> 
>>> diff --git a/libavutil/mem.h b/libavutil/mem.h
>>> index 5fb1a02dd9..a35230360d 100644
>>> --- a/libavutil/mem.h
>>> +++ b/libavutil/mem.h
>>> @@ -506,6 +506,19 @@ void *av_memdup(const void *p, size_t size);
>>>  */
>>> void av_memcpy_backptr(uint8_t *dst, int back, int cnt);
>>> 
>>> +/**
>>> + * memcpy() implementation without a NULL pointer special case
>>> + *
>>> + * @param dst  Destination buffer
>>> + * @param src  Source buffer
>>> + * @param cnt  Number of bytes to copy; must be >= 0
>>> + */
>>> +static inline void av_memcpy(void *dst, const void *src, size_t cnt)
>> 
>> How many cases are there in the codebase where cnt can be 0, and dst or
>> src NULL, without it having been checked before calling memcpy? And from
>> those, which would not be from situations where the code should have
>> instead aborted and returned ENOMEM, or EINVAL if either of them are
>> function arguments?
> 
> There are around 2500 occurances of memcpy in the codebase
> To awnser your question it would be needed to review all of them and in many
> cases their surrounding code.
> So that is unlikely to be awnsered by anyone accuratly
> 
> Also iam not sure i understand why you ask or why this would matter
> the suggested function allows to simplify cases where the NULL can
> occur, not where it cannot or should not. That is this is intended for
> the cases where we already have or are adding explicit checks to
> avoid the NULL case.
> 
> i could rename this to av_memcpy_nullsafe which would make it clearer but
> also its more to write and read

I admit I thought that a worthwhile idea originally,
but I have to think back to a long time ago that every function added to our "API" has a cost of people having to know about it and how to use it.
And if it's currently only 2 places that would benefit I think James is right to ask if it makes sense.
Of course another question might be if it might make sense to replace all memcpy uses with it.
I mean, isn't it naturally expected behaviour that the pointers would be ignored if the copy amount is 0? There might be a lot of code assuming that we do not know about...