421650 - don't allocate [out] BSTR

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Michael Curran]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008030607 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008030706 Minefield/3.0b5pre


The March 07 (time 06) build of Firefox is unusable with NVDA because of this crash. It seems it may have something to do with the change to NSAccessibleWrap.cpp in revision 1.108. Where a call in nsAccessibleWrap::get_attributes to SysAllocString was replaced by SysReallocStringLen. 

Reproducible: Always

Steps to Reproduce:
1.
2.
3.



Crash ID: ed3d3a05-eccb-11dc-8375-001a4bd43e5c
Seems to crash in module ntdll.dll, which was called by oleaut32 which was called by NSAccessibleWrap::get_attributes. And in this case in the crash, it shows it was called by VBufBackend_gecko_ia2 (which is part of the in-process virtualBuffer code for NVDA).

Comment 1

[Aaron Leventhal]

I backed out the part of bug 417018 which Mick is talking about. Mick, let us know if this issue is not really fixed from this.

Comment 2

[alexander :surkov (:asurkov)]

Would be nice to know how Mick uses get_attributes because we use SysReAllocStringLen everywhere and we should khow what's exactly wrong.

Comment 3

[alexander :surkov (:asurkov)]

MSDN: Passing invalid (and under some circumstances NULL) pointers to this function causes an unexpected termination of the application.

Obviously we get NULL and therefore we crash. I sent mail to Pete for clarification. Reopen bug since in any way we should do something here (either to fix another methods or to fix this one).

Comment 4

[alexander :surkov (:asurkov)]

reassign it to me

Comment 5

[alexander :surkov (:asurkov)]

Mick, can you give details how you use get_attributes?

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

I'm not sure about get_oldText and get_newText. Once I'll get clarification about this I'll update the patch.

Comment 7

[Michael Curran]

(In reply to comment #5)
> Mick, can you give details how you use get_attributes?
An example is:
	BSTR IA2Attributes;
	if((res=pacc->get_attributes(&IA2Attributes))!=S_OK) {
...

Should I be initializing that BSTR to something?

I don't quite understand why SysReallocStringLen (a function that sort of assumes that the original string was allocated) needs to be used. I can never think of a case where the AT would pass get_attributes an allocated bstr via the bstr pointer address. I am sorry if I am miss-understanding the problem. But all I can really say is that up until now I have had no problems with any of the bstr passing MSAA/IA2 methods. It is clear that SysReallocStringLen causes issues.   

As far as I understand this:
The AT provides a memory address to get_attributes, in which get_attributes can place a bstr wich it allocated. All the AT needs to allocate is  memory of the size of a bstr type. It shouldn't have to allocate any of the actual memory inside the bstr.
  

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 9

[alexander :surkov (:asurkov)]

Mick, you're right. We shouldn't use Re methods. Some rules:

1) assign NULL to [out] *BSTR
2) if string is empty return S_FALSE
3) otherwise allocate memory

Comment 10

[Aaron Leventhal]

Comment on attachment 309389 [details] [diff] [review]
patch

Do we have a lot of other SysReAlloc methods to change?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> (From update of attachment 309389 [details] [diff] [review])
> Do we have a lot of other SysReAlloc methods to change?
> 

No, we haven't realloc methods more.

Comment 12

[Damon Sicore (:damons)]

Comment on attachment 309389 [details] [diff] [review]
patch

a1.9+=damons

Comment 13

[alexander :surkov (:asurkov)]

/cvsroot/mozilla/accessible/src/msaa/CAccessibleAction.cpp,v  <--  CAccessibleAction.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in accessible/src/msaa/CAccessibleHyperlink.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleHyperlink.cpp,v  <--  CAccessibleHyperlink.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in accessible/src/msaa/CAccessibleImage.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleImage.cpp,v  <--  CAccessibleImage.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in accessible/src/msaa/CAccessibleTable.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleTable.cpp,v  <--  CAccessibleTable.cpp
new revision: 1.10; previous revision: 1.9
done
Checking in accessible/src/msaa/CAccessibleText.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleText.cpp,v  <--  CAccessibleText.cpp
new revision: 1.10; previous revision: 1.9
done
Checking in accessible/src/msaa/nsAccessNodeWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessNodeWrap.cpp,v  <--  nsAccessNodeWrap.cpp
new revision: 1.43; previous revision: 1.42
done
Checking in accessible/src/msaa/nsAccessibleRelationWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleRelationWrap.cpp,v  <--  nsAccessibleRelationWrap.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in accessible/src/msaa/nsAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp,v  <--  nsAccessibleWrap.cpp
new revision: 1.113; previous revision: 1.112
done
Checking in accessible/src/msaa/nsApplicationAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsApplicationAccessibleWrap.cpp,v  <--  nsApplicationAccessibleWrap.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in accessible/src/msaa/nsDocAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsDocAccessibleWrap.cpp,v  <--  nsDocAccessibleWrap.cpp
new revision: 1.53; previous revision: 1.52
done
Checking in accessible/src/msaa/nsTextAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsTextAccessibleWrap.cpp,v  <--  nsTextAccessibleWrap.cpp
new rev

Comment 14

[Aaron Leventhal]

(In reply to comment #9)
> Mick, you're right. We shouldn't use Re methods. Some rules:
> 
> 1) assign NULL to [out] *BSTR
> 2) if string is empty return S_FALSE
> 3) otherwise allocate memory

I think rules 2 & 3 need to be switched. This caused bug 423495. Should be:

1) assign NULL to [out] *BSTR, return E_FAIL if there is any error
2) allocate memory
3) if string is empty return S_FALSE

Comment 15

[Aaron Leventhal]

I think this caused bug 424073.

Comment 16

[Aaron Leventhal]

We might want to consider reversing all of the NULL/S_FALSE changes to IAccessible methods. They're not helping anything and it's pretty late in the game to deal with crashes they cause in older screen readers.