php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Bug #61797 DOMNode::appendChild doesn't increment refcount
Submitted: 2012-04-21 10:37 UTC Modified: 2019-09-22 22:10 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: beberlei (profile)
Status: Wont fix Package: DOM XML related
PHP Version: 5.4.0 OS: Windows, Linux
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
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)

Pull Requests

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.
 [2015-07-10 16:04 UTC] cmb@php.net 
Related To: Bug #70026
 [2017-10-24 06:14 UTC] kalle@php.net
-Status: Assigned +Status: Open -Assigned To: rrichards +Assigned To:
 [2019-09-22 22:10 UTC] beberlei@php.net
-Status: Open +Status: Wont fix
 [2019-09-22 22:10 UTC] beberlei@php.net 
This is not a bug. DOMDocument guarantees you the return of a DOMElement from a childNode. If you pass a subclass of DOMElement, then you must register is using "registerNodeClass". Not doing so will only guarantee you a DOMElement, depending if original element fell out of scope.
 [2019-09-22 22:10 UTC] beberlei@php.net
-Assigned To: +Assigned To: beberlei
 [2019-09-22 22:33 UTC] beberlei@php.net 
In addition, you should see childNodes, documentElement and other properties holding DOMNode and subclasses as "magic" properties using __get, that return new instances as they like based on the underlying libxml structure.

ext/dom makes *no* guarantee to keep references matching to individual libxml nodes. This strategy is also used to keep memory usage low(er).
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Wed Dec 11 13:01:29 2024 UTC