[PATCH] array_combine is not binary-key safe

19 years ago by Matt W — view source

unread

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

19 years ago by Richard Quadling — view source

unread

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() ?

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
Zend Certified Engineer : http://zend.com/zce.php?c=ZEND002498&r=213474731
"Standing on the shoulders of some very clever giants!"

19 years ago by Matt W — view source

unread

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() ?

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

19 years ago by Andrei Zmievski — view source

unread

Yeah, that's probably a good idea. You can submit a patch if you
want. :)

-Andrei

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

19 years ago by Matt W — view source

unread

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

Yeah, that's probably a good idea. You can submit a patch if you
want. :)

-Andrei

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

19 years ago by bertrand Gugger — view source

unread

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 ?

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

19 years ago by Matt W — view source

unread

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

19 years ago by Andrei Zmievski — view source

unread

Fixed now.

-Andrei

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

19 years ago by Richard Quadling — view source

unread

Is the patch to use the _ex functions needed?

Fixed now.

-Andrei

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

--

--

--

Richard Quadling
Zend Certified Engineer : http://zend.com/zce.php?c=ZEND002498&r=213474731
"Standing on the shoulders of some very clever giants!"

19 years ago by Ilia Alshanetsky — view source

unread

Most compiler (I know gcc and Visual C do) will optimize strlen
("static_string").

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() ?

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

--

Ilia Alshanetsky

19 years ago by Richard Quadling — view source

unread

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.

Most compiler (I know gcc and Visual C do) will optimize
strlen("static_string").

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&r=213474731
"Standing on the shoulders of some very clever giants!"

19 years ago by Richard Quadling — view source

unread

Hi. Sorry about the delay. Had a blonde moment.

I've used a small script to process the CVS. There are 420 changes!!!

I've used the following regexps for a search/replace ...

Everything SEEMS to look ok. Until I've got MS VC Express Edition
compiling ok, this patch is UNTESTED!! (Sorry about that).

Files changed.

php-src/ext/bz2/bz2.c
php-src/ext/calendar/calendar.c
php-src/ext/curl/multi.c
php-src/ext/curl/streams.c
php-src/ext/date/php_date.c
php-src/ext/dbase/dbase.c
php-src/ext/exif/exif.c
php-src/ext/fbsql/php_fbsql.c
php-src/ext/fdf/fdf.c
php-src/ext/gd/gd.c
php-src/ext/gmp/gmp.c
php-src/ext/hwapi/hwapi.cpp
php-src/ext/iconv/iconv.c
php-src/ext/interbase/ibase_blobs.c
php-src/ext/interbase/ibase_query.c
php-src/ext/interbase/ibase_service.c
php-src/ext/ldap/ldap.c
php-src/ext/mbstring/mbstring.c
php-src/ext/oci8/oci8_interface.c
php-src/ext/openssl/openssl.c
php-src/ext/pdo/pdo_stmt.c
php-src/ext/pdo_mysql/mysql_statement.c
php-src/ext/pdo_pgsql/pgsql_statement.c
php-src/ext/pdo_sqlite/sqlite_statement.c
php-src/ext/pgsql/pgsql.c
php-src/ext/posix/posix.c
php-src/ext/session/session.c
php-src/ext/soap/php_http.c
php-src/ext/sockets/sockets.c
php-src/ext/standard/basic_functions.c
php-src/ext/standard/datetime.c
php-src/ext/standard/dns.c
php-src/ext/standard/file.c
php-src/ext/standard/html.c
php-src/ext/standard/image.c
php-src/ext/standard/microtime.c
php-src/ext/standard/proc_open.c
php-src/ext/standard/streamsfuncs.c
php-src/ext/standard/string.c
php-src/ext/standard/url.c
php-src/ext/sysvmsg/sysvmsg.c
php-src/main/output.c
php-src/main/streams/filter.c
php-src/main/streams/memory.c
php-src/main/streams/xp_socket.c

Patch is available at
http://rquadling.phpnet.us/add_assoc_xxx__to__add_assoc_xxx_ex__diff.txt

Regards,

Richard Quadling.

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

Yeah, that's probably a good idea. You can submit a patch if you
want. :)

-Andrei

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&r=213474731
"Standing on the shoulders of some very clever giants!"

19 years ago by Matt W — view source

unread

Hi Richard,

Replacement script made it pretty simple I guess. :-) But sizeof("key")+1
should just be sizeof("key") (which == strlen("key")+1).

Are there any uses of add_u_assoc_* with hard-coded keys?

