Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:56774
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain akbkhome.com designates 202.81.246.113 as permitted sender)
Message-ID: <4EDC015C.3080701@akbkhome.com>
Date: Mon, 05 Dec 2011 07:25:16 +0800
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20110922 Thunderbird/3.1.15
MIME-Version: 1.0
To: internals@lists.php.net
References: <4EDA4989.2010702@akbkhome.com> <CAH-PCH76g4LwEwzaEvU3Hp7PeeyMUN4sc0uZExkedhHvYYnm9A@mail.gmail.com> <CABBJUpfNRaa580ZCtsYHCbDat1Sxbo7tNz2MiNQ68foPFTq61g@mail.gmail.com> <CABBJUpe2tWo68hpPrfovhDtFYVczU3-XhDD8ynh0i5E=vUnuFg@mail.gmail.com>
In-Reply-To: <CABBJUpe2tWo68hpPrfovhDtFYVczU3-XhDD8ynh0i5E=vUnuFg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] Fixing string offsets of strings.
From: alan@akbkhome.com (Alan Knowles)

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




On Monday, December 05, 2011 01:28 AM, Laruence wrote:
> Hi:
>      I have submit a new patch based on the origin patch,  which only
> trigger notice when string offset cast occurred.
>
> thanks
>
> On Sun, Dec 4, 2011 at 10:25 PM, 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
>>>>
>>> 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/
>
>