Re: Fixing string offsets of strings.

On Mon, Dec 5, 2011 at 12:39 AM, Etienne Kneuss <colder@php.net> wrote: > Hi, > > On Sun, Dec 4, 2011 at 15:25, Laruence <laruence@php.net> wrote: >> >> +1. >> >> thanks. >> >> On Sun, Dec 4, 2011 at 10:05 PM, Ferenc Kovacs <tyra3l@gmail.com> wrote: >> > On Sat, Dec 3, 2011 at 5:08 PM, Alan Knowles <alan@akbkhome.com> wrote: >> > >> >> 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<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 > > > 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, > >> >> >> >> > >> > 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/ >> >> -- >> PHP Internals - PHP Runtime Development Mailing List >> To unsubscribe, visit: http://www.php.net/unsub.php >> > > > > -- > Etienne Kneuss > http://www.colder.ch -- Laruence  Xinchen Hui http://www.laruence.com/