Re: com php-src: fix leak in 5.6: ext/session/mod_files.c

Hi Anatol, On Sat, Jan 30, 2016 at 7:15 PM, Anatol Belski <ab@php.net> wrote: >> -----Original Message----- >> From: yohgaki@gmail.com [mailto:yohgaki@gmail.com] On Behalf Of Yasuo >> Ohgaki >> Sent: Saturday, January 30, 2016 4:13 AM >> To: Anatol Belski <ab@php.net> >> Cc: php-cvs@lists.php.net >> Subject: Re: [PHP-CVS] com php-src: fix leak in 5.6: ext/session/mod_files.c >> >> On Sat, Jan 30, 2016 at 11:03 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: >> > I'm just curious which case causes memory leak. >> > php_rshutdown_globals() is always called at shutdown and s_close() >> > should clean them up. >> > >> > http://lxr.php.net/xref/PHP_5_6/ext/session/session.c#104 >> > >> > Thank you. >> >> s/php_rshutdown_globals()/php_rshutdown_session_globals()/ >> > Thanks for the check. The actual reproduce case in #69111 was causing crash in 5.6 only, so I > made just a simple NULL check. So far it was fine on Debian stable and old stable, but then travis > showed that there was a memory leak on the session data. Furthermore, despite the code base in 7.0 > is somewhat different, the memory leak is present as well. The basic situation is the same - when > the session id is invalid, it refuses the read/write operation and seems that at some place the > proper freeing doesn't happen. > > As valgrind on my dev environment was not showing any issue, I attempted to analyze the code > flow, then commit and see what travis tells. But as I don't reproduce this leak locally, > it's impossible to properly debug the case. So finally I've reverted my attempts to fix > the memory leak and set the test to XFAIL until me or someone else can reproduce and debug it > locally :( I'll probably need to setup some more recent OS version for that, which I have no > time to. If it's reproducable for someone, please go ahead. Thank you for the explanation. I'll look into this. Session module should manage state precisely and it makes things complicated. I guess there is/are missing state management in it. Current code is not explicit about state management. I may try to clean them up and rewrite it for PHP8. In the meantime, I'll add many asserts and comments for better code readability and future rewrite. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net