These changes make things look a little messy, you think? The string is in
2 places, etc... (Maybe why it wasn't done in the first place?) Seems like
that ZEND_STRS() macro you mentioned would work well. Or maybe cleanest
would be some new add_static_assoc_* macros (and _u versions if needed), or
some other naming convention, that uses sizeof() instead of strlen()+1.

What do the main devs think?

Matt

----- Original Message -----
From: "Richard Quadling"
Sent: Saturday, July 22, 2006

Hi. Sorry about the delay. Had a blonde moment.

I've used a small script to process the CVS. There are 420 changes!!!

I've used the following regexps for a search/replace ...

$s_search =
'add_assoc_(bool|double|long|null|resource|rt_stringl|rt_string|stringl|str ing|unicodel|unicode|zval)*\(([^,]*), "(.*?)",i';
$s_replace = 'add_assoc_$1_ex($2, "$3", sizeof("$3")+1,';

Everything SEEMS to look ok. Until I've got MS VC Express Edition
compiling ok, this patch is UNTESTED!! (Sorry about that).

Files changed.

php-src/ext/bz2/bz2.c
php-src/ext/calendar/calendar.c
php-src/ext/curl/multi.c
php-src/ext/curl/streams.c
php-src/ext/date/php_date.c
php-src/ext/dbase/dbase.c
php-src/ext/exif/exif.c
php-src/ext/fbsql/php_fbsql.c
php-src/ext/fdf/fdf.c
php-src/ext/gd/gd.c
php-src/ext/gmp/gmp.c
php-src/ext/hwapi/hwapi.cpp
php-src/ext/iconv/iconv.c
php-src/ext/interbase/ibase_blobs.c
php-src/ext/interbase/ibase_query.c
php-src/ext/interbase/ibase_service.c
php-src/ext/ldap/ldap.c
php-src/ext/mbstring/mbstring.c
php-src/ext/oci8/oci8_interface.c
php-src/ext/openssl/openssl.c
php-src/ext/pdo/pdo_stmt.c
php-src/ext/pdo_mysql/mysql_statement.c
php-src/ext/pdo_pgsql/pgsql_statement.c
php-src/ext/pdo_sqlite/sqlite_statement.c
php-src/ext/pgsql/pgsql.c
php-src/ext/posix/posix.c
php-src/ext/session/session.c
php-src/ext/soap/php_http.c
php-src/ext/sockets/sockets.c
php-src/ext/standard/basic_functions.c
php-src/ext/standard/datetime.c
php-src/ext/standard/dns.c
php-src/ext/standard/file.c
php-src/ext/standard/html.c
php-src/ext/standard/image.c
php-src/ext/standard/microtime.c
php-src/ext/standard/proc_open.c
php-src/ext/standard/streamsfuncs.c
php-src/ext/standard/string.c
php-src/ext/standard/url.c
php-src/ext/sysvmsg/sysvmsg.c
php-src/main/output.c
php-src/main/streams/filter.c
php-src/main/streams/memory.c
php-src/main/streams/xp_socket.c

Patch is available at
http://rquadling.phpnet.us/add_assoc_xxx__to__add_assoc_xxx_ex__diff.txt

Regards,

Richard Quadling.

19 years ago by Matt W — view source

unread

Hi Richard, Andrei,

Hold on about a patch to optimize add_assoc_* and hard-coded keys. I just
finished some testing to see what difference it makes: none! :-) Compilers
must optimize strlen("static string") to the literal value; at least VC++
Express does. Using a dynamic string from estrdup(), *_ex with the
hard-coded length was about 10% faster with a 12 character key on my old
system (dual 930MHz). But again, no difference with static ones like in
PHP's code. It was an optimization though in array_combine(), being dynamic
key values.

So, nothin' to worry about here... :-)

Matt

----- Original Message -----
From: "Matt W"
Sent: Saturday, July 22, 2006

Hi Richard,

Replacement script made it pretty simple I guess. :-) But sizeof("key")+1
should just be sizeof("key") (which == strlen("key")+1).

Are there any uses of add_u_assoc_* with hard-coded keys?

These changes make things look a little messy, you think? The string is
in
2 places, etc... (Maybe why it wasn't done in the first place?) Seems
like
that ZEND_STRS() macro you mentioned would work well. Or maybe cleanest
would be some new add_static_assoc_* macros (and _u versions if needed),
or
some other naming convention, that uses sizeof() instead of strlen()+1.

What do the main devs think?

Matt

[PATCH] array_combine is not binary-key safe

--

Sorry for the noise , case I did something wrong , but what ? (or case it is repaired in the mean time)

--

--

Sorry for the noise , case I did something wrong , but what ?
(or case it is repaired in the mean time)