124095 – Multiple IAccessible and IAccessible2 interface methods do not check for NULL pointer access, nor do they trap exceptions

Issue 124095 - Multiple IAccessible and IAccessible2 interface methods do not check for NULL pointer access, nor do they trap exceptions

Summary: Multiple IAccessible and IAccessible2 interface methods do not check for NULL...

Status:	CLOSED FIXED

Alias:	None

Product:	General
Classification:	Code
Component:	accessibility (show other issues)
Version:	4.1.0-dev
Hardware:	All Windows, all

Importance:	P3 Normal (vote)
Target Milestone:	4.1.0
Assignee:	AOO issues mailing list
QA Contact:

URL:
Keywords:

Depends on:
Blocks:	winA11y
	Show dependency tree

Reported:	2014-01-24 06:23 UTC by Michael Curran
Modified:	2017-05-20 10:35 UTC (History)
CC List:	4 users (show)

See Also:
Issue Type:	DEFECT
Latest Confirmation in:	---
Developer Difficulty:	---

Attachments
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 (1.07 KB, text/plain) 2014-01-24 06:23 UTC, Michael Curran	no flags	Details
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this issue.

Description Michael Curran 2014-01-24 06:23:55 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.

Comment 1 James Teh 2014-01-24 06:30:16 UTC

I (and other users) have seen the lag. Also, Mick and I discussed this at length.

Comment 2 Steve Yin 2014-01-24 17:07:01 UTC

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. :)

Comment 3 Steve Yin 2014-01-27 05:47:43 UTC

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.

Comment 4 SVN Robot 2014-01-27 05:49:27 UTC

"steve_y" committed SVN revision 1561587 into trunk:
Bug 124095 - Multiple IAccessible and IAccessible2 interface methods do not c...

Comment 5 Steve Yin 2014-01-27 05:50:05 UTC

UT passed.

Comment 6 V Stuart Foote 2014-01-30 02:12:05 UTC

@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

Comment 7 Steve Yin 2014-01-30 03:06:11 UTC

@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. :)