Fix crash in VBufStorage_buffer_t::getTextInRange when at the end of a buffer#8907
Conversation
jcsteh
left a comment
jcsteh
left a comment
There was a problem hiding this comment.
Ouch! I too am... very surprised by the compiler.
| wstring textString; | ||
| backend->getTextInRange(startOffset,endOffset,textString,useMarkup!=false); | ||
| backend->lock.release(); | ||
| if(textString.empty()) { |
There was a problem hiding this comment.
backend->getTextInRange differentiates between "error" and "empty", but this function looks at "empty" to determine whether to return failure. Effectively, I think it amounts to the same thing, but it'd be good to be consistent, unless there's some reason you're deliberately checking empty instead of false here.
There was a problem hiding this comment.
I'm not sure it is safe to pass an empty string to sysAllocString? We could trust that if there was no error, the string is not empty... though I'd need to read the code much deeper to be sure that is always the case.
| } | ||
|
|
||
| wstring VBufStorage_buffer_t::getTextInRange(int startOffset, int endOffset, bool useMarkup) { | ||
| bool VBufStorage_buffer_t::getTextInRange(int startOffset, int endOffset, wstring& text, bool useMarkup) { |
There was a problem hiding this comment.
Can I ask about the order of the params here? I would expect out params like text to be last?.
| bool VBufStorage_buffer_t::getTextInRange(int startOffset, int endOffset, wstring& text, bool useMarkup) { | ||
| if(this->rootNode==NULL) { | ||
| LOG_DEBUGWARNING(L"buffer is empty, returning NULL"); | ||
| return NULL; |
There was a problem hiding this comment.
Alternatively this could just return empty strings in this case.
|
Just for reference, a Mozilla bug was filed for this crash: https://bugzilla.mozilla.org/show_bug.cgi?id=1504272 I've now closed it. |
4 participants
Link to issue number:
None.
Summary of the issue:
PR #8866 (merging of VBufBackends into nvdaHelperRemote) introduced a crash in VBufStorage_buffer_t::getTextInRange.
To reproduce:
data:text/html,<p></p>This is because VBufStorage_buffer_t::getTextInRange is declaired as returning a wstring, but on some errors it returns NULL. I am very surprised the compiler allows this.
Description of how this pull request fixes the issue:
This PR changes getTextInRange so that it takes a string by reference which it copies the text into, and it returns a bool communicating success or failure. Note that this is the way that the node-specific getTextInRange methods work already.
Testing performed:
In Firefox:
Known issues with pull request:
None.
Change log entry:
None needed.