Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [PATCH] array_combine is not binary-key safe »

[PATCH] array_combine is not binary-key safe

php.internals

Matt W

20 years ago
Hi, I don't know if array_combine() was intentionally made binary-key unsafe, but it seems wrong and inconsistent since binary keys work everywhere else I can think of -- including array_flip() and the new array_fill_keys(). And updating it is a bit of an optimization by eliminating strlen() calls. :-) I also changed the error message to be more grammatically-correct and fixed some typos in the description. http://www.realplain.com/php/array_combine_binkey.diff http://www.realplain.com/php/array_combine_binkey_5_2.diff Thanks, Matt

Richard Quadling

20 years ago
Hi, There are 46 uses of add_assoc_zval() in the CVS. Many are with fixed length strings for the key. Should the others all be using add_assoc_zval_ex() ? On 21/07/06, Matt W <php_lists@realplain.com> wrote:
> Hi, > > I don't know if array_combine() was intentionally made binary-key unsafe, > but it seems wrong and inconsistent since binary keys work everywhere else I > can think of -- including array_flip() and the new array_fill_keys(). And > updating it is a bit of an optimization by eliminating strlen() calls. :-) > > I also changed the error message to be more grammatically-correct and fixed > some typos in the description. > > http://www.realplain.com/php/array_combine_binkey.diff > http://www.realplain.com/php/array_combine_binkey_5_2.diff > > > Thanks, > Matt > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
-- ----- Richard Quadling Zend Certified Engineer : http://zend.com/zce.php?c=ZEND002498&amp;r=213474731 "Standing on the shoulders of some very clever giants!"

Matt W

20 years ago
Hi Richard, I think I've seen those instances that you're referring to. By fixed length string I assume you mean hard-coded "string_key". Yeah, I would think those should use add_assoc_*_ex() since the length is known (sizeof("string_key") etc.) to save unnecessary strlen() calls. Unless compilers optimize the strlen("string_key") + 1 to a constant from the add_assoc_*() macro. But I wouldn't think that's the case...? :-/ Matt ----- Original Message ----- From: "Richard Quadling" Sent: Friday, July 21, 2006
> Hi, > > There are 46 uses of add_assoc_zval() in the CVS. Many are with fixed > length strings for the key. Should the others all be using > add_assoc_zval_ex() ? > > On 21/07/06, Matt W <php_lists@realplain.com> wrote: > > Hi, > > > > I don't know if array_combine() was intentionally made binary-key
unsafe,
> > but it seems wrong and inconsistent since binary keys work everywhere
else I
> > can think of -- including array_flip() and the new array_fill_keys().
And
> > updating it is a bit of an optimization by eliminating strlen() calls.
:-)
> > > > I also changed the error message to be more grammatically-correct and
fixed

Andrei Zmievski

20 years ago
Yeah, that's probably a good idea. You can submit a patch if you want. :) -Andrei On Jul 21, 2006, at 6:04 AM, Matt W wrote:

Matt W

20 years ago
Hi Andrei, I see you applied my patch. However, the 5.2 code still isn't binary-key safe (you only changed the second occurrence of add_assoc_zval to the _ex version). Or was that intentional and you only want to change the behavior in 6? And you know 5.2's description is still wrong -- with "keys" at the end instead of "values"? :-) When you changed that part in HEAD last week, you also added a "the" -- "... as _the_ corresponding _values_" -- which was in my patch, if you want both branches *exactly* the same. :-P Matt P.S. The other patch you're talking about below... I think Richard Quadling said he'll do it. ----- Original Message ----- From: "Andrei Zmievski" Sent: Friday, July 21, 2006

bertrand Gugger

20 years ago
Matt W wrote:
> Hi Andrei, > > I see you applied my patch.
Testing with a php5.2-200607222030 snaps having /* $Id: array.c,v 1.308.2.21.2.7 2006/07/22 16:58:39 andrei Exp $ */ Looks by me as the ext/standard/tests/array/array_combine.phpt fails Is it by me ? $ diff -W 60 -y --suppress-common-lines ext/standard/tests/array/array_combine.exp ext/standard/tests/array/array_combine.out [green] => green | [gree] => green [red] => red | [re] => red [yellow] => yellow | [yello] => yellow [green] => 1 | [gree] => 1 [red] => 2 | [re] => 2 [yellow] => 3 | [yello] => 3 [green] => 0 | [gree] => 0 [red] => 1 | [re] => 1 [yellow] => 2 | [yello] => 2 [green] => 1 | [gree] => 1 [red] => | [re] => [yellow] => | [yello] => [1] => green | [] => green [2] => red | [] => red [3] => yellow | [] => yellow [1] => 1 | [] => 1 [2] => 2 | [] => 2 [3] => 3 | [] => 3 [1] => 0 | [] => 0 [2] => 1 | [] => 1 [3] => 2 | [] => 2 [1] => 1 | [] => 1 [2] => | [] => [3] => | [] => PHP : /home/bertrand/php/php5.2-200607222030/sapi/cli/php PHP_SAPI : cli PHP_VERSION : 5.2.0-dev ZEND_VERSION: 2.2.0 PHP_OS : Linux - Linux ancilla.toggg.net 2.6.12-1.1381_FC3 #1 Fri Oct 21 03 :46:55 EDT 2005 i686 Configure Command => './configure' Sorry for the noise , case I did something wrong , but what ? (or case it is repaired in the mean time)
-- toggg

