Bug 230140 - Use of memcpy with overlapping memory pointers
Summary: Use of memcpy with overlapping memory pointers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-09 17:33 PDT by Jean-Yves Avenard [:jya]
Modified: 2021-09-12 17:49 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.03 KB, patch)
2021-09-09 17:56 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Yves Avenard [:jya] 2021-09-09 17:33:47 PDT
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.
Comment 1 Jean-Yves Avenard [:jya] 2021-09-09 17:33:55 PDT
rdar://82946555
Comment 2 Jean-Yves Avenard [:jya] 2021-09-09 17:56:05 PDT
Created attachment 437814 [details]
Patch
Comment 3 David Kilzer (:ddkilzer) 2021-09-10 02:14:36 PDT
Comment on attachment 437814 [details]
Patch

r=me
Comment 4 EWS 2021-09-10 03:23:38 PDT
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].
Comment 5 Darin Adler 2021-09-10 11:19:30 PDT
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?
Comment 6 Darin Adler 2021-09-10 11:20:52 PDT
(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.
Comment 7 Darin Adler 2021-09-10 12:18:10 PDT
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".
Comment 8 David Kilzer (:ddkilzer) 2021-09-11 00:11:05 PDT
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.
Comment 9 Darin Adler 2021-09-12 16:45:30 PDT
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.
Comment 10 Jean-Yves Avenard [:jya] 2021-09-12 17:49:09 PDT
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.