Another crash via UIAutomationCore

Message Topic Search Topic Options Post Reply Create New Topic Printable Version Translate Topic

   CJ,

Please fix this code. The input index isn't being tested for validity and you are accessing an item in an array past the array index:

HRESULT CXTPRibbonControlTab::AccessibleLocation(long* pxLeft, long* pyTop, long* pcxWidth,
                                                 long* pcyHeight, VARIANT varChild)
{
    *pxLeft = *pyTop = *pcxWidth = *pcyHeight = 0;

    int nIndex = GetChildIndex(&varChild);

    if (!m_pParent->GetSafeHwnd())
        return S_OK;

    if (!IsVisible())
        return S_OK;

    CRect rcControl = GetRect();

    if (nIndex != CHILDID_SELF)
    {
        rcControl = GetItem(nIndex - 1)->GetRect();
    }

    m_pParent->ClientToScreen(&rcControl);

    *pxLeft       = rcControl.left;
    *pyTop       = rcControl.top;
    *pcxWidth  = rcControl.Width();
    *pcyHeight = rcControl.Height();

    return S_OK;
}

In my case, the array size is 7 and the input is an VT_I4 with value 8. So, 6 is the last valid index. Just return an error instead of crashing the process. In my case, we are MDI and we are rebuilding the command ribbon. This call was made for the ribbon tab control. Unfortunately this UIAutomationCore is calling here when I have the OS Text Cursor Indicator setting on because our customer base is SCREAMING about how unstable we are when it is running.

Unfortunately this UIAutomationCore code related to this cursor text indicator has its own thread and its own hooking code. I have found no way to shut this down on a per process basis. So, we are left with finding these "random" crashes and do our best to address them. This is random because the OS code is using a thread to try to track what is going on and it is often behind the curve. We have customer videos showing us that if they work too fast, we crash. It can take a number of times to crash but eventually we do.

What I don't understand right now is from where Microsoft is getting this index they pass in. I am trapping no breakpoints in the get_accChildCount method.

In this case, an easy test of the array size is all that is needed. I'd return E_FALI or some other COM error like E_INVALIDARG.

Author	Message Topic Search Topic Options Post Reply Create New Topic Printable Version Translate Topic
rdhd Members Profile Send Private Message Find Members Posts Add to Buddy List Senior Member Joined: 13 August 2007 Location: United States Status: Offline Points: 886	Post Options Post Reply Quote rdhd Report Post Thanks(0) Quote Reply Topic: Another crash via UIAutomationCore Posted: 21 June 2022 at 3:14pm
	CJ, Please fix this code. The input index isn't being tested for validity and you are accessing an item in an array past the array index: HRESULT CXTPRibbonControlTab::AccessibleLocation(long* pxLeft, long* pyTop, long* pcxWidth, long* pcyHeight, VARIANT varChild) { pxLeft = pyTop = pcxWidth = pcyHeight = 0; int nIndex = GetChildIndex(&varChild); if (!m_pParent->GetSafeHwnd()) return S_OK; if (!IsVisible()) return S_OK; CRect rcControl = GetRect(); if (nIndex != CHILDID_SELF) { rcControl = GetItem(nIndex - 1)->GetRect(); } m_pParent->ClientToScreen(&rcControl); pxLeft = rcControl.left; pyTop = rcControl.top; pcxWidth = rcControl.Width(); pcyHeight = rcControl.Height(); return S_OK; } In my case, the array size is 7 and the input is an VT_I4 with value 8. So, 6 is the last valid index. Just return an error instead of crashing the process. In my case, we are MDI and we are rebuilding the command ribbon. This call was made for the ribbon tab control. Unfortunately this UIAutomationCore is calling here when I have the OS Text Cursor Indicator setting on because our customer base is SCREAMING about how unstable we are when it is running. Unfortunately this UIAutomationCore code related to this cursor text indicator has its own thread and its own hooking code. I have found no way to shut this down on a per process basis. So, we are left with finding these "random" crashes and do our best to address them. This is random because the OS code is using a thread to try to track what is going on and it is often behind the curve. We have customer videos showing us that if they work too fast, we crash. It can take a number of times to crash but eventually we do. What I don't understand right now is from where Microsoft is getting this index they pass in. I am trapping no breakpoints in the get_accChildCount method. In this case, an easy test of the array size is all that is needed. I'd return E_FALI or some other COM error like E_INVALIDARG.