Matt W

20 years ago
Hi, ----- Original Message ----- From: "bertrand Gugger" Sent: Monday, July 24, 2006
> Matt W wrote: > > Hi Andrei, > > > > I see you applied my patch. > Testing with a php5.2-200607222030 snaps having > /* $Id: array.c,v 1.308.2.21.2.7 2006/07/22 16:58:39 andrei Exp $ */ > > Looks by me as the ext/standard/tests/array/array_combine.phpt fails > Is it by me ? > > $ diff -W 60 -y --suppress-common-lines > ext/standard/tests/array/array_combine.exp > ext/standard/tests/array/array_combine.out > [green] => green | [gree] => green > [red] => red | [re] => red > [yellow] => yellow | [yello] => yellow > [green] => 1 | [gree] => 1 > [red] => 2 | [re] => 2 > [yellow] => 3 | [yello] => 3 > [green] => 0 | [gree] => 0 > [red] => 1 | [re] => 1 > [yellow] => 2 | [yello] => 2 > [green] => 1 | [gree] => 1 > [red] => | [re] => > [yellow] => | [yello] => > [1] => green | [] => green > [2] => red | [] => red > [3] => yellow | [] => yellow > [1] => 1 | [] => 1 > [2] => 2 | [] => 2 > [3] => 3 | [] => 3 > [1] => 0 | [] => 0 > [2] => 1 | [] => 1 > [3] => 2 | [] => 2 > [1] => 1 | [] => 1 > [2] => | [] => > [3] => | [] => > > PHP : /home/bertrand/php/php5.2-200607222030/sapi/cli/php > PHP_SAPI : cli > PHP_VERSION : 5.2.0-dev > ZEND_VERSION: 2.2.0 > PHP_OS : Linux - Linux ancilla.toggg.net 2.6.12-1.1381_FC3 #1 Fri > Oct 21 03 :46:55 EDT 2005 i686 > > Configure Command => './configure' > > Sorry for the noise , case I did something wrong , but what ? > (or case it is repaired in the mean time)
Nope, it's broken. :-) I was confused as to why Andrei only fully applied my patch for HEAD, and seemed to manually "take part of" the 5.2 patch (I thought the binary key change was unwanted in 5.2, but he did update it then). Now it needs a 4th update... *shrug* :-) Andrei, in the first add_assoc_zval_ex(), after Z_STRLEN_PP(entry_keys), you forgot the +1.
> -- > toggg
It wasn't your fault, good catch. ;-) Matt

bertrand Gugger

20 years ago
Matt W wrote:
> > Nope, it's broken. :-)
Thx to confirm the report , It's old linux here , so maybe it's related. I did a full report of the whole run-tests with the snap from 06:30 this morning and this minimal build. (attached for complements of info) off topic: As recommended (7 other failures , 4 in ext/filter/tests/ ), I sent it to qa-reports@lists.php.net , however as ezmlm rejected it because message larger than 30000 bytes , I had to pack it (would be possibly a good idea that the procedures pack it itself). To avoid double reports , in such next release period ,could be a good idea too to refresh http://gcov.php.net/ too. à+
-- toggg

Andrei Zmievski

20 years ago
Fixed now. -Andrei On Jul 24, 2006, at 12:52 AM, Matt W wrote:

Richard Quadling

