327623 - nsHTMLEditor::SelectElement crashes if passed null argument

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Tony Goold]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-us) AppleWebKit/417.9 (KHTML, like Gecko) Safari/417.8
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060215 SeaMonkey/1.5a

If you pass null to nsHTMLEditor::SelectElement, it crashes.

Reproducible: Always

Steps to Reproduce:
1. Load chrome that has an editor element in it.
2. Get its nsIHTMLEditor by calling getHTMLEditor on it.
3. Invoke its SelectElement method with a NULL argument.

Actual Results:  
Mozilla crashes with an invalid memory access.

Expected Results:  
SelectElement should return NS_ERROR_INVALID_POINTER.

I found this initially on a custom build of Firefox 1.5.0.1 on Mac OS X, and reproduced it with a nightly SeaMonkey (also on OS X). In nsHTMLEditor.cpp, the argument is unchecked, so I think it's safe to assume this isn't platform specific. It's odd that nsTextEditUtils::InBody (called by nsHTMLEditor::IsElementInBody) returns true for a null value. Could that also be a bug?

Comment 1

[Tony Goold]

Attachment: Patch to fix described problem

Added NS_ENSURE_ARG(aElement) to the beginning of SelectElement.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 212213 [details] [diff] [review]
Patch to fix described problem

I don't think Joe Francis is still active with the project, feel free to correct if I'm wrong.

Comment 3

[timeless]

Comment on attachment 212213 [details] [diff] [review]
Patch to fix described problem

if you want to do this, it should be NS_ENSURE_ARG_POINTER.

but is this really what you'd expect? personally if i was calling selectElement(null), i'd expect the selection to be cleared, that's certainly what it would mean to me.

One could instead use GetSelection, selection->removeAllRanges()

Either way, I wonder if the interface should indicate what will happen when null is passed.

Comment 4

[Tony Goold]

(In reply to comment #3)
> but is this really what you'd expect? personally if i was calling
> selectElement(null), i'd expect the selection to be cleared, that's certainly
> what it would mean to me.

Maybe I could get a bit of guidance here on what the expected behaviour should be. I think the root of the problem is that nsTextEditUtils::InBody returns PR_TRUE when passed null, but it seems to me it should be returning PR_FALSE instead. If that were the case, then SelectElement would return NS_ERROR_INVALID_POINTER without any modifications.

Should I submit a patch to nsTextEditUtils::InBody instead?

Comment 5

[timeless]

makes sense to me

Comment 6

[Tony Goold]

Attachment: Returns false in InBody if element is null

This changes nsTextEditUtils::InBody to return PR_FALSE if the element passed in is null.

Comment 7

[[e-mail]]

Comment on attachment 212494 [details] [diff] [review]
Returns false in InBody if element is null

>+  if (! node)
Nit: (!node). sr=me with the space removed.

>+  nsCOMPtr<nsIDOMNode> rootNode = do_QueryInterface(rootElement);
Note: Comparing to rootElement should be safe.

>+  nsCOMPtr<nsIDOMNode> tmp;
>+  nsCOMPtr<nsIDOMNode> p = node;
>+  while (p && p != rootNode)
Note: p is never null at this point.

I note these because you're changing the lines anyway so people might blame you for them even though you're only reindenting them.

Comment 8

[Wayne Woods]

*** Bug 329895 has been marked as a duplicate of this bug. ***

Comment 9

[Martijn Wargers (dead)]

Tony, do you need a check-in of the patch?

Comment 10

[Tony Goold]

(In reply to comment #9)
> Tony, do you need a check-in of the patch?
> 

Yes, please. I'll attach a fresh patch with Neil's requested changes from his sr.

Comment 11

[Tony Goold]

Attachment: Fixed requests from sr

Comment 12

[Martijn Wargers (dead)]

Checking in editor/libeditor/text/nsTextEditUtils.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsTextEditUtils.cpp,v  <--  nsTextEditUti
ls.cpp
new revision: 1.15; previous revision: 1.14
done

Thanks for the patch, Tony.