nagai/fune
3
0
Fork 0
forked from mirrors/gecko-dev
Code Pull requests Activity

Bug 1650590: A11y COM handler: Use CoTaskMemAlloc/Free for row/column header ids arrays when returned by IGeckoBackChannel::Refresh. r=MarcoZ

Browse source 
DynamicIA2Data can be built to be transmitted in two different ways:

1. As part of the payload included in the stream when an accessible is marshaled; or
2. As an out parameter returned by IGeckoBackChannel::Refresh().

DynamicIA2Data includes arrays for row/column header ids.
Normally, such arrays would be allocated by CoTaskMemAlloc and freed by CoTaskMemFree.
However, in the first case, the struct is actually marshaled by RPC encoding functions, not by COM itself.
This means we must use midl_user_allocate/free, lest we crash.
We previously used midl_user_allocate/free for the second case as well.
Unfortunately, it turns out that this too causes crashes.

To fix this, we now use different memory allocation functions depending on how the struct is transmitted.

This patch also cleans up the old DynamicIA2Data in the client before calling IGeckoBackChannel::Refresh.
Previously, we didn't do this, which would have resulted in a leak.

Differential Revision: https://phabricator.services.mozilla.com/D82823
This commit is contained in:
James Teh 2020-07-09 06:56:24 +00:00
parent 6959f8e469
commit ba7d805a62
5 changed files with 59 additions and 18 deletions
repo.diff.show_diff_stats Download patch file Download diff file Expand all files Collapse all files

32
accessible/ipc/win/HandlerProvider.cpp
View file

