Closed Bug 1451308 Opened 3 years ago Closed 2 years ago
Crash in ns
Clipboard::Get Native Data Off Clipboard
This bug was filed from the Socorro interface and is report bp-9656c64a-571d-44f6-920c-13a700180404. ============================================================= Top 10 frames of crashing thread: 0 xul.dll nsClipboard::GetNativeDataOffClipboard widget/windows/nsClipboard.cpp:560 1 xul.dll nsClipboard::GetDataFromDataObject widget/windows/nsClipboard.cpp:627 2 xul.dll nsClipboard::GetNativeClipboardData widget/windows/nsClipboard.cpp:1002 3 xul.dll nsBaseClipboard::GetData widget/nsBaseClipboard.cpp:77 4 xul.dll mozilla::dom::ContentParent::RecvGetClipboard dom/ipc/ContentParent.cpp:2648 5 xul.dll mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:8024 6 xul.dll mozilla::ipc::MessageChannel::DispatchSyncMessage ipc/glue/MessageChannel.cpp:2101 7 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2059 8 xul.dll mozilla::ipc::MessageChannel::RunMessage ipc/glue/MessageChannel.cpp:1909 9 xul.dll mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:1942 ============================================================= these crash reports on windows seem to have become a bit more common after firefox 58 - overall they are still fairly low in volume though.
Looks like maybe we're getting non-null-terminated data and the strlen is running off the end? If we found a random null in our own memory before hitting an access violation we might then leak memory contents.
I'm not sure I understand why we're taking a strlen of |aData|, GetGlobalData writes a length for the value to a return-argument.
bug 1461739 describes some steps that led to the crash. 35% of the crashes come from zh-cn builds, so perhaps there's content that's more prone to crashing there.
GetGlobalData always returns a null-terminated buffer though: https://hg.mozilla.org/releases/mozilla-beta/annotate/d2564a0c25e5084dbd064cf546d98a9f72eed1f3/widget/windows/nsClipboard.cpp#l303 I'm guessing from the code comments that the intention of the NS_strlen is to catch any embedded NULs in the actual data preceding that? (I don't know why that's desirable when we don't know what kind of data we're dealing with though. It seems bogus if it's some kind of binary data for example.) In theory, GlobalSize could return an odd number and then we would write the two zero bytes misaligned to the start when interpreted as char16_t so that NS_strlen doesn't find it. A possible fix for that would be to allocate/write three extra zeroed bytes when allocSize is odd, instead of just two. Alternatively, as Alex suggests, replace the NS_strlen call with "*aLen = allocLen;". Unrelated issue, but we might want to fix it while we're here: if malloc() fails then we miss to call GlobalUnlock(aHGBL): https://hg.mozilla.org/releases/mozilla-beta/annotate/d2564a0c25e5084dbd064cf546d98a9f72eed1f3/widget/windows/nsClipboard.cpp#l305
With the ASAN crash in hand, we now that the issue is GlobalSize returning an odd number: "0x11ede48c83e3 is located 0 bytes to the right of 419-byte region [0x11ede48c8240,0x11ede48c83e3)". Simply padding by 3 zero bytes, instead of 2 seems like the easiest fix.
(ASAN crash is from bug 1513201)
Duping this to the ASan bug after talking Alex. According to him, the ASan trace provided missing information to fix this issue properly and he would like to make it possible for a small bounty to be considered for that reason. The patch will move over to bug 1513201 as well.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1513201
You need to log in before you can comment on or make changes to this bug.