rdhd Members Profile Send Private Message Find Members Posts Add to Buddy List Senior Member Joined: 13 August 2007 Location: United States Status: Offline Points: 886	Post Options Post Reply Quote rdhd Report Post Thanks(0) Quote Reply Posted: 22 June 2022 at 11:08am
	I was running the RibbonMDISample seeing if I could crash in the same place as our app. What I was doing is using control-n to open a new file and then F4 to close the window. I repeated that a number of times. I crashed but not in the same spot: riched20.dll!00007fff7f854962() Unknown riched20.dll!00007fff7f8806b3() Unknown riched20.dll!00007fff7f8650b1() Unknown UIAutomationCore.dll!RichEditProxy::~RichEditProxy(void) Unknown UIAutomationCore.dll!RichEditProxy::`vector deleting destructor'(unsigned int) Unknown UIAutomationCore.dll!RefcountBase::Release(void) Unknown UIAutomationCore.dll!UiaNode::ProviderInfo::~ProviderInfo(void) Unknown UIAutomationCore.dll!`eh vector destructor iterator'(void ,unsigned __int64,unsigned __int64,void ()(void )) Unknown UIAutomationCore.dll!UiaNode::`vector deleting destructor'(unsigned int) Unknown UIAutomationCore.dll!UiaNode::Release(void) Unknown UIAutomationCore.dll!ReleaseOnCorrectContext_Callback() Unknown UIAutomationCore.dll!ComInvoker::CallTarget() Unknown UIAutomationCore.dll!ReleaseCollection::DispatchReleases() Unknown UIAutomationCore.dll!ChannelBasedServerConnection::ReleaseObjects() Unknown UIAutomationCore.dll!HookBasedServerConnection::`vector deleting destructor'(unsigned int) Unknown UIAutomationCore.dll!RefcountBase::Release(void) Unknown UIAutomationCore.dll!HookBasedServerConnectionManager::HookCallback() Unknown UIAutomationCore.dll!HookUtil<&HookBasedClientConnection::HookCallback(void ,unsigned long,void * ,unsigned long ,void * ),0>::CallOut(void ,unsigned long,void * ,unsigned long ,void * ) Unknown UIAutomationCore.dll!HandleSyncHookMessage() Unknown UIAutomationCore.dll!HookUtil<&HookBasedClientConnection::HookCallback,0>::CallWndProc() Unknown user32.dll!fnHkINLPCWPSTRUCTW() Unknown user32.dll!__fnDWORD() Unknown ntdll.dll!KiUserCallbackDispatcherContinue() Unknown win32u.dll!NtUserPeekMessage() Unknown user32.dll!_PeekMessage() Unknown user32.dll!PeekMessageW() Unknown > mfc140ud.dll!CWinThread::Run() Line 617 C++ mfc140ud.dll!CWinApp::Run() Line 787 C++ mfc140ud.dll!AfxWinMain(HINSTANCE__ , HINSTANCE__ , wchar_t , int) Line 47 C++ RibbonMDISample.exe!wWinMain(HINSTANCE__ , HINSTANCE__ , wchar_t *, int) Line 26 C++

rdhd Members Profile Send Private Message Find Members Posts Add to Buddy List Senior Member Joined: 13 August 2007 Location: United States Status: Offline Points: 886	Post Options Post Reply Quote rdhd Report Post Thanks(0) Quote Reply Posted: 22 June 2022 at 11:19am
	Regarding the AccessibleLocation code. This crash is once again due to CJ's usage of the code pattern: GetX()->CallMethodUsingNullPtrReturnedFromOperator(). I found GetItem will return nullptr when the index is invalid as it is in the case that is crashing our app with the text cursor indicator setting on. I am rebuilding the code now with this: CXTPTabManagerItem* pItem = GetItem(nIndex - 1); if( pItem ) { rcControl = pItem->GetRect(); } else { return E_INVALIDARG; } Using that coding pattern may be convenient but it just isn't solid code, in my opinion. Nor is the use of that pattern consistent. See this which is in the same source file: HRESULT CXTPRibbonControlTab::GetAccessibleName(VARIANT varChild, BSTR* pszName) { SAFE_MANAGE_STATE(m_pModuleState); int nIndex = GetChildIndex(&varChild); if (nIndex == CHILDID_SELF) return CXTPControl::GetAccessibleName(varChild, pszName); CXTPTabManagerItem* pItem = GetItem(nIndex - 1); if (!pItem) return E_INVALIDARG; CString strCaption = pItem->GetCaption(); CXTPDrawHelpers::StripMnemonics(strCaption); *pszName = strCaption.AllocSysString(); return S_OK; } Much better as it won't crash if some silly OS code calls that API with an invalid arg. Now that I noticed that, I'm going back to my source change and doing a copy&paste so the two code blocks look the same as well as behaving the same. CodeJock, can you make the mods to this source so I don't have to keep updating your code in the future. If you haven't already done so, that is.