Bug 230140 – Use of memcpy with overlapping memory pointers

• Component: Media
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

Problem introduced in bug 229251; the memory buffer content is shifted to the left; the use of memcpy under these circumstances is fine. But it triggers Asan "AddressSanitizer detected: memcpy-param-overlap"

Should change memcpy to memmove here to keep asan happy.

[Jean-Yves Avenard [:jya]]

rdar://82946555

[Jean-Yves Avenard [:jya]]

Created attachment 437814 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 437814 [details]
Patch

r=me

[EWS]

Committed r282263 (241541@main): <https://commits.webkit.org/241541@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437814 [details].

[Darin Adler]

Comment on attachment 437814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437814&action=review

> Source/WebCore/ChangeLog:11
> +        We use memcpy with overlapping pointers which triggers Asan. In practice,
> +        with how memcpy was used the behaviour wasn't undefined and so would have
> +        been fine.

I don’t understand the comment that memcpy would have been fine? Could you elaborate, just to satisfy my curiosity?

[Darin Adler]

(In reply to Jean-Yves Avenard [:jya] from comment #0)
> the memory buffer content is shifted to
> the left; the use of memcpy under these circumstances is fine

I see now: You are saying that typically memcpy works fine as long as you are moving left. Never thought of it that way, but I suppose as a practical matter that is so, but I am glad we fixed this because I don’t think it is OK to rely on that.

[Darin Adler]

Comment on attachment 437814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437814&action=review

> Source/WebCore/ChangeLog:10
> +        with how memcpy was used the behaviour wasn't undefined and so would have

What you probably should have said is that even though technically the behavior was undefined, in practice real world implementations of memcpy work with overlapping ranges as long as you are moving "left".

[David Kilzer (:ddkilzer)]

Comment on attachment 437814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437814&action=review

>> Source/WebCore/ChangeLog:11
>> +        been fine.
> 
> I don’t understand the comment that memcpy would have been fine? Could you elaborate, just to satisfy my curiosity?

On Darwin platforms, memcpy() is implemented identically to memmove(), so there is no undefined behavior for overlapping regions.

This is not true on other platforms like Linux, though.

[Darin Adler]

Comment on attachment 437814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437814&action=review

>>> Source/WebCore/ChangeLog:11
>>> +        been fine.
>> 
>> I don’t understand the comment that memcpy would have been fine? Could you elaborate, just to satisfy my curiosity?
> 
> On Darwin platforms, memcpy() is implemented identically to memmove(), so there is no undefined behavior for overlapping regions.
> 
> This is not true on other platforms like Linux, though.

Let’s not quibble, but "undefined behavior" is not about what the functions actually do in practice, but about what the C standard guarantees they will do.

[Jean-Yves Avenard [:jya]]

Comment on attachment 437814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437814&action=review

>>>> Source/WebCore/ChangeLog:11
>>>> +        been fine.
>>> 
>>> I don’t understand the comment that memcpy would have been fine? Could you elaborate, just to satisfy my curiosity?
>> 
>> On Darwin platforms, memcpy() is implemented identically to memmove(), so there is no undefined behavior for overlapping regions.
>> 
>> This is not true on other platforms like Linux, though.
> 
> Let’s not quibble, but "undefined behavior" is not about what the functions actually do in practice, but about what the C standard guarantees they will do.

undefined behaviour is the easiest explanation that could be given if the user isn't paying attention on this matter.
I was merely pointing that there was no logic flaw in the original code: that is the end result would be wrong with rubbish data. Considering:
1- the code is only on mac, where memmove is always used
2- I'm not aware of any libc's memcpy implementation or any compiler optimisation on any supported platforms where copying when dst < src would cause unexpected results. memmove == memcpy whenever dst < src. glibc calls memcpy explicitly  from memmove if that is so.

When I wrote the comment, my concern was that the code could be dangerous or produce rubbish, and I wrote it with that concern in mind. I'd be more explicit next time. sorry for that.