php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #61797 DOMNode::appendChild doesn't increment refcount
Submitted: 2012-04-21 10:37 UTC Modified: 2017-10-24 06:14 UTC
Votes:5
Avg. Score:3.8 ± 1.2
Reproduced:4 of 4 (100.0%)
Same Version:0 (0.0%)
Same OS:4 (100.0%)
From: kuba dot brecka at gmail dot com Assigned:
Status: Open Package: DOM XML related
PHP Version: 5.4.0 OS: Windows, Linux
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: kuba dot brecka at gmail dot com
New email:
PHP Version: OS:

 

 [2012-04-21 10:37 UTC] kuba dot brecka at gmail dot com
Description:
------------
When I create a DOM element and I add it via DOMDocument->appendChild, it seems 
that the refcount to that object is not increased. This causes the appended child 
to be deallocated if there are no other references. It also causes some funky 
behaviour, because the freed object can still be accesssed (doesn't) crash, but 
it seems to be of different type. Don't know the cause, maybe the memory gets 
either reused to create another object.

I created a simple test case that proves and reproduces this buggy behaviour, 
AFAIK all PHP versions are affected, I tested 5.4.0 on Windows.

Test script:
---------------
class MyElement extends DOMElement { }

// #1 - okay
$dom = new DOMDocument();
$e = new MyElement("e");
$dom->appendChild($e);
echo get_class($dom->childNodes->item(0)) . "\n";

// #2 - wrong
$dom = new DOMDocument();
$dom->appendChild(new MyElement("e"));
echo get_class($dom->childNodes->item(0)) . "\n";

// #3 - wrong
$dom = new DOMDocument();
$e = new MyElement("e");
$dom->appendChild($e);
$e = null;
echo get_class($dom->childNodes->item(0)) . "\n";


Expected result:
----------------
MyElement
MyElement
MyElement


Actual result:
--------------
MyElement
DOMElement
DOMElement


Patches

dom_node_append_child.patch (last revision 2013-12-02 19:55 UTC) by krakjoe@php.net)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2012-04-26 13:27 UTC] maarten@php.net
Shouldn't you just use registerNodeClass() ?

See http://3v4l.org/E88Nk for a modified script
 [2012-04-26 13:27 UTC] maarten@php.net
-Status: Open +Status: Feedback
 [2012-04-26 13:46 UTC] kuba dot brecka at gmail dot com
-Status: Feedback +Status: Open
 [2012-04-26 13:46 UTC] kuba dot brecka at gmail dot com
Well, registerNodeClass solves it. But I still believe that without it, the 
behaviour should not depend on the refcount of the object you are trying to use in 
DOM. It should at least emit a warning or an exception, that you are using an 
object, which has not beed registered.

Anyway, the need to use registerNodeClass should be mentioned in the docs, at 
least in appendChild.
 [2013-12-02 16:35 UTC] mike@php.net
-Type: Bug +Type: Documentation Problem
 [2013-12-02 19:43 UTC] krakjoe@php.net
The following patch has been added/updated:

Patch Name: dom_node_append_child.patch
Revision:   1386013405
URL:        https://bugs.php.net/patch-display.php?bug=61797&patch=dom_node_append_child.patch&revision=1386013405
 [2013-12-02 19:44 UTC] krakjoe@php.net
That is clearly wrong ... patch attached ... review please, no need to wait any longer ...
 [2013-12-02 19:55 UTC] krakjoe@php.net
The following patch has been added/updated:

Patch Name: dom_node_append_child.patch
Revision:   1386014157
URL:        https://bugs.php.net/patch-display.php?bug=61797&patch=dom_node_append_child.patch&revision=1386014157
 [2013-12-02 19:57 UTC] krakjoe@php.net
It is obvious from tests that this behaviour is expected, that doesn't make it any more correct ... it might create compatibility issues if fixed ... I don't know ... patch updated to fix the tests
 [2013-12-03 00:26 UTC] bjori@php.net
-Type: Documentation Problem +Type: Bug
 [2013-12-03 00:26 UTC] bjori@php.net
This is not a doc issue.. Reclassified as a bug.
 [2013-12-03 07:37 UTC] mike@php.net
-Assigned To: +Assigned To: rrichards
 [2013-12-03 07:37 UTC] mike@php.net
Your patch reads like a bug fix, but reading it in context with the source it is not definitely a bug. Why would you suggest registerNodeClass() exists?

If we change that, then obviously only for master.
But let's first try to reach an author for comment.

Rob?
 [2013-12-03 07:45 UTC] krakjoe@php.net
The non-standard registerNodeClass exists to service the createElement functionality of the DOM, there is no other logical reason for it's existence.

The patch doesn't change that, it enforces reference counting, which is required, everywhere.
 [2015-05-17 16:57 UTC] cmb@php.net
It appears to me that this is not a reference counting issue; at
least the additional zval_copy_ctor() leaks memory with a recent
master on Windows.
 [2017-10-24 06:14 UTC] kalle@php.net
-Status: Assigned +Status: Open -Assigned To: rrichards +Assigned To:
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Sun Nov 19 01:31:42 2017 UTC