Fix bug #80332 Completely broken array access functionality with DOMNamedNodeMap #10829
Conversation
NathanFreeman
force-pushed
the
bug_80332
branch
from
7b3cb9e to
0c18ffe
Compare
|
Thanks for confidence but I know too little about the dom extension 🙂. |
Thank you! |
|
cc @beberlei |
|
Is there any other developer who know more about |
|
I'm also not the best person to review this as I only did 2/3 fixes to XML/DOM in the past. Usually Christoph seems to review these DOM/XML changes but he seems quite busy. I could take a look this weekend to see if it kinda makes sense or if there are obvious mistakes, but won't be able to approve it due to my lack of expertise in this area. |
There was a problem hiding this comment.
So as I previously said, I'm not a DOM expert.
I checked the original bug report and there the reporter complained that non-numeric strings are silently converted to 0. This patch attempts to fix it by special casing numeric strings, but in doing so it unfortunately breaks some existing use cases.
Still, I think the general idea behind this patch makes sense, just the approach needs some additional work to fix the newly introduced issues.
I also wonder how much of a BC break fixing this is, even with my remarks fixed.
| @@ -21,6 +21,7 @@ | |||
| #endif | |||
|
|
|||
| #include "php.h" | |||
|
|
|||
There was a problem hiding this comment.
Please don't do unrelated whitespace changes.
| } else if (Z_TYPE_P(offset) == IS_STRING && is_numeric_string(Z_STRVAL_P(offset), Z_STRLEN_P(offset), &lval, NULL, false) == IS_LONG) { | ||
| ZVAL_LONG(&offset_copy, lval); | ||
| } else { | ||
| return NULL; |
There was a problem hiding this comment.
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_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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
No longer relevant as I fixed this in another way some months ago. Thanks for your interest anyway! |
https://bugs.php.net/bug.php?id=80332
The function
zval_get_longwill always return0if theoffsetis string, resulting in always pointing to the first element of the array.