Re: Re: another fix for max_input_vars.

On Thu, Jan 5, 2012 at 12:50 PM, Rasmus Lerdorf <rasmus@lerdorf.com> wrote: > On 01/04/2012 08:13 PM, Laruence wrote: >> On Thu, Jan 5, 2012 at 5:56 AM, Rasmus Lerdorf <rasmus@lerdorf.com> wrote: >>> On 01/04/2012 01:48 PM, Rasmus Lerdorf wrote: >>>> On 01/04/2012 01:27 PM, Stas Malyshev wrote: >>>>> Hi! >>>>> >>>>>> Right, like I said in my previous message, if this is caught by >>>>>> display_start_errors, I am ok with it. We need the default/no php.ini >>>>>> file case to not leak information like this. >>>>> >>>>> Just checked - it does not display error if display_startup_errors if >>>>> off, does display if it's on. >>>> >>>> Right, that seems ok. The other thing is that we need to clarify that it >>>> actually only limits the number of variables per nesting level. The >>>> current name and the description doesn't make that clear. You can still >>>> send 1M post vars in a single POST if you just nest them in a 1000x1000 >>>> 2d array. Of course, this is likely going to hit the post_max_size >>>> limit, although many sites that do file uploads will have cranked that >>>> way up. >>> >>> Oh, and a final issue to address. >>> >>> This code: >>> >>> for($data=[],$i=0; $i<=999; $i++) $data[$i] = range(0,1001); >>> echo >>> curl_post("http://localhost/index.php",['a'=>$data]); >>> >>> will spew the warning 2000 times. >>> >>> & php post.php | grep Warning | wc -l >>> 2000 >>> >> could you try with this new patch: >> >> https://bugs.php.net/patch-display.php?bug_id=60655&patch=max_input_vars.patch&revision=latest >> ? >> >> this patch also restrict the json / serializer ,  all of them must >> less than PG(max_input_vars). >> >> and different with the fix which was commited now,  this patch count >> the num vars in a global scope, that means if there are 2 elements >> which both have 500 elements in post,  the restriction will also >> affect, >> >> and this patch will not affect the existsing called to json or >> serializer,   only affect the zif_json_decode and zif_serialize, >> after patch, the serialize will have a sencode parameter :"$max_vars". >> >> and the restriction can also be closed by set max_input_vars to 0. > > Right, I don't think this is going to work. A simple 'make install' > after applying your patch fails with: > > Warning: unserialize(): Unserialized variables exceeded 1000 in > phar:///home/rasmus/php-src/branches/PHP_5_4/pear/install-pear-nozlib.phar/PEAR/Registry.php > on line 1145 > > Warning: unserialize(): Unserialized variables exceeded 1000 in > phar:///home/rasmus/php-src/branches/PHP_5_4/pear/install-pear-nozlib.phar/PEAR/Registry.php > on line 1145 > > Warning: unserialize(): Unserialized variables exceeded 1000 in > phar:///home/rasmus/php-src/branches/PHP_5_4/pear/install-pear-nozlib.phar/PEAR/Registry.php > on line 1145 > [PEAR] PEAR: pear.php.net/PEAR not installed > > I really don't think this manual per-feature limiting is going to cut it > here. The real problem is the predictability of the hashing which we can > address by seeding it with a random value. That part is easy enough, the Hi: that will be a better fix, we have disscussed this before in irc, I also can have a try in that way. and for this fix(in such way), to be honest, yes, this is a little ugly fix, but quick. the default 1000 is a little insufficient when counting the elements in a global scope. IMO it should set to be 4096, then I think 99% of developers will not see this warning at all. thanks > hard part is figuring out where people may be storing these hashes > externally and providing some way of associating the seed with these > external caches so they will still work. > > -Rasmus -- Laruence  Xinchen Hui http://www.laruence.com/