1451308 - Crash in nsClipboard::GetNativeDataOffClipboard

Status: RESOLVED DUPLICATE of bug 1513201

(Core :: Widget: Win32, defect)

Description

[[:philipp]]

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.

Comment 1

[Daniel Veditz [:dveditz]]

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.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 4

[[:philipp]]

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.

Comment 5

[Mats Palmgren (inactive)]

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

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

(ASAN crash is from bug 1513201)

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1451308 - handle pasted data of certain types with an odd length; r?mats

Comment 10

[Christian Holler (:decoder)]

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.