Skip to content

Fix bug #80332 Completely broken array access functionality with DOMNamedNodeMap #10829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix bug #80332 Completely broken array access functionality with DOMNamedNodeMap #10829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,8 @@ PHP NEWS
- DOM:
. Fixed bug #79451 (DOMDocument->replaceChild on doctype causes double free).
(Nathan Freeman)
. Fixed bug #80332 (Completely broken array access functionality with
DOMNamedNodeMap). (Nathan Freeman)

- FPM:
. Fixed bug GH-8885 (FPM access.log with stderr begins to write logs to
Expand Down
19 changes: 17 additions & 2 deletions ext/dom/php_dom.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#endif

#include "php.h"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do unrelated whitespace changes.

#if defined(HAVE_LIBXML) && defined(HAVE_DOM)
#include "ext/standard/php_rand.h"
#include "php_dom.h"
Expand Down Expand Up @@ -1519,7 +1520,14 @@ static zval *dom_nodelist_read_dimension(zend_object *object, zval *offset, int
return NULL;
}

ZVAL_LONG(&offset_copy, zval_get_long(offset));
zend_long lval;
if (Z_TYPE_P(offset) == IS_LONG) {
ZVAL_LONG(&offset_copy, Z_LVAL_P(offset));
} else if (Z_TYPE_P(offset) == IS_STRING && is_numeric_string(Z_STRVAL_P(offset), Z_STRLEN_P(offset), &lval, NULL, false) == IS_LONG) {
Copy link
Member

@nielsdos nielsdos Mar 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty big behavioural change, and a BC break.
Offsets like 0.0, false, null previously worked and would all implicitly convert to 0. Now it would result in a NULL element. Similarly, true would be converted to 1 but now it also always results in NULL. In particular it's also dangerous that floating point numbers that don't have a decimal part will now break silently.

I think you need to place the numeric string check first, and only then fall back using zval_get_long which will do these kind of converts for you.
Perhaps an even better solution is just adding proper support for using $a->attributes["my_attribute_name_here"], which is what the original bug report complained about. But I think that's not the expected API, so the current idea to check for numeric strings might be fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the ZPP (something like zend_parse_parameter_long() in Zend_API.h) API might make a lot of sense for this use case as it will already handle those weird cases. Another possible function, which is not a public API is zendi_try_get_long() defined in zend_operators.c

ZVAL_LONG(&offset_copy, lval);
} else {
return NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't just return NULL here, because in that case read_dimension() expects to get an exception.
i.e. the following code (from the bug report) will crash PHP with an assertion failure in debug mode:

<?php

$doc = new DOMDocument;
$doc->loadHTML('<a href="https://example.com">hi!</a>');
$a = (new DOMXPath($doc))->query('//a')[0];
$a->attributes['href']->nodeValue = 'https://example.net';
print $doc->saveHTML();

}

zend_call_method_with_1_params(object, object->ce, NULL, "item", rv, &offset_copy);

Expand All @@ -1528,7 +1536,14 @@ static zval *dom_nodelist_read_dimension(zend_object *object, zval *offset, int

static int dom_nodelist_has_dimension(zend_object *object, zval *member, int check_empty)
{
zend_long offset = zval_get_long(member);
zend_long lval;
zend_long offset = -1;
if (Z_TYPE_P(member) == IS_LONG) {
offset = Z_LVAL_P(member);
} else if (Z_TYPE_P(member) == IS_STRING && is_numeric_string(Z_STRVAL_P(member), Z_STRLEN_P(member), &lval, NULL, false) == IS_LONG) {
offset = lval;
}

zval rv;

if (offset < 0) {
Expand Down
12 changes: 8 additions & 4 deletions ext/dom/tests/bug67949.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,16 @@ array(1) {
string(4) "test"
}
string(4) "test"
bool(true)
string(4) "data"

Warning: Attempt to read property "textContent" on null in %s on line %d
bool(false)
NULL
string(4) "test"
string(4) "test"
bool(true)
string(4) "data"

Warning: Attempt to read property "textContent" on null in %s on line %d
bool(false)
NULL
string(4) "test"
testing read_dimension with null offset
Cannot access node list without offset
Expand Down
86 changes: 86 additions & 0 deletions ext/dom/tests/bug80332.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
--TEST--
Bug #80332 (Completely broken array access functionality with DOMNamedNodeMap)
--EXTENSIONS--
dom
--FILE--
<?php
$doc = new DOMDocument;
$doc->loadHTML('<span attr1="value1" attr2="value2"></span>');

$x = new DOMXPath($doc);
$span = $x->query('//span')[0];

var_dump($span->attributes[0]->nodeName);
var_dump($span->attributes[0]->nodeValue);

var_dump($span->attributes['1']->nodeName);
var_dump($span->attributes['1']->nodeValue);

var_dump($span->attributes[0.6]->nodeName);
var_dump($span->attributes[0.6]->nodeValue);

var_dump($span->attributes['0.6']->nodeName);
var_dump($span->attributes['0.6']->nodeValue);

var_dump($span->attributes[12345678]->nodeName);
var_dump($span->attributes[12345678]->nodeValue);

var_dump($span->attributes['12345678']->nodeName);
var_dump($span->attributes['12345678']->nodeValue);

var_dump($span->attributes[9999999999999999999999999999999999]->nodeName);
var_dump($span->attributes[9999999999999999999999999999999999]->nodeValue);

var_dump($span->attributes['9999999999999999999999999999999999']->nodeName);
var_dump($span->attributes['9999999999999999999999999999999999']->nodeValue);

var_dump($span->attributes['hi']->nodeName);
var_dump($span->attributes['hi']->nodeValue);
?>
--EXPECTF--
string(5) "attr1"
string(6) "value1"
string(5) "attr2"
string(6) "value2"

Warning: Attempt to read property "nodeName" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeValue" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeName" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeValue" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeName" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeValue" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeName" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeValue" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeName" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeValue" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeName" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeValue" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeName" on null in %s on line %d
NULL

Warning: Attempt to read property "nodeValue" on null in %s on line %d
NULL