1824644 - Crash in [@ nsClipboard::SaveIStorage]

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

First reports from the 20230322211554 nightly.

Crash report: https://crash-stats.mozilla.org/report/index/fe9ab1b3-4ba6-4333-9ed8-657170230324

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  nsClipboard::SaveIStorage  widget/windows/nsClipboard.cpp:1427
1  xul.dll  nsClipboard::GetNativeDataOffClipboard  widget/windows/nsClipboard.cpp:817
2  xul.dll  nsClipboard::FindURLFromLocalFile  widget/windows/nsClipboard.cpp:1157
3  xul.dll  nsClipboard::GetDataFromDataObject  widget/windows/nsClipboard.cpp:953
4  xul.dll  nsClipboard::GetNativeClipboardData  widget/windows/nsClipboard.cpp:1330
5  xul.dll  nsBaseClipboard::GetData  widget/nsBaseClipboard.cpp:89
6  xul.dll  XPTC__InvokebyIndex  
7  xul.dll  NS_InvokeByIndex  xpcom/reflect/xptcall/md/win32/xptcinvoke_x86_64.cpp:57
7  xul.dll  CallMethodHelper::Invoke  js/xpconnect/src/XPCWrappedNative.cpp:1626
7  xul.dll  CallMethodHelper::Call  js/xpconnect/src/XPCWrappedNative.cpp:1179

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Timing fits this being a regression from bug 580928.

Comment 2

[marco.spiess]

I suspect the bug hides somewhere in the following snippet:

auto releaseMediumGuard = MakeScopeExit([&] { ReleaseStgMedium(&stm); });
RefPtr<IStorage> file;
hres = StgCreateStorageEx(
    aFileName.Data(), STGM_CREATE | STGM_READWRITE | STGM_SHARE_EXCLUSIVE,
    STGFMT_STORAGE, 0, NULL, NULL, IID_IStorage, getter_AddRefs(file));
if (FAILED(hres)) {
  if (hres == E_OUTOFMEMORY) {
    return NS_ERROR_OUT_OF_MEMORY;
  }
  return NS_ERROR_FAILURE;
}

hres = stm.pstg->CopyTo(0, NULL, NULL, file);
if (FAILED(hres)) {
  if (hres == STG_E_INSUFFICIENTMEMORY) {
    return NS_ERROR_OUT_OF_MEMORY;
  }
  return NS_ERROR_FAILURE;
}

file->Commit(STGC_DEFAULT);

The top of the stack is the last line.

The low crash address (0x0000000000000048) could mean that file is a null pointer.
However, I don't know how it could end up null without having StgCreateStorageEx and stm.pstg->CopyTo return failures.

Comment 3

[Ray Kraesig [:rkraesig]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #0)

Crash report: https://crash-stats.mozilla.org/report/index/fe9ab1b3-4ba6-4333-9ed8-657170230324

Per the minidump, these three lines are the source of the crash:

HRESULT hres = aDataObject->GetData(&fe, &stm);
...
hres = stm.pstg->CopyTo(0, NULL, NULL, file);
...
file->Commit(STGC_DEFAULT);

The GetData call succeeds, storing a STGMEDIUM tagged-union into stm. The tag is assumed to be TYMED_ISTORAGE, with the IStorage* pstg member live; but in this case it was actually TYMED_ISTREAM, with the IStream* pstm member live.

The call to stm.pstg->CopyTo then actually invokes IStream::CopyTo, rather than IStorage::CopyTo (*). This has a sufficiently-compatible function signature and sufficiently fixed arguments that, rather than crashing here, it merely writes NULL into *file and then returns success.

file->Commit(...) then crashes predictably.

 

(*) There isn't a hidden nominal lookup or anything; the two CopyTo functions just happen to both be at slot #7 in their respective vtables.

Comment 4

[Chris Martin [:cmartin]]

Attachment: Bug 1824644 - Handle IDataObject::GetData returning wrong type

It appears that we can ask for TYMED_ISTORAGE from an IDataObject and
receive something different than we asked for. Since the return is a tagged
union, problems ahoy if we don't check that tag.

Since we have to handle both cases anyway, let's just request both an
IStream and IStorage and just handle whichever gets returned (or fail if
something we didn't expect is returned).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6053fd4465fd
Handle IDataObject::GetData returning wrong type r=rkraesig

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6053fd4465fd