Apache OpenOffice (AOO) Bugzilla – Issue 124095
Multiple IAccessible and IAccessible2 interface methods do not check for NULL pointer access, nor do they trap exceptions
Last modified: 2017-05-20 10:35:25 UTC
Created attachment 82380 [details] Patch adding ISDESTROY calls to get_nRelations, get_relations and get_attributes. ENTER_PROTECTED_BLOCK and LEAVE_PROTECTED_BLOCK to get_attributes. And checks NULL pointer from getChildInterface in A few methods on CMAccessible do not call ISDESTROY, but then go on to access pUnoInterface wich causes an access violation if the object is in deed destroied (i.e. NotifyDestroy has been called). These include get_nRelations, get_relations and get_attributes. get_accChild also does not check the pointer it gets back from getChildInterface, which most certainly could be NULL. get_attributes also does not call ENTER_PROTECTED_BLOCK and LEAVE_PROTECTED_BLOCK which means the exceptions could bubble up to Windows RPC, Open Office itself or an in-process AT. Although these are bad enough in theory, practically speaking you can see the consequences of this if using an AT such as NVDA with Open Office Calc. If you move focus across the cells, each focus move causes an access violation either in get_accChild (if UI automation is in use) or get_attributes (if not). Although Windows seems to catch some or all of these exceptions, the first causes a huge lag (most likely as Windows makes a mini dump or something). I have provided a patch to fix these methods. This is my first ever patch to this project, so not sure if there are any formatting issues.
I (and other users) have seen the lag. Also, Mick and I discussed this at length.
Terrific! Welcome Michael to be a contributor on development! The patch looks good. I will try it and do some UT later. I just noticed the null pointer issue of get_accChild and modified it locally for bug 123745 before this. :)
Hi Mick, I just modified one line of your patch from return (*ppdispChild)?S_OK:S_FALSE; to return S_OK; Because when (*ppdispChild) is NULL, it will return E_FAIL before that. Thanks.
"steve_y" committed SVN revision 1561587 into trunk: Bug 124095 - Multiple IAccessible and IAccessible2 interface methods do not c...
UT passed.
@Steve, *, In addition to Mick's patch, might need to have a look at AccRelation.cxx and wrapping the BSTR int type return in a ::SysAllocString() to assure type assignment. This was done over on the LibreOffice side and has improved UAccCOM stability notably. Also, since the PMC agreed to spin a Dev Snapshot build as rev 1560773, I'm still recommending folks needing AT support work with the Windows nightly build--currently at rev 1562109--to include the r1561587 IA2 patches. Should we lobby to respin the snapshot revision to get a stable IAccessible2 build more widely distributed for testing beyond en-US language set? Stuart
@Stuart: +1. The last revision is recommended from IA2 respect. Thanks for your suggestion. I will check the type mismatch problem in AccRelation.cxx. Due to the legal reasons of the license. As a developer of AOO, I have no privilege to refer the patches from LibreOffice. And it will be nice if someone from LibreOffice can contribute the patches to AOO. :)