On Wed, Aug 27, 2014 at 8:38 AM, Anatol Belski <a...@php.net> wrote: > On Wed, August 27, 2014 01:32, Nikita Popov wrote: > > On Mon, Aug 18, 2014 at 3:39 PM, Anatol Belski <a...@php.net> wrote: > > > > > >> Commit: 97e9d058f09c12161863e5c3832552eb5da3f3c6 > >> Author: Anatol Belski <a...@php.net> Mon, 18 Aug 2014 15:39:38 > >> +0200 > >> Parents: 22dbb38d5e234f5987e226298ba9b86d9f9ea52a > >> Branches: master > >> > >> > >> Link: > >> > http://git.php.net/?p=php-src.git;a=commitdiff;h=97e9d058f09c12161863e5c > >> 3832552eb5da3f3c6 > >> > >> > >> Log: > >> fixes to string functions > >> > >> Changed paths: > >> M ext/standard/php_string.h > >> M ext/standard/string.c > >> > >> > > > > The changes here related to using STR_INIT for a zpp default value won't > > work correctly (also including followup changes). The way it is currently > > implemented you will still get leaks (e.g. if the str_pad default *is* > > used) or segfaults (if zpp fails after assigning the pad value). > > > > I would suggest reverting the STR_INIT related changes altogether, or if > > you want to use this approach, then the zend_string for the default value > > should be allocated on startup and destroyed on shutdown. > > > yeah, that's why I made the follow ups like this > > > http://git.php.net/?p=php-src.git;a=commitdiff;h=6bea54b7b9d5fff11bf40ab0464800baa9fbada3 > > http://git.php.net/?p=php-src.git;a=commitdiff;h=98bb620bfe132fbc3ee6c4e630e859ab6dc8ba78 > > Those are failsafe, however i'm not sure it'll work always with interned > strings and anyway it's overkill for the default value. >
Yes, I am already referring to the versions with the follow up fixes. These still leak - you likely don't see the leak because you asked for a permanent string (the last 1 in STR_INIT), so they are not managed by ZMM. The segfault I describe also occurs as an assertion failure in some of our tests. The necessary fixes would be: 1. Make it a non-permanent string 2. Always use _save to free 3. Add a if (pad_str == pad_str_safe) zend_str_release(pad_str_safe) at all exit paths. I think this is very complicated and additional has bad performance, because now we always allocate the default pad string, where previously it was stack allocated. Of course, that can be reverted back to 's', in most cases that will > suffice (like for pad_str in str_pad()). Probably can be done if one can't > initialize zend_string to a default value for ZPP. But actually, to not to > allocate, can we initialize zend_string with an interned string? As often > something like "", "\n" (so a simple char) was used for that. > That was my second suggestion. Allocate the necessary strings in the RINIT handler for the standard/string module and the release them in RSHUTDOWN. The " " and "\n" strings are available as interned strings via the single char table, but that's not always the case - not in zts and I think also not when opcache is disabled. So we can't rely on this, have to handle it separately. Nikita