20 years ago
Is the patch to use the _ex functions needed? On 24/07/06, Andrei Zmievski <andrei@gravitonic.com> wrote:
> Fixed now. > > -Andrei > > > On Jul 24, 2006, at 12:52 AM, Matt W wrote: > > > Hi, > > > > ----- Original Message ----- > > From: "bertrand Gugger" > > Sent: Monday, July 24, 2006 > > > > > >> Matt W wrote: > >>> Hi Andrei, > >>> > >>> I see you applied my patch. > >> Testing with a php5.2-200607222030 snaps having > >> /* $Id: array.c,v 1.308.2.21.2.7 2006/07/22 16:58:39 andrei Exp $ */ > >> > >> Looks by me as the ext/standard/tests/array/array_combine.phpt fails > >> Is it by me ? > >> > >> $ diff -W 60 -y --suppress-common-lines > >> ext/standard/tests/array/array_combine.exp > >> ext/standard/tests/array/array_combine.out > >> [green] => green | [gree] => green > >> [red] => red | [re] => red > >> [yellow] => yellow | [yello] => yellow > >> [green] => 1 | [gree] => 1 > >> [red] => 2 | [re] => 2 > >> [yellow] => 3 | [yello] => 3 > >> [green] => 0 | [gree] => 0 > >> [red] => 1 | [re] => 1 > >> [yellow] => 2 | [yello] => 2 > >> [green] => 1 | [gree] => 1 > >> [red] => | [re] => > >> [yellow] => | [yello] => > >> [1] => green | [] => green > >> [2] => red | [] => red > >> [3] => yellow | [] => yellow > >> [1] => 1 | [] => 1 > >> [2] => 2 | [] => 2 > >> [3] => 3 | [] => 3 > >> [1] => 0 | [] => 0 > >> [2] => 1 | [] => 1 > >> [3] => 2 | [] => 2 > >> [1] => 1 | [] => 1 > >> [2] => | [] => > >> [3] => | [] => > >> > >> PHP : /home/bertrand/php/php5.2-200607222030/sapi/cli/php > >> PHP_SAPI : cli > >> PHP_VERSION : 5.2.0-dev > >> ZEND_VERSION: 2.2.0 > >> PHP_OS : Linux - Linux ancilla.toggg.net 2.6.12-1.1381_FC3 #1 > >> Fri > >> Oct 21 03 :46:55 EDT 2005 i686 > >> > >> Configure Command => './configure' > >> > >> Sorry for the noise , case I did something wrong , but what ? > >> (or case it is repaired in the mean time) > > > > Nope, it's broken. :-) I was confused as to why Andrei only fully > > applied > > my patch for HEAD, and seemed to manually "take part of" the 5.2 > > patch (I > > thought the binary key change was unwanted in 5.2, but he did > > update it > > then). Now it needs a 4th update... *shrug* :-) > > > > Andrei, in the first add_assoc_zval_ex(), after Z_STRLEN_PP > > (entry_keys), you > > forgot the +1. > > > >> -- > >> toggg > > > > It wasn't your fault, good catch. ;-) > > > > > > Matt > > > > -- > > PHP Internals - PHP Runtime Development Mailing List > > To unsubscribe, visit: http://www.php.net/unsub.php > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
-- ----- Richard Quadling Zend Certified Engineer : http://zend.com/zce.php?c=ZEND002498&amp;r=213474731 "Standing on the shoulders of some very clever giants!"

Ilia A.

20 years ago
Most compiler (I know gcc and Visual C do) will optimize strlen ("static_string"). On 21-Jul-06, at 9:04 AM, Matt W wrote:
> Hi Richard, > > I think I've seen those instances that you're referring to. By > fixed length > string I assume you mean hard-coded "string_key". Yeah, I would > think those > should use add_assoc_*_ex() since the length is known (sizeof > ("string_key") > etc.) to save unnecessary strlen() calls. > > Unless compilers optimize the strlen("string_key") + 1 to a > constant from > the add_assoc_*() macro. But I wouldn't think that's the case...? :-/ > > > Matt > > > ----- Original Message ----- > From: "Richard Quadling" > Sent: Friday, July 21, 2006 > >> Hi, >> >> There are 46 uses of add_assoc_zval() in the CVS. Many are with >> fixed >> length strings for the key. Should the others all be using >> add_assoc_zval_ex() ? >> >> On 21/07/06, Matt W <php_lists@realplain.com> wrote: >>> Hi, >>> >>> I don't know if array_combine() was intentionally made binary-key > unsafe, >>> but it seems wrong and inconsistent since binary keys work >>> everywhere > else I >>> can think of -- including array_flip() and the new array_fill_keys >>> (). > And >>> updating it is a bit of an optimization by eliminating strlen() >>> calls. > :-) >>> >>> I also changed the error message to be more grammatically-correct >>> and > fixed >>> some typos in the description. >>> >>> http://www.realplain.com/php/array_combine_binkey.diff >>> http://www.realplain.com/php/array_combine_binkey_5_2.diff >>> >>> >>> Thanks, >>> Matt > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
Ilia Alshanetsky

Richard Quadling

20 years ago
Either way, is it worth committing the patch ? 1 - For the non-optimizing compilers. 2 - Consistency across all source - static string use sizeof() rather than strlen() +1 If so, I'll need to amend the patch to NOT use +1, though judging by the comments made by bertrand Gugger, maybe something needs to be revisited. On 23/07/06, Ilia Alshanetsky <ilia@prohost.org> wrote:
> > Most compiler (I know gcc and Visual C do) will optimize > strlen("static_string"). > > > > On 21-Jul-06, at 9:04 AM, Matt W wrote: > > > Hi Richard, > > I think I've seen those instances that you're referring to. By fixed length > string I assume you mean hard-coded "string_key". Yeah, I would think those > should use add_assoc_*_ex() since the length is known (sizeof("string_key") > etc.) to save unnecessary strlen() calls. > > Unless compilers optimize the strlen("string_key") + 1 to a constant from > the add_assoc_*() macro. But I wouldn't think that's the case...? :-/ > > > Matt
-- ----- Richard Quadling Zend Certified Engineer : http://zend.com/zce.php?c=ZEND002498&amp;r=213474731 "Standing on the shoulders of some very clever giants!"