230140 2021-09-09 17:33:47 -0700 Use of memcpy with overlapping memory pointers 2021-09-12 17:49:09 -0700 1 1 1 Unclassified WebKit Media Other Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jean-yves.avenard jean-yves.avenard darin ddkilzer eric.carlson ews-watchlist glenn jer.noble philipj sergio webkit-bug-importer oldest_to_newest 1792182 0 jean-yves.avenard 2021-09-09 17:33:47 -0700 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. 1792183 1 jean-yves.avenard 2021-09-09 17:33:55 -0700 rdar://82946555 1792190 2 437814 jean-yves.avenard 2021-09-09 17:56:05 -0700 Created attachment 437814 Patch 1792280 3 437814 ddkilzer 2021-09-10 02:14:36 -0700 Comment on attachment 437814 Patch r=me 1792285 4 ews-feeder 2021-09-10 03:23:38 -0700 Committed r282263 (241541@main): <https://commits.webkit.org/241541@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 437814. 1792454 5 437814 darin 2021-09-10 11:19:30 -0700 Comment on attachment 437814 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? 1792455 6 darin 2021-09-10 11:20:52 -0700 (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. 1792484 7 437814 darin 2021-09-10 12:18:10 -0700 Comment on attachment 437814 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". 1792661 8 437814 ddkilzer 2021-09-11 00:11:05 -0700 Comment on attachment 437814 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. 1792819 9 437814 darin 2021-09-12 16:45:30 -0700 Comment on attachment 437814 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. 1792821 10 437814 jean-yves.avenard 2021-09-12 17:49:09 -0700 Comment on attachment 437814 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. 437814 2021-09-09 17:56:05 -0700 2021-09-10 03:23:39 -0700 Patch bug-230140-20210910105604.patch text/plain 2074 jean-yves.avenard U3VidmVyc2lvbiBSZXZpc2lvbjogMjgyMTk2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjMyYTBhNjVlOTFmODkx MjllMmFjNWEzZGQ5Y2VmNzgxNzYzMGJlNy4uYTM2ODI4NzYzYTc3OTRjOTc4MmY5MGE1Y2YwN2Rl MzI1YmNkMTUxNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDIxLTA5LTA5ICBKZWFu LVl2ZXMgQXZlbmFyZCAgPGp5YUBhcHBsZS5jb20+CisKKyAgICAgICAgVXNlIG9mIG1lbWNweSB3 aXRoIG92ZXJsYXBwaW5nIG1lbW9yeSBwb2ludGVycworICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjMwMTQwCisgICAgICAgIHJkYXI6Ly84Mjk0NjU1NQor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdlIHVzZSBt ZW1jcHkgd2l0aCBvdmVybGFwcGluZyBwb2ludGVycyB3aGljaCB0cmlnZ2VycyBBc2FuLiBJbiBw cmFjdGljZSwKKyAgICAgICAgd2l0aCBob3cgbWVtY3B5IHdhcyB1c2VkIHRoZSBiZWhhdmlvdXIg d2Fzbid0IHVuZGVmaW5lZCBhbmQgc28gd291bGQgaGF2ZQorICAgICAgICBiZWVuIGZpbmUuCisg ICAgICAgIEFscmVhZHkgY292ZXJlZCBieSBleGlzdGluZyB0ZXN0cy4KKworICAgICAgICAqIHBs YXRmb3JtL2F1ZGlvL2NvY29hL0F1ZGlvRmlsZVJlYWRlckNvY29hLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OkF1ZGlvRmlsZVJlYWRlcjo6ZGVjb2RlV2ViTURhdGEgY29uc3QpOiBSZXBsYWNlIG1l bWNweSB3aXRoIG1lbW1vdmUKKwogMjAyMS0wOS0wNyAgSmVhbi1ZdmVzIEF2ZW5hcmQgIDxqeWFA YXBwbGUuY29tPgogCiAgICAgICAgIERlY29kaW5nIHdlYm0gZmlsZSB3aXRoIG1vcmUgdGhhbiAy IGNoYW5uZWxzIHdpbGwgZmFpbApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v YXVkaW8vY29jb2EvQXVkaW9GaWxlUmVhZGVyQ29jb2EuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vYXVkaW8vY29jb2EvQXVkaW9GaWxlUmVhZGVyQ29jb2EuY3BwCmluZGV4IDhiMzhkMTE3 YjMxZDI5ZmE4ODIxYmM4ZDFhMzY3MmIzYjM3NDhkMGUuLjFiMGQ0OGUzOWE4YzdiN2JjYTJlOWRi MzQyNmE5MTQ4YTA2OGFlOGUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2F1 ZGlvL2NvY29hL0F1ZGlvRmlsZVJlYWRlckNvY29hLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS9hdWRpby9jb2NvYS9BdWRpb0ZpbGVSZWFkZXJDb2NvYS5jcHAKQEAgLTM5Miw3ICsz OTIsNyBAQCBzdGQ6Om9wdGlvbmFsPHNpemVfdD4gQXVkaW9GaWxlUmVhZGVyOjpkZWNvZGVXZWJN RGF0YShBdWRpb0J1ZmZlckxpc3QmIGJ1ZmZlckxpcwogICAgICAgICAgICAgaWYgKGxlYWRpbmdU cmltID4gMCkgewogICAgICAgICAgICAgICAgIFVJbnQzMiB0b1RyaW0gPSBzdGQ6Om1pbihsZWFk aW5nVHJpbSwgbnVtRnJhbWVzKTsKICAgICAgICAgICAgICAgICBmb3IgKFVJbnQzMiBpID0gMDsg aSA8IG91dEZvcm1hdC5tQ2hhbm5lbHNQZXJGcmFtZTsgaSsrKQotICAgICAgICAgICAgICAgICAg ICBtZW1jcHkoZGVjb2RlZEJ1ZmZlckxpc3QtPm1CdWZmZXJzW2ldLm1EYXRhLCBzdGF0aWNfY2Fz dDxmbG9hdCo+KGRlY29kZWRCdWZmZXJMaXN0LT5tQnVmZmVyc1tpXS5tRGF0YSkgKyB0b1RyaW0s IChudW1GcmFtZXMgLSB0b1RyaW0pICogc2l6ZW9mKGZsb2F0KSk7CisgICAgICAgICAgICAgICAg ICAgIG1lbW1vdmUoZGVjb2RlZEJ1ZmZlckxpc3QtPm1CdWZmZXJzW2ldLm1EYXRhLCBzdGF0aWNf Y2FzdDxmbG9hdCo+KGRlY29kZWRCdWZmZXJMaXN0LT5tQnVmZmVyc1tpXS5tRGF0YSkgKyB0b1Ry aW0sIChudW1GcmFtZXMgLSB0b1RyaW0pICogc2l6ZW9mKGZsb2F0KSk7CiAgICAgICAgICAgICAg ICAgbGVhZGluZ1RyaW0gLT0gdG9UcmltOwogICAgICAgICAgICAgICAgIG51bUZyYW1lcyAtPSB0 b1RyaW07CiAgICAgICAgICAgICB9Cg==