Closed Bug 541013 Opened 15 years ago Closed 14 years ago

UMR [@ nsRange::SetEnd] from alloc [@ nsHTMLEditor::SplitStyleAbovePoint]

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a4
Tracking Flags:
Tracking Status
blocking2.0 --- -

People

(Reporter: jruderman, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: testcase, valgrind, Whiteboard: [sg:low?])

Attachments

(4 files)

testcase
125 bytes, text/html
Details
possible fix
1.23 KB, patch
Details | Diff | Splinter Review
fix 2
4.53 KB, patch
peterv
: review+
Details | Diff | Splinter Review
mochitest.diff
2.28 KB, patch
Details | Diff | Splinter Review
Attached file testcase — 
To reproduce:

valgrind --dsymutil=yes --track-origins=yes ~/central/debug-obj/dist/MinefieldDebug.app/Contents/MacOS/firefox-bin f.html

Result:

==967== Conditional jump or move depends on uninitialised value(s)
==967==    at 0x3619DC2: nsRange::SetEnd(nsINode*, int) (nsRange.cpp:699)
==967==    by 0x33AE70D: nsTypedSelection::Collapse(nsINode*, int) (nsSelection.cpp:4864)
==967==    by 0x33AE9B9: nsTypedSelection::Collapse(nsIDOMNode*, int) (nsSelection.cpp:4837)
==967==    by 0x3C83D95: nsHTMLEditRules::CreateStyleForInsertText(nsISelection*, nsIDOMDocument*) (nsHTMLEditRules.cpp:4564)
==967==    by 0x3C88F58: nsHTMLEditRules::WillInsert(nsISelection*, int*) (nsHTMLEditRules.cpp:1333)
==967==    by 0x3C946CD: nsHTMLEditRules::WillDoAction(nsISelection*, nsRulesInfo*, int*, int*) (nsHTMLEditRules.cpp:686)
==967==    by 0x3C46F1A: nsHTMLEditor::InsertHTMLWithContext(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsIDOMDocument*, nsIDOMNode*, int, int) (nsHTMLDataTransfer.cpp:417)
==967==    by 0x3C3F83E: nsHTMLEditor::InsertHTML(nsAString_internal const&) (nsHTMLDataTransfer.cpp:256)
==967==    by 0x3EF5176: nsInsertHTMLCommand::DoCommandParams(char const*, nsICommandParams*, nsISupports*) (nsComposerCommands.cpp:1472)
==967==    by 0x3D6F2D0: nsControllerCommandTable::DoCommandParams(char const*, nsICommandParams*, nsISupports*) (nsControllerCommandTable.cpp:208)
==967==    by 0x3D696FC: nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) (nsBaseCommandController.cpp:185)
==967==    by 0x3D6C781: nsCommandManager::DoCommand(char const*, nsICommandParams*, nsIDOMWindow*) (nsCommandManager.cpp:271)
==967==  Uninitialised value was created by a stack allocation
==967==    at 0x3C6CD7A: nsHTMLEditor::SplitStyleAbovePoint(nsCOMPtr<nsIDOMNode>*, int*, nsIAtom*, nsAString_internal const*, nsCOMPtr<nsIDOMNode>*, nsCOMPtr<nsIDOMNode>*) (nsHTMLEditorStyle.cpp:583)
SplitStyleAbovePoint calls SplitNodeDeep without checking rv.  SplitNodeDeep fails and doesn't set its out-param, but SplitStyleAbovePoint assumes it set its out-param.
blocking2.0: --- → ?
Attached patch possible fix — — Splinter Review 
This quiets Valgrind (tested with the dup's testcase).  I didn't even try to understand the surrounding code enough to tell whether adding an early return here can create new problems.
Attached patch fix 2 — — Splinter Review 
Jesse's patch makes sense since SplitNodeDeep() could fail for any
number of reasons (so I included it here).
For the testcase though, I don't think it makes sense to call
SplitStyleAbovePoint() on the root node.  This patch gives the
resulting DOM <html>foo<head>...
Assignee: nobody → matspal
Attachment #428121 - Flags: review?(peterv)
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 428121 [details] [diff] [review]
fix 2

>diff --git a/editor/libeditor/html/nsHTMLEditorStyle.cpp b/editor/libeditor/html/nsHTMLEditorStyle.cpp

>+      nsresult rv = SplitNodeDeep(tmp, *aNode, *aOffset, &offset, PR_FALSE, outLeftNode, outRightNode);

Long line.
We probably also have a bug where SplitStyleAbovePoint might split nodes that are not editable? :-/
Attachment #428121 - Flags: review?(peterv) → review+
Attached patch mochitest.diff — — Splinter Review 

    
    

    
  
Landed with nit fixed, but without the mochitest for now:
http://hg.mozilla.org/mozilla-central/rev/a20ed5e85c3d
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4

    
    

    
  
(In reply to comment #5)
> We probably also have a bug where SplitStyleAbovePoint might split nodes that
> are not editable? :-/

I couldn't find a bug, so I filed bug 552610.
Group: core-security

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: