# Fixing string offsets of strings. - Thread: https://externals.io/message/56734 - Original archive: https://news-web.php.net/php.internals/56734 - Started by Alan Knowles on 2011-12-03T16:08:41+00:00 - Messages: 20 - 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 --- ## #56734 — Alan Knowles — 2011-12-03T16:08:41+00:00 URL: https://externals.io/message/56734#56734 I've had a look at making string offsets of strings a bit saner. At present with the fix for array dereferencing : ?search=hello and a test like isset($_GET['search']['name']) results in true, which is has potential security problems and is very confusing for any programmer finding and working out why something like that may be failing. To solve this quite a few people agreed that not allowing non-numeric string offsets on strings would be the smart way to go, the change is going to break BC, so the idea is to at least not break it too badly... This patch is a start. https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest It's been quite a while since I hacked on the engine, so the patch only works reasonably well.. (see the FIXME on the tests at the bottom of the patch.) The patch changes the following: * $s = "string"; $s['offset'] -- produces a warning (and returns an empty string) * $s = "string"; $s['1'] -- works as before.. * $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it to an int if you want to get rid of the notice) - however work as before. * changes the warning on invalid indexes to say "Uninitialized or invalid" rather than just "Uninitialized" * fixes most of the related tests I would appreciate if someone with better engine knowledge would have a look and work out what the "BAD" usage should return. In theory.. the fetch_dim behavior should be return a empty string if an invalid offset is used, or uninitialized zval if ISSET is calling it Regards Alan --- ## #56755 — Alan Knowles — 2011-12-04T12:55:12+00:00 URL: https://externals.io/message/56734#56755 · In reply to: #56734 This is ready for review now. https://bugs.php.net/patch-display.php?bug=60362&patch=fix_disabling_bad_string_offsets&revision=1323002696 This resolves the worst behavior changes introduced by the dereferencing of strings fix. https://bugs.php.net/bug.php?id=60362 All tests (in Zend/tests) pass (after they have been modified to suit the change) Please review / comment... Regards Alan > I've had a look at making string offsets of strings a bit saner. > > At present with the fix for array dereferencing : ?search=hello and a > test like isset($_GET['search']['name']) results in true, which is > has potential security problems and is very confusing for any > programmer finding and working out why something like that may be > failing. > > To solve this quite a few people agreed that not allowing non-numeric > string offsets on strings would be the smart way to go, the change is > going to break BC, so the idea is to at least not break it too badly... > > This patch is a start. > https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest > > > It's been quite a while since I hacked on the engine, so the patch > only works reasonably well.. (see the FIXME on the tests at the bottom > of the patch.) > > The patch changes the following: > * $s = "string"; $s['offset'] -- produces a warning (and returns an > empty string) > * $s = "string"; $s['1'] -- works as before.. > * $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast > it to an int if you want to get rid of the notice) - however work as > before. > * changes the warning on invalid indexes to say "Uninitialized or > invalid" rather than just "Uninitialized" > * fixes most of the related tests > > I would appreciate if someone with better engine knowledge would have > a look and work out what the "BAD" usage should return. > > In theory.. the fetch_dim behavior should be return a empty string if > an invalid offset is used, or uninitialized zval if ISSET is calling it > > Regards > Alan --- ## #56756 — Ilia Alshanetsky — 2011-12-04T13:28:29+00:00 URL: https://externals.io/message/56734#56756 · In reply to: #56755 Seems like a good change +1. > This is ready for review now. > > https://bugs.php.net/patch-display.php?bug=60362&patch=fix_disabling_bad_string_offsets&revision=1323002696 > > This resolves the worst behavior changes introduced by the dereferencing of > strings fix. > https://bugs.php.net/bug.php?id=60362 > > All tests (in Zend/tests) pass (after they have been modified to suit the > change) > > Please review / comment... > > Regards > Alan > > > > > > […] --- ## #56757 — Pierre Joye — 2011-12-04T13:40:37+00:00 URL: https://externals.io/message/56734#56757 · In reply to: #56756 hi! same here. Thanks for this patch! > Seems like a good change +1. Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #56758 — Felipe Pena — 2011-12-04T13:44:03+00:00 URL: https://externals.io/message/56734#56758 · In reply to: #56755 Hi, 2011/12/4 Alan Knowles : > This is ready for review now. > > https://bugs.php.net/patch-display.php?bug=60362&patch=fix_disabling_bad_string_offsets&revision=1323002696 > > This resolves the worst behavior changes introduced by the dereferencing of > strings fix. > https://bugs.php.net/bug.php?id=60362 > > All tests (in Zend/tests) pass (after they have been modified to suit the > change) > > Please review / comment... > > Regards > Alan > > > > > > […] > > > -- > > > Take a look at Zend/tests/offset_assign.phpt, there is a path hardcoded. -- Regards, Felipe Pena --- ## #56759 — Ferenc Kovacs — 2011-12-04T14:05:02+00:00 URL: https://externals.io/message/56734#56759 · In reply to: #56734 > I've had a look at making string offsets of strings a bit saner. > > At present with the fix for array dereferencing : ?search=hello and a > test like isset($_GET['search']['name']) results in true, which is has > potential security problems and is very confusing for any programmer > finding and working out why something like that may be failing. > > To solve this quite a few people agreed that not allowing non-numeric > string offsets on strings would be the smart way to go, the change is going > to break BC, so the idea is to at least not break it too badly... > > This patch is a start. > https://bugs.php.net/patch-**display.php?bug_id=60362&** > patch=first_effort_to_fix_**this&revision=latest > > It's been quite a while since I hacked on the engine, so the patch only > works reasonably well.. (see the FIXME on the tests at the bottom of the > patch.) > > The patch changes the following: > * $s = "string"; $s['offset'] -- produces a warning (and returns an > empty string) > * $s = "string"; $s['1'] -- works as before.. > * $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it to > an int if you want to get rid of the notice) - however work as before. > * changes the warning on invalid indexes to say "Uninitialized or > invalid" rather than just "Uninitialized" > * fixes most of the related tests > I think that those changes are pretty much in line with the discussion that we had. Thanks for fixing this! -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #56761 — Laruence — 2011-12-04T14:25:41+00:00 URL: https://externals.io/message/56734#56761 · In reply to: #56759 +1. thanks. > > > > […] > > I think that those changes are pretty much in line with the discussion that > we had. > Thanks for fixing this! > > > -- > Ferenc Kovács > @Tyr43l - http://tyrael.hu -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #56766 — Etienne Kneuss — 2011-12-04T16:39:30+00:00 URL: https://externals.io/message/56734#56766 · In reply to: #56761 Hi, > +1. > > thanks. > > > > […] > going > > […] > https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest > > […] > to > > […] > What about other edge cases like $string[' 2 '], $string['2foo']? I like the idea of the patch, I just find it a bit inconsistent for $s['offset'] to return an empty string while other cases of implicit conversions work as before. It doesn't bring much to return an empty string instead of the first char. I believe every case should work as before, throwing a notice is enough IMO. Also, you don't mention whether your patch modifies the behavior of isset(), is $str = "foo"; isset($str['bar']) true or false ? Best, > > […] > that > > […] > > > > -- > Laruence Xinchen Hui > http://www.laruence.com/ > > -- > > > > -- Etienne Kneuss http://www.colder.ch --- ## #56767 — Laruence — 2011-12-04T16:55:18+00:00 URL: https://externals.io/message/56734#56767 · In reply to: #56766 > Hi, > > > > […] > > > What about other edge cases like $string['   2   '], $string['2foo']? > > I like the idea of the patch, I just find it a bit inconsistent for > $s['offset'] to return an empty string while other cases of implicit > conversions work as before. It doesn't bring much to return an empty string > instead of the first char. I believe every case should work as before, > throwing a notice is enough IMO. I agree after a deep think, a notice is enough. thanks > > Also, you don't mention whether your patch modifies the behavior of isset(), > is $str = "foo"; isset($str['bar'])  true or false ? > > Best, > > > […] > > > > -- > Etienne Kneuss > http://www.colder.ch -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #56768 — johannes@schlueters.de — 2011-12-04T16:55:42+00:00 URL: https://externals.io/message/56734#56768 · In reply to: #56734 > This patch is a start. > https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest > > It's been quite a while since I hacked on the engine, so the patch only > works reasonably well.. (see the FIXME on the tests at the bottom of the > patch.) > > The patch changes the following: > * $s = "string"; $s['offset'] -- produces a warning (and returns an > empty string) > * $s = "string"; $s['1'] -- works as before.. > * $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it > to an int if you want to get rid of the notice) - however work as before. > * changes the warning on invalid indexes to say "Uninitialized or > invalid" rather than just "Uninitialized" > * fixes most of the related tests I think it's bad to add another set of casting rules to the language. I'd prefer splitting the string offset operator from array offset. $a = [0,1,2,3]; $s = "string"; $a{0}; // wrong $s{0}; // ok $a[0]; // ok $s[0]; // wrong Yes, something like this has been discussed before, back and forth and doing this is too late for 5.4, but let's look to 5.5. johannes --- ## #56771 — Laruence — 2011-12-04T17:28:38+00:00 URL: https://externals.io/message/56734#56771 · In reply to: #56761 Hi: I have submit a new patch based on the origin patch, which only trigger notice when string offset cast occurred. thanks > +1. > > thanks. > > > > […] > > > > -- > Laruence  Xinchen Hui > http://www.laruence.com/ -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #56774 — Alan Knowles — 2011-12-04T23:25:16+00:00 URL: https://externals.io/message/56734#56774 · In reply to: #56771 A few answers... $s = "string"; isset($s['offset']) returns false This is pretty critical, as it's the only way to detect this situation, and ensure that array tests do not return positive results for strings. It also catches an obvious, but previously hidden and probably serious bugs in the PHP code.. $s = "string"; $s[' 2 ']; and $s['2foo'] both emit errors, and return false from isset() - the code pretty much uses `is_numeric()` internally. - I'm pretty sure these would be un-intentional bugs, so behaving as such seems consistent. I do not mind either way if $s['offset'] returns the first char or an empty string, however it seemed a little inconstant to return false on isset(), yet actually return a string. (although technically an empty string does kind of indicate it is 'isset') - it at least leaves the engine in a consistent state. Perhaps that change can wait's until 5.5.. thoughts... As for dropping the syntax for Strings eventually.. It would be nice, but I'm not sure it is feasible anymore unfortunately. Regards Alan > Hi: > I have submit a new patch based on the origin patch, which only > trigger notice when string offset cast occurred. > > thanks > > > > […] --- ## #56775 — Ferenc Kovacs — 2011-12-04T23:28:28+00:00 URL: https://externals.io/message/56734#56775 · In reply to: #56768 2011/12/4 Johannes Schlüter > > > […] > https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest > > […] > > I think it's bad to add another set of casting rules to the language. > I'd prefer splitting the string offset operator from array offset. > > > $a = [0,1,2,3]; > $s = "string"; > > $a{0}; // wrong > $s{0}; // ok > $a[0]; // ok > $s[0]; // wrong > > Yes, something like this has been discussed before, back and forth and > doing this is too late for 5.4, but let's look to 5.5. > > this would have a much bigger impact than the currently proposed change (more people use [] than $foo['bar'] where $foo is a string. and while I think this would make the language much more cleaner, and explicit, this would be a major U-turn. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #56777 — Laruence — 2011-12-05T03:21:29+00:00 URL: https://externals.io/message/56734#56777 · In reply to: #56774 Hi: I think we can only trigger notice, then act the same behavior as before. include isset. this would introduce the fewest bc breaks, what do you think? thanks > A few answers... > > $s = "string";  isset($s['offset']) returns false > This is pretty critical, as it's the only way to detect this situation, and > ensure that array tests do not return positive results for strings. It also > catches an obvious, but previously hidden and probably serious bugs in the > PHP code.. > > $s = "string";  $s['  2  ']; and $s['2foo'] both emit errors, and return > false from isset() - the code pretty much uses `is_numeric()` internally. - > I'm pretty sure these would be un-intentional bugs, so behaving as such > seems consistent. > > I do not mind either way if $s['offset'] returns the first char or an empty > string, however it seemed a little inconstant to return false on isset(), > yet actually return a string. (although technically an empty string does > kind of indicate it is 'isset') - it at least leaves the engine in a > consistent state. Perhaps that change can wait's until 5.5.. thoughts... > > As for dropping the syntax for Strings eventually.. It would be nice, but > I'm not sure it is feasible anymore unfortunately. > > Regards > Alan > > > > > > > > […] > > > -- > > > -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #56778 — Alan Knowles — 2011-12-05T03:52:14+00:00 URL: https://externals.io/message/56734#56778 · In reply to: #56777 > Hi: > I think we can only trigger notice, then act the same behavior as > before. include isset. > > this would introduce the fewest bc breaks, I think the isset behavior should be fixed (as the BC will be broken anyway with deferenced strings), for the other fix, If it was my project (haha), I'd release the empty string change, and revert it quickly in the next release if there were any serious bug reports saying working code was affected by it. Not sure if that's a great approach, but I'd be interested in hearing opinions about it. Regards Alan > what do you think? > > thanks > > > > […] --- ## #56779 — Daniel Convissor — 2011-12-05T14:59:50+00:00 URL: https://externals.io/message/56734#56779 · In reply to: #56755 Hi Alan: > https://bugs.php.net/patch-display.php?bug=60362&patch=fix_disabling_bad_string_offsets&revision=1323002696 Thanks so much for the patch. Looking at the source I noticed some small things (care to build a bike shed with me? :). Many of the test EXPECT's have %d changed to hard coded line numbers and others have %s changed to hard coded file names. In some of them, that's all that changed. I assume that will be cleaned up before committing. There are some extraneous whitespace changes in the source. I tried to apply the patch to 54 and trunk via "patch < bug.diff" (which usually works fine) and all hunks failled. Not sure what I'm doing wrong. Thanks, --Dan -- T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y data intensive web and database programming http://www.AnalysisAndSolutions.com/ 4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 f: 718-854-0409 --- ## #56780 — Daniel Convissor — 2011-12-05T17:30:59+00:00 URL: https://externals.io/message/56734#56780 · In reply to: #56779 Hi Alan: > > I tried to apply the patch to 54 and trunk via "patch < bug.diff" > (which usually works fine) and all hunks failled. Not sure what I'm > doing wrong. Ah, I needed to do "patch -p0 < bug.diff" But now that I applied it to 54, running make chokes... [ snip ] /bin/bash /php/php/php-src/branches/PHP_5_4/libtool --silent --preserve-dup-deps --mode=compile cc -Isapi/cli/ -I/php/php/php-src/branches/PHP_5_4/sapi/cli/ -DPHP_ATOM_INC -I/php/php/php-src/branches/PHP_5_4/include -I/php/php/php-src/branches/PHP_5_4/main -I/php/php/php-src/branches/PHP_5_4 -I/php/php/php-src/branches/PHP_5_4/ext/date/lib -I/php/php/php-src/branches/PHP_5_4/ext/ereg/regex -I/usr/include/libxml2 -I/php/php/php-src/branches/PHP_5_4/ext/mbstring/oniguruma -I/php/php/php-src/branches/PHP_5_4/ext/mbstring/libmbfl -I/php/php/php-src/branches/PHP_5_4/ext/mbstring/libmbfl/mbfl -I/usr/include/postgresql -I/php/php/php-src/branches/PHP_5_4/ext/sqlite3/libsqlite -I/php/php/php-src/branches/PHP_5_4/TSRM -I/php/php/php-src/branches/PHP_5_4/Zend -I/usr/include -g -O2 -fvisibility=hidden -DZEND_SIGNALS -c /php/php/php-src/branches/PHP_5_4/sapi/cli/php_cli.c -o sapi/cli/php_cli.lo make: *** [Zend/zend_execute.lo] Error 1 make: *** Waiting for unfinished jobs.... That's after running make clean and vcsclean. And before that having built 54 successfully with the exact same configure options. Thanks, --Dan -- T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y data intensive web and database programming http://www.AnalysisAndSolutions.com/ 4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 f: 718-854-0409 --- ## #56942 — Stas Malyshev — 2011-12-17T23:54:56+00:00 URL: https://externals.io/message/56734#56942 · In reply to: #56774 Hi! I think the idea behind this patch is good. I'll do final checks and apply it tomorrow if no objections heard until then. Some notes: > $s = "string"; isset($s['offset']) returns false > This is pretty critical, as it's the only way to detect this situation, > and ensure that array tests do not return positive results for strings. > It also catches an obvious, but previously hidden and probably serious > bugs in the PHP code.. This change concerns me a bit since it's a behavior change and provided that most of the people aren't actually aware that $s['offset'] means $s[0] it may lead to subtle code breakage. Then again, doing $s['offset'] leads to code breakage right now - I just encountered another bug caused by this in production code mere days ago, code was checking that $s['offset'] is set without actually checking that $s is an array, and it happened to be not... > $s = "string"; $s[' 2 ']; and $s['2foo'] both emit errors, and return > false from isset() - the code pretty much uses `is_numeric()` internally. > - I'm pretty sure these would be un-intentional bugs, so behaving as > such seems consistent. Agreed, I think is_numeric is the right way. > I do not mind either way if $s['offset'] returns the first char or an > empty string, however it seemed a little inconstant to return false on Here I am conflicted. The right thing would be to return empty but it may cause some breakage. Then again, doing what we do now causes breakage right now too (not new in 5.4, same breakage happening in 5.3 too) so I'm inclined to go with false here unless somebody gives me a use case where it's useful for anything. > isset(), yet actually return a string. (although technically an empty > string does kind of indicate it is 'isset') - it at least leaves the > engine in a consistent state. Perhaps that change can wait's until 5.5.. > thoughts... > > As for dropping the syntax for Strings eventually.. It would be nice, > but I'm not sure it is feasible anymore unfortunately. We tried to move to {} syntax but that didn't work, and probably isn't possible anymore without massive breakage. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #56943 — Laruence — 2011-12-18T03:31:59+00:00 URL: https://externals.io/message/56734#56943 · In reply to: #56942 > Hi! > > I think the idea behind this patch is good. I'll do final checks and apply > it tomorrow if no objections heard until then. > > Some notes: > > > > […] > > > This change concerns me a bit since it's a behavior change and provided that > most of the people aren't actually aware that $s['offset'] means $s[0] it > may lead to subtle code breakage. Then again, doing $s['offset'] leads to > code breakage right now - I just encountered another bug caused by this in > production code mere days ago, code was checking that $s['offset'] is set > without actually checking that $s is an array, and it happened to be not... > > > > […] > > > Agreed, I think is_numeric is the right way. > > > > […] > > > Here I am conflicted. The right thing would be to return empty but it may > cause some breakage. Then again, doing what we do now causes breakage right > now too (not new in 5.4, same breakage happening in 5.3 too) so I'm inclined > to go with false here unless somebody gives me a use case where it's useful > for anything. > > > > […] > > > We tried to move to {} syntax but that didn't work, and probably isn't > possible anymore without massive breakage. > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > Hi: I think only trigger notice when a convertion of string to number index is a good way (trivial bc break). :) thanks > > -- > > > -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #56944 — Stas Malyshev — 2011-12-18T03:36:52+00:00 URL: https://externals.io/message/56734#56944 · In reply to: #56943 Hi! > I think only trigger notice when a convertion of string to number > index is a good way (trivial bc break). This however doesn't solve the problem with isset() (which still produces true then). BTW, notices there may lead to interruption problems (imagine some error handler that may change variables that being involved in the expression). -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227