Re: [PATCH] More array filling optimizations

Hi Dmitry, all, I looked into and fixed the -1 size and memory overflow error a couple weeks or so ago, and finally updated the patches now. :-) I made the mistake of assuming the code that checks offset/length (in array_splice, etc.) set length correctly, but it can still be negative, which broke array_init_size... Should be OK now there, and also in array_slice and array_chunk. So if it works correctly now, it sounded like the patch could be used? (This doesn't have the compile/VM part, of course.) I would be able to commit these things myself now, but wouldn't unless I was given the OK first! :-) http://realplain.com/php/array_init_size.diff http://realplain.com/php/array_init_size_5_3.diff Thanks, Matt ----- Original Message ----- From: "Dmitry Stogov" Sent: Wednesday, April 30, 2008 > > Matt Wilmas wrote: > > Hi again Dmitry, > > > > Hmm, if a -1 size was passed, I thought it would just make a huge array size > > (bad, obviously), but the tests would still work. (Or I guess it might > > exhaust memory_limit.) Anyway, the code for checking the size (length > > variable) in array_splice() is copied from php_splice(), so maybe there's > > some other error if it's becoming -1... > > The bug causes memory overflow error, but the size definitely shouldn't > be -1, as the same tests work fine before the patch. > > > You don't think it makes sense to check return_value_used like > > next/prev/end/reset do? They would only create a 1 element array, but > > array_splice() could waste time filling a large array that isn't used. I > > think the function is used a lot to only remove/replace elements, not using > > the return_value: > > http://www.google.com/codesearch?q=lang%3Aphp+array_splice > > Ah. I see. My mistake. Of course it makes sense. > > Thanks. Dmitry. > > > If there's a -1 size bug, the array_splice changes can be reverted and > > simply have: > > > > if (return_value_used) { > > /* Initialize return value */ > > array_init(return_value); > > rem_hash = &Z_ARRVAL_P(return_value); > > } > > > > - Matt > > > > > > ----- Original Message ----- > > From: "Dmitry Stogov" > > Sent: Wednesday, April 30, 2008 > > > >> For some reason "make test" with the patch reported several broken > >> array_splice() tests. Looking in gdb I saw that init_array() got -1 as a > >> size of new array. > >> > >> I don't think checks for return_value_used for array_splice() have a lot > >> of sense. > >> > >> Thanks. Dmitry.