# [PATCH] array_combine is not binary-key safe - Thread: https://externals.io/message/24833 - Original archive: https://news-web.php.net/php.internals/24833 - Started by Matt W on 2006-07-21T12:08:41+00:00 - Messages: 14 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #24833 — Matt W — 2006-07-21T12:08:41+00:00 URL: https://externals.io/message/24833#24833 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 --- ## #24836 — Richard Quadling — 2006-07-21T12:43:08+00:00 URL: https://externals.io/message/24833#24836 · In reply to: #24833 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!" --- ## #24840 — Matt W — 2006-07-21T13:04:40+00:00 URL: https://externals.io/message/24833#24840 · In reply to: #24836 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() ? > > > > […] unsafe, > > […] else I > > […] And > > […] :-) > > […] fixed > > […] --- ## #24865 — Andrei Zmievski — 2006-07-21T23:40:17+00:00 URL: https://externals.io/message/24833#24865 · In reply to: #24840 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 --- ## #24873 — Matt W — 2006-07-22T07:36:58+00:00 URL: https://externals.io/message/24833#24873 · In reply to: #24865 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 > > > > > > […] --- ## #24888 — Richard Quadling — 2006-07-22T21:54:17+00:00 URL: https://externals.io/message/24833#24888 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|string|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. > > > […] -- ----- Richard Quadling Zend Certified Engineer : http://zend.com/zce.php?c=ZEND002498&r=213474731 "Standing on the shoulders of some very clever giants!" --- ## #24892 — Matt W — 2006-07-23T03:45:27+00:00 URL: https://externals.io/message/24833#24892 · In reply to: #24888 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. --- ## #24897 — Matt W — 2006-07-23T09:23:24+00:00 URL: https://externals.io/message/24833#24897 · In reply to: #24892 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 --- ## #24912 — Ilia Alshanetsky — 2006-07-23T17:29:27+00:00 URL: https://externals.io/message/24833#24912 · In reply to: #24840 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 > > > […] > unsafe, > > […] > else I > > […] > And > > […] > :-) > > […] > fixed > > […] > > -- > > > > Ilia Alshanetsky --- ## #24918 — bertrand Gugger — 2006-07-24T06:02:21+00:00 URL: https://externals.io/message/24833#24918 · In reply to: #24873 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 --- ## #24919 — Richard Quadling — 2006-07-24T07:50:34+00:00 URL: https://externals.io/message/24833#24919 · In reply to: #24912 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!" --- ## #24920 — Matt W — 2006-07-24T07:52:52+00:00 URL: https://externals.io/message/24833#24920 · In reply to: #24918 Hi, ----- Original Message ----- From: "bertrand Gugger" Sent: Monday, July 24, 2006 > Matt W wrote: > > […] > 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 --- ## #24944 — Andrei Zmievski — 2006-07-24T18:18:35+00:00 URL: https://externals.io/message/24833#24944 · In reply to: #24920 Fixed now. -Andrei > Hi, > > ----- Original Message ----- > From: "bertrand Gugger" > Sent: Monday, July 24, 2006 > > > > […] > > 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. > > > […] > > It wasn't your fault, good catch. ;-) > > > Matt --- ## #24982 — Richard Quadling — 2006-07-26T12:02:37+00:00 URL: https://externals.io/message/24833#24982 · In reply to: #24944 Is the patch to use the _ex functions needed? > Fixed now. > > -Andrei > > > > > > […] > > -- > > > > -- ----- Richard Quadling Zend Certified Engineer : http://zend.com/zce.php?c=ZEND002498&r=213474731 "Standing on the shoulders of some very clever giants!"