Thanks for your reply :)
Sascha Schumann wrote:
> Hi,
>
> looks like you have put a lot of work into this one :-)
>
> Thanks for keeping up the work on the session module.
>
>
>> - Crashes are caused by invlaid save_path, invalid
>> session id name, return value from user defined session function.
>> There may be other crashes observed that I don't know.
>>
>
> It is not apparent from either your description or the patch
> which issue caused the above behaviour. Please elaborate.
Please read further.
> If you are just referring to the unconditional freeing of val
> as returned from the read handler, I disagree that it should be
> conditional. A handler should return an empty string in that
> case. Segfaulting is ok, because that will alert the
> author of the module to fix his code and thus will improve
> code quality.
Agreed.
I conditinally freed to get rid of malloc(strdup) for mm save handler.
I'll change it to return empty string.
ps_sd_lookup() returns NULL for 1st(New session) request at least with
my environment (Linux 2.4.17/glibc-2.2.4/mm-1.1.3). Due to this, mm
save handler does not work for me.
>
>
>> - Broken mm save handler.
>>
>
> Your patch to mod_mm.c always returns SUCCESS, even if the
> operation fails. That's pointless, the caller function
> checks for that return value. I also disagree with not
> stopping script execution, if the session module startup
> fails. Every script which is based upon sessions completely
> depends on them. Execution shall halt, if such a fatal error
> is encountered.
If it returns FALSE, mm save handler does not work for me.
session_pgsql does not work if returns false.
msession never returns false also.
= reproduce procedure =
1) In php.ini, set save_hander=files
2) In script, set save handler module with session_module_name()
(Set to mm - user, sessoin_pgsql and msession works)
3) In script, start sesson with session_start()
4) From browser, reload and see if session variales are saved.
(Vars are not saved with my Linux)
Not only it just does not work, but returning false causes
verious segfaults also. I observed this problem for all save
handler files, mm, mod_user and session_pgsql.
For files, invalid tmp dir, invalid session name, etc make
read return false and it caused segfaults.
Perhaps, I should change session.c to accepts false.
I don't remeber why I didn't change that way.
I'll write again for this. I don't have much time now.
>
>
>> - Sticky session module name
>>
>
> Please elaborate.. AFAICT, the call to alter_ini is the same.
We should prevent useless ini changes. I'll create appropriate MH for
session ini changes later.
Sorry for improper description. "Sticky session module name" should be
"Sticky session save handler module"
= reproduce procedure =
1) In php.ini, set save_hander=files
2) In script, set save handler module with session_module_name()
other than specified 1)
3) In script, start sesson with session_start()
4) Then start session without session_module_name(). Session module
should use handler specified by 1), but it uses handler specified
2)
Since ps_global.mod is not reinitilized for each request, new
module will be used once session_module_name() is used.
This problem may create nasty bug in PHP scripts.
>
>
>> - Change error level for failure to open session.
>> (E_ERROR -> E_WARNING)
>>
>
> See above.
>
>
>> - Dead lock with files handler
>>
>
> Not needed.. the code checks for data->fd < 0 which is a more
> subtle way for explicit error reporting.
I didn't test this one, but I think my patch will fix files deadlock.
Session module seems to reintilize session when session_module_name()
is used without this patch for some reason.
After patch is applied, I'll ask users who reported this
bug to confirm.
>
>
>> - Prevent calling session_name() and session_set_cookie_parameter()
>> once session is started. It doesn't work after session is started,
>> anyways.
>> - Prevent to set module name after session is started. (This causes
>> dead lock with files hadler. It may do something bad for other save
>> handlers also. It should be prevented, IMO.)
>>
>
> Good. There is a bogus error message in the cache_limiter
> code.
I would like to add more checks for other functions :)
I'll submit patch when I finish this one.
--
Yasuo Ohgaki
--
PHP Development Mailing List <http://www.php.net/>
To unsubscribe, visit: http://www.php.net/unsub.php