@ -222,7 +222,8 @@ void HandlerProvider::BuildStaticIA2Data(
}
}
void HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data) {
void HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data,
bool aMarshaledByCom) {
MOZ_ASSERT(aOutIA2Data);
MOZ_ASSERT(NS_IsMainThread());
MOZ_ASSERT(IsTargetInterfaceCacheable());
@ -242,13 +243,24 @@ void HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data) {
auto hasFailed = [&hr]() -> bool { return FAILED(hr); };
auto cleanup = [aOutIA2Data]() -> void {
CleanupDynamicIA2Data(*aOutIA2Data);
auto cleanup = [aOutIA2Data, aMarshaledByCom]() -> void {
CleanupDynamicIA2Data(*aOutIA2Data, aMarshaledByCom);
};
mscom::ExecuteWhen<decltype(hasFailed), decltype(cleanup)> onFail(hasFailed,
cleanup);
// When allocating memory to be returned to the client, you *must* use
// allocMem, not CoTaskMemAlloc!
auto allocMem = [aMarshaledByCom](size_t aSize) {
if (aMarshaledByCom) {
return ::CoTaskMemAlloc(aSize);
}
// We use midl_user_allocate rather than CoTaskMemAlloc because this
// struct is being marshaled by RPC, not COM.
return ::midl_user_allocate(aSize);
};
const VARIANT kChildIdSelf = {VT_I4};
VARIANT varVal;
@ -363,10 +375,8 @@ void HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data) {
return;
}
if (aOutIA2Data->mNRowHeaderCells > 0) {
// We use midl_user_allocate rather than CoTaskMemAlloc because this
// struct is being marshaled by RPC, not COM.
aOutIA2Data->mRowHeaderCellIds = static_cast<long*>(
::midl_user_allocate(sizeof(long) * aOutIA2Data->mNRowHeaderCells));
allocMem(sizeof(long) * aOutIA2Data->mNRowHeaderCells));
for (long i = 0; i < aOutIA2Data->mNRowHeaderCells; ++i) {
RefPtr<IAccessible2> headerAcc;
hr = headers[i]->QueryInterface(IID_IAccessible2,
@ -385,11 +395,8 @@ void HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data) {
return;
}
if (aOutIA2Data->mNColumnHeaderCells > 0) {
// We use midl_user_allocate rather than CoTaskMemAlloc because this
// struct is being marshaled by RPC, not COM.
aOutIA2Data->mColumnHeaderCellIds =
static_cast<long*>(::midl_user_allocate(
sizeof(long) * aOutIA2Data->mNColumnHeaderCells));
aOutIA2Data->mColumnHeaderCellIds = static_cast<long*>(
allocMem(sizeof(long) * aOutIA2Data->mNColumnHeaderCells));
for (long i = 0; i < aOutIA2Data->mNColumnHeaderCells; ++i) {
RefPtr<IAccessible2> headerAcc;
hr = headers[i]->QueryInterface(IID_IAccessible2,
@ -575,7 +582,8 @@ HandlerProvider::Refresh(DynamicIA2Data* aOutData) {
if (!mscom::InvokeOnMainThread("HandlerProvider::BuildDynamicIA2Data", this,
&HandlerProvider::BuildDynamicIA2Data,
std::forward<DynamicIA2Data*>(aOutData))) {
std::forward<DynamicIA2Data*>(aOutData),
/* aMarshaledByCom */ true)) {
return E_FAIL;
}

11
accessible/ipc/win/HandlerProvider.h
View file

@ -75,7 +75,16 @@ class HandlerProvider final : public IGeckoBackChannel,
NotNull<mscom::IInterceptor*> aInterceptor);
void BuildStaticIA2Data(NotNull<mscom::IInterceptor*> aInterceptor,
StaticIA2Data* aOutData);
void BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data);
/**
* Pass true for aMarshaledByCom if this struct is being directly marshaled as
* an out parameter of a COM method, currently only
* IGeckoBackChannel::Refresh.
* When aMarshaledByCom is false, this means the struct is being marshaled
* by RPC encoding functions. This means we must allocate memory differently,
* even though we're using this as part of a COM handler payload.
*/
void BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data,
bool aMarshaledByCom = false);
void BuildInitialIA2Data(NotNull<mscom::IInterceptor*> aInterceptor,
StaticIA2Data* aOutStaticData,
DynamicIA2Data* aOutDynamicData);

12
accessible/ipc/win/handler/AccessibleHandler.cpp
View file

@ -82,6 +82,7 @@ AccessibleHandler::AccessibleHandler(IUnknown* aOuter, HRESULT* aResult)
mIATableCellPassThru(nullptr),
mIAHypertextPassThru(nullptr),
mCachedData(),
mCachedDynamicDataMarshaledByCom(false),
mCacheGen(0),
mCachedHyperlinks(nullptr),
mCachedNHyperlinks(-1),
@ -103,7 +104,8 @@ AccessibleHandler::AccessibleHandler(IUnknown* aOuter, HRESULT* aResult)
}
AccessibleHandler::~AccessibleHandler() {
CleanupDynamicIA2Data(mCachedData.mDynamicData);
CleanupDynamicIA2Data(mCachedData.mDynamicData,
mCachedDynamicDataMarshaledByCom);
if (mCachedData.mGeckoBackChannel) {
mCachedData.mGeckoBackChannel->Release();
}
@ -218,12 +220,16 @@ AccessibleHandler::MaybeUpdateCachedData() {
return E_POINTER;
}
// Clean up the old data.
CleanupDynamicIA2Data(mCachedData.mDynamicData,
mCachedDynamicDataMarshaledByCom);
HRESULT hr =
mCachedData.mGeckoBackChannel->Refresh(&mCachedData.mDynamicData);
if (SUCCEEDED(hr)) {
// We just updated the cache, so update this object's cache generation
// so we only update the cache again after the next change.
mCacheGen = gen;
mCachedDynamicDataMarshaledByCom = true;
}
return hr;
}
@ -466,8 +472,10 @@ AccessibleHandler::ReadHandlerPayload(IStream* aStream, REFIID aIid) {
return E_FAIL;
}
// Clean up the old data.
CleanupDynamicIA2Data(mCachedData.mDynamicData);
CleanupDynamicIA2Data(mCachedData.mDynamicData,
mCachedDynamicDataMarshaledByCom);
mCachedData = newData;
mCachedDynamicDataMarshaledByCom = false;
// These interfaces have been aggregated into the proxy manager.
// The proxy manager will resolve these interfaces now on QI,

1
accessible/ipc/win/handler/AccessibleHandler.h
View file

@ -280,6 +280,7 @@ class AccessibleHandler final : public mscom::Handler,
IAccessibleTableCell* mIATableCellPassThru; // weak
IAccessibleHypertext2* mIAHypertextPassThru; // weak
IA2Payload mCachedData;
bool mCachedDynamicDataMarshaledByCom;
UniquePtr<mscom::StructToStream> mSerializer;
uint32_t mCacheGen;
IAccessibleHyperlink** mCachedHyperlinks;

21
accessible/ipc/win/handler/HandlerDataCleanup.h
View file

@ -37,7 +37,22 @@ inline void ReleaseStaticIA2DataInterfaces(StaticIA2Data& aData) {
}
}
inline void CleanupDynamicIA2Data(DynamicIA2Data& aData) {
/**
* Pass true for aMarshaledByCom if this struct was directly marshaled as an
* out parameter of a COM method, currently only IGeckoBackChannel::Refresh.
*/
inline void CleanupDynamicIA2Data(DynamicIA2Data& aData,
bool aMarshaledByCom = false) {
// If freeing generic memory returned to the client, you *must* use freeMem,
// not CoTaskMemFree!
auto freeMem = [aMarshaledByCom](void* aMem) {
if (aMarshaledByCom) {
::CoTaskMemFree(aMem);
} else {
::midl_user_free(aMem);
}
};
::VariantClear(&aData.mRole);
if (aData.mKeyboardShortcut) {
::SysFreeString(aData.mKeyboardShortcut);
@ -67,10 +82,10 @@ inline void CleanupDynamicIA2Data(DynamicIA2Data& aData) {
::SysFreeString(aData.mIA2Locale.variant);
}
if (aData.mRowHeaderCellIds) {
::midl_user_free(aData.mRowHeaderCellIds);
freeMem(aData.mRowHeaderCellIds);
}
if (aData.mColumnHeaderCellIds) {
::midl_user_free(aData.mColumnHeaderCellIds);
freeMem(aData.mColumnHeaderCellIds);
}
}