Re: [PHP-DEV] REVIEW: Imminent session patch

[Sascha Schumann]

> Are you sure it was only 4.2?  I seem to remember seeing similar code in
> 4.1.

Possibly.  We changed the serialization strategy which is
appealing because of its simplicity, but it is not backwards
compatible.

I'll commit my changes so far (the patch I posted earlier),
tag the session extension and then will try to revert the
serialization strategy and everything associated with it.
The logic needs to be tweaked a bit to make
unset($_SESSION[..]) work.

- Sascha


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Rasmus Lerdorf]

> > I am assuming this documentation was not written recently since it talks
> > about track_vars and there is no way to turn off track_vars today.  And
>
> You are right -- it was not written recently.  The semantics
> of the session module have not changed since then though.  If
> a bug temporarily made something possible which should not
> have been, well, that is bad luck.
>
> I'm still considering bug compatibility with 4.2, if readding
> that behaviour is not too difficult or introduces other bugs.

Are you sure it was only 4.2?  I seem to remember seeing similar code in
4.1.

My main concern here is breaking BC.  Code that worked in 4.2 and likely
earlier versions as well will now break.  If we do decide to break this
code, and that may be the right decision, we need to be very clear about
what we are breaking and shout it from the rooftops in the release
announcement.

-Rasmus


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Sascha Schumann]

> I am assuming this documentation was not written recently since it talks
> about track_vars and there is no way to turn off track_vars today.  And

You are right -- it was not written recently.  The semantics
of the session module have not changed since then though.  If
a bug temporarily made something possible which should not
have been, well, that is bad luck.

I'm still considering bug compatibility with 4.2, if readding
that behaviour is not too difficult or introduces other bugs.

> since it isn't new and the code up until now did not enforce the
> restriction of only using track vars, then I don't see how you can say
> that this documentation was ever an accurate reflection of what the code
> actually did.

This bug/feature we are talking about does not work since
this commit and has been killed further by successive commits.

revision 1.81
date: 2002/08/15 21:44:44;  author: zeev;  state: Exp; lines: +19 -9
Make unset($_SESSION['foo']) actually remove the variable from the session,
if register_globals is off.

Now, please choose which bug you prefer -- from my point of
view, a symmetric way of accessing session variables is the
way to go.  The documentation has always stated that for
register_globals=off you would need to use $HTTP_SESSION_VARS
(or $_SESSION), respectively.

- Sascha


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Rasmus Lerdorf]

On Wed, 2 Oct 2002, Sascha Schumann wrote:

> On Wed, 2 Oct 2002, Rasmus Lerdorf wrote:
>
> > Hrm, but that documentation is very out of date and didn't match the code
> > even when it was written.
>
> Huh, excuse me, would you please leave that judgement to the
> authors of the code and the documentation (Andrei and me).
>
> The documentation is correct in stating that only track_vars
> ($_SESSION is a track var) are used, if track_vars is on (it
> is by default on today) and if register_globals is off.

I am assuming this documentation was not written recently since it talks
about track_vars and there is no way to turn off track_vars today.  And
since it isn't new and the code up until now did not enforce the
restriction of only using track vars, then I don't see how you can say
that this documentation was ever an accurate reflection of what the code
actually did.

-Rasmus


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Sascha Schumann]

On Wed, 2 Oct 2002, Rasmus Lerdorf wrote:

> Hrm, but that documentation is very out of date and didn't match the code
> even when it was written.

Huh, excuse me, would you please leave that judgement to the
authors of the code and the documentation (Andrei and me).

The documentation is correct in stating that only track_vars
($_SESSION is a track var) are used, if track_vars is on (it
is by default on today) and if register_globals is off.

- Sascha


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Sascha Schumann]

On Wed, 2 Oct 2002, Rasmus Lerdorf wrote:

> > That example only works when register_globals are on. When they are off, it
>
> Up until 4.3 it worked just fine.

Yes, there were a series of modification with regard to
$_SESSION which -- modified some of the behaviour.

get_session_var falls back to EG(symbol_table) in 4.2, if a
slot has not been found in $_SESSION.  So, your example works
in 4.2 although it really should not.

- Sascha


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Rasmus Lerdorf]

Hrm, but that documentation is very out of date and didn't match the code
even when it was written.

On Wed, 2 Oct 2002, Sascha Schumann wrote:

> > But that is a bit of a twist of what "register_globals" is supposed to
> > mean.  As far as I am concerned register_globals only affects how data is
> > imported into PHP.  Having that flag trigger other behaviours is
> > completely undocumented and outside the scope of the original intent of
> > register_globals.  You are effectively overloading register_globals.
>
> Nope, it is not.
>
>  If track_vars is enabled and register_globals  is disabled,
>  only members of the global associative array
>  $HTTP_SESSION_VARS can be registered as session variables.
>  The restored session variables will only be available in the
>  array $HTTP_SESSION_VARS.
>
> http://de.php.net/manual/en/ref.session.php
>
> - Sascha
>


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Sascha Schumann]

> But that is a bit of a twist of what "register_globals" is supposed to
> mean.  As far as I am concerned register_globals only affects how data is
> imported into PHP.  Having that flag trigger other behaviours is
> completely undocumented and outside the scope of the original intent of
> register_globals.  You are effectively overloading register_globals.

Nope, it is not.

 If track_vars is enabled and register_globals  is disabled,
 only members of the global associative array
 $HTTP_SESSION_VARS can be registered as session variables.
 The restored session variables will only be available in the
 array $HTTP_SESSION_VARS.

http://de.php.net/manual/en/ref.session.php

- Sascha


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Andrei Zmievski]

On Wed, 02 Oct 2002, Sascha Schumann wrote:
> Uh, how ugly.  This has never been supported intentionally.
> Looks like a result of multiple people modifying code and not
> talking to each other.
> 
> Note that the docs for session_register are out of date.  It
> is supposed to mean "register a slot in the session domain".
> The global variable scope is only included in that domain, if
> register_globals is enabled.

Makes sense to me.

-Andrei   http://www.gravitonic.com/

Documentation is worth it just to be able
to answer all your mail with 'RTFM'. -- Alan Cox

-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Rasmus Lerdorf]

> That example only works when register_globals are on. When they are off, it

Up until 4.3 it worked just fine.

-Rasmus


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Rasmus Lerdorf]

> On Wed, 2 Oct 2002, Rasmus Lerdorf wrote:
>
> > I really don't see why would want to deprecate session_register().
> > Regardless of whether register_globals is on or off, code like this works
> > fine:
> >
> > Set a session var:
> >   session_register('a');
> >   $a=1;
> >
> > Get a session var:
> >   session_start();
> >   $a = $_SESSION['a'];
>
> Uh, how ugly.  This has never been supported intentionally.
> Looks like a result of multiple people modifying code and not
> talking to each other.
>
> Note that the docs for session_register are out of date.  It
> is supposed to mean "register a slot in the session domain".
> The global variable scope is only included in that domain, if
> register_globals is enabled.

But that is a bit of a twist of what "register_globals" is supposed to
mean.  As far as I am concerned register_globals only affects how data is
imported into PHP.  Having that flag trigger other behaviours is
completely undocumented and outside the scope of the original intent of
register_globals.  You are effectively overloading register_globals.

-Rasmus


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Sascha Schumann]

On Wed, 2 Oct 2002, Rasmus Lerdorf wrote:

> I really don't see why would want to deprecate session_register().
> Regardless of whether register_globals is on or off, code like this works
> fine:
>
> Set a session var:
>   session_register('a');
>   $a=1;
>
> Get a session var:
>   session_start();
>   $a = $_SESSION['a'];

Uh, how ugly.  This has never been supported intentionally.
Looks like a result of multiple people modifying code and not
talking to each other.

Note that the docs for session_register are out of date.  It
is supposed to mean "register a slot in the session domain".
The global variable scope is only included in that domain, if
register_globals is enabled.

> This makes perfect sense to me and I see no reason to change that way of
> writing session code.

I've verified that this works in the 4.2 tree.  I'll update
the patch to include a change to get_session_var.  Sigh.

- Sascha


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Ilia A.]

On October 2, 2002 04:26 pm, Rasmus Lerdorf wrote:
> I really don't see why would want to deprecate session_register().
> Regardless of whether register_globals is on or off, code like this works
> fine:
>
> Set a session var:
>   session_register('a');
>   $a=1;

That example only works when register_globals are on. When they are off, it 
produces:
var_dump($a) -> int(1)
var_dump($_SESSION['a']) -> NULL

If register globals are enabled, then this example results in:
var_dump($a) -> int(1)
var_dump($_SESSION['a']) -> int(1)

When register_globals are off register_session('a') merely creates a NULL 
entry 'a' entry inside http_session_vars. Since that is the case, would it 
not be simpler to use:

$_SESSION['a'] = &$a;
$a=1;

or

$a = 1;
$_SESSION['a'] = $a;

which work fine regardless of whether register_globals are on or off.

Ilia

> Get a session var:
>   session_start();
>   $a = $_SESSION['a'];
>
> This makes perfect sense to me and I see no reason to change that way of
> writing session code.
>
> -Rasmus
>
> On Wed, 2 Oct 2002, Ilia A. wrote:
> > On October 2, 2002 03:46 pm, Sascha Schumann wrote:
> > > > Its very simply really, as you well know, since PHP 4.2.0
> > > > register_globals are off by default. Because they are off,
> > > > session_register does not retrieve a value from the variable and only
> > > > creates a null variable inside the session. So, unless the user is
> > > > aware of this issue and knows that to fix the problem they need to
> > > > enable register globals, which somewhat of a security risk, the
> > > > application they are trying to use won't work. Session is a fairly
> > > > popular extension, it is used by many PHP applications, so this is
> > > > rather serious problem.
> > >
> > > You fail to see that an application is either designed to
> > > work with enabled register_globals, or it is not.  There is
> > > little in between.  If an application's session part fails
> > > because of register_globals, the rest of the application
> > > which handles input data will also fail.
> > >
> > > No magic code in the session extension will cure the support
> > > hassle created by the register_globals change.
> > >
> > > The main goal is to have a stable session extension with
> > > little to no behavioral changes from 4.2 to 4.3.  Please keep
> > > that in mind.
> >
> > In this case, would it not be prudent to either depreciate the
> > session_register() function or add an E_NOTICE message in the event it is
> > used on a system with session_register() disabled?
> > That way we can atleast provide the users some idea as to why their code
> > is not working and perphaps make the 1st step towards eliminating
> > session_register() completely at some point in the future.
> >
> > > > My patch does not force the association between track and global
> > > > variables all the time, it is conditional on register_globals being
> > > > enabled. This does not prevent the user from having $test0 &
> > > > $_SESSION['test0'] as 2 seperate varaibles containing unrelated data.
> > >
> > > That's incorrect.  Consider this code with your patch applied
> > > and register_globals=0:
> > >
> > > $c = "bla";
> > > session_register("c");
> > > // $_SESSION["c"] is now a ref to $c
> > > $_SESSION["c"] = "other stuff";
> > > // voila, you now have changed the global variable $c
> >
> > That is quite correct. Of course it can be avoided by some additional
> > hacking to the code. However, based on your defenition of the goals for
> > the session extension, I believe it would be quite useless.
> >
> > Ilia
> >
> > --
> > PHP Development Mailing List 
> > To unsubscribe, visit: http://www.php.net/unsub.php


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Rasmus Lerdorf]

I really don't see why would want to deprecate session_register().
Regardless of whether register_globals is on or off, code like this works
fine:

Set a session var:
  session_register('a');
  $a=1;

Get a session var:
  session_start();
  $a = $_SESSION['a'];

This makes perfect sense to me and I see no reason to change that way of
writing session code.

-Rasmus


On Wed, 2 Oct 2002, Ilia A. wrote:

> On October 2, 2002 03:46 pm, Sascha Schumann wrote:
> > > Its very simply really, as you well know, since PHP 4.2.0
> > > register_globals are off by default. Because they are off,
> > > session_register does not retrieve a value from the variable and only
> > > creates a null variable inside the session. So, unless the user is aware
> > > of this issue and knows that to fix the problem they need to enable
> > > register globals, which somewhat of a security risk, the application they
> > > are trying to use won't work. Session is a fairly popular extension, it
> > > is used by many PHP applications, so this is rather serious problem.
> >
> > You fail to see that an application is either designed to
> > work with enabled register_globals, or it is not.  There is
> > little in between.  If an application's session part fails
> > because of register_globals, the rest of the application
> > which handles input data will also fail.
> >
> > No magic code in the session extension will cure the support
> > hassle created by the register_globals change.
> >
> > The main goal is to have a stable session extension with
> > little to no behavioral changes from 4.2 to 4.3.  Please keep
> > that in mind.
>
> In this case, would it not be prudent to either depreciate the
> session_register() function or add an E_NOTICE message in the event it is
> used on a system with session_register() disabled?
> That way we can atleast provide the users some idea as to why their code is
> not working and perphaps make the 1st step towards eliminating
> session_register() completely at some point in the future.
>
> > > My patch does not force the association between track and global
> > > variables all the time, it is conditional on register_globals being
> > > enabled. This does not prevent the user from having $test0 &
> > > $_SESSION['test0'] as 2 seperate varaibles containing unrelated data.
> >
> > That's incorrect.  Consider this code with your patch applied
> > and register_globals=0:
> >
> > $c = "bla";
> > session_register("c");
> > // $_SESSION["c"] is now a ref to $c
> > $_SESSION["c"] = "other stuff";
> > // voila, you now have changed the global variable $c
>
> That is quite correct. Of course it can be avoided by some additional hacking
> to the code. However, based on your defenition of the goals for the session
> extension, I believe it would be quite useless.
>
> Ilia
>
> --
> PHP Development Mailing List 
> To unsubscribe, visit: http://www.php.net/unsub.php
>


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Ilia A.]

On October 2, 2002 03:46 pm, Sascha Schumann wrote:
> > Its very simply really, as you well know, since PHP 4.2.0
> > register_globals are off by default. Because they are off,
> > session_register does not retrieve a value from the variable and only
> > creates a null variable inside the session. So, unless the user is aware
> > of this issue and knows that to fix the problem they need to enable
> > register globals, which somewhat of a security risk, the application they
> > are trying to use won't work. Session is a fairly popular extension, it
> > is used by many PHP applications, so this is rather serious problem.
>
> You fail to see that an application is either designed to
> work with enabled register_globals, or it is not.  There is
> little in between.  If an application's session part fails
> because of register_globals, the rest of the application
> which handles input data will also fail.
>
> No magic code in the session extension will cure the support
> hassle created by the register_globals change.
>
> The main goal is to have a stable session extension with
> little to no behavioral changes from 4.2 to 4.3.  Please keep
> that in mind.

In this case, would it not be prudent to either depreciate the 
session_register() function or add an E_NOTICE message in the event it is 
used on a system with session_register() disabled?
That way we can atleast provide the users some idea as to why their code is 
not working and perphaps make the 1st step towards eliminating 
session_register() completely at some point in the future.

> > My patch does not force the association between track and global
> > variables all the time, it is conditional on register_globals being
> > enabled. This does not prevent the user from having $test0 &
> > $_SESSION['test0'] as 2 seperate varaibles containing unrelated data.
>
> That's incorrect.  Consider this code with your patch applied
> and register_globals=0:
>
> $c = "bla";
> session_register("c");
> // $_SESSION["c"] is now a ref to $c
> $_SESSION["c"] = "other stuff";
> // voila, you now have changed the global variable $c

That is quite correct. Of course it can be avoided by some additional hacking 
to the code. However, based on your defenition of the goals for the session 
extension, I believe it would be quite useless.

Ilia

-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Sascha Schumann]

> Its very simply really, as you well know, since PHP 4.2.0 register_globals are
> off by default. Because they are off, session_register does not retrieve a
> value from the variable and only creates a null variable inside the session.
> So, unless the user is aware of this issue and knows that to fix the problem
> they need to enable register globals, which somewhat of a security risk, the
> application they are trying to use won't work. Session is a fairly popular
> extension, it is used by many PHP applications, so this is rather serious
> problem.

You fail to see that an application is either designed to
work with enabled register_globals, or it is not.  There is
little in between.  If an application's session part fails
because of register_globals, the rest of the application
which handles input data will also fail.

No magic code in the session extension will cure the support
hassle created by the register_globals change.

The main goal is to have a stable session extension with
little to no behavioral changes from 4.2 to 4.3.  Please keep
that in mind.

> My patch does not force the association between track and global variables all
> the time, it is conditional on register_globals being enabled. This does not
> prevent the user from having $test0 & $_SESSION['test0'] as 2 seperate
> varaibles containing unrelated data.

That's incorrect.  Consider this code with your patch applied
and register_globals=0:

$c = "bla";
session_register("c");
// $_SESSION["c"] is now a ref to $c
$_SESSION["c"] = "other stuff";
// voila, you now have changed the global variable $c

- Sascha


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Ilia A.]

On October 2, 2002 02:31 pm, Sascha Schumann wrote:
> > believe should be addressed. The problem being that session_register()
> > function does not work unless the user has register_globals enabled.
>
> I think there is a misunderstanding with regard to how
> sessions interact with global variables.
>
> The session extension makes use of globals only, if
> register_globals is set.
>
> Your patch creates references between track and global
> variables, even if register_globals is not enabled.

> Prior to 4.3, the session module did not create any
> references upon session_register() in the register_globals=1
> case.  This however is necesssary to make get_session_var()
> reliably work and avoid any lookups in the global sym table.
>
> Because this behaviour is new, I don't see how existing
> applications could break.

Its very simply really, as you well know, since PHP 4.2.0 register_globals are 
off by default. Because they are off, session_register does not retrieve a 
value from the variable and only creates a null variable inside the session. 
So, unless the user is aware of this issue and knows that to fix the problem 
they need to enable register globals, which somewhat of a security risk, the 
application they are trying to use won't work. Session is a fairly popular 
extension, it is used by many PHP applications, so this is rather serious 
problem.
My patch does not force the association between track and global variables all 
the time, it is conditional on register_globals being enabled. This does not 
prevent the user from having $test0 & $_SESSION['test0'] as 2 seperate 
varaibles containing unrelated data. 

Ilia

-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Derick Rethans]

On Wed, 2 Oct 2002, Tit "Black" Petric wrote:

> correct me if i am wrong, but with $_SESSION you dont even need to have
> session_register() anymore, as for the behaviour of session_register(), if
> it held up to the latest php version, i dont see any sense of changing its
> functionality now, it's a less logical step than to encourage people to use
> $_SESSION.

There is nothing wrong with encouraging, but that does not mean you can 
break thousands of scripts because it's better to use $_SESSION :)

> 
> personally i think session_register() should have been removed after the
> creation of $_SESSION, but i see the logics in backwards compatibility, but
> for the future of php (php5.0.0) maybe some functions like this one should
> acctually be removed in favour of $_SESSION addressing.

Removing is not an option of course, or can we direct all usesr with 
questions to you? :)

Derick

--

---
 Derick Rethans   http://derickrethans.nl/ 
 JDI Media Solutions
--[ if you hold a unix shell to your ear, do you hear the c? ]-


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Tit \"Black\" Petric]

> (snip).. There is however another problem, which I
> believe should be addressed. The problem being that session_register()
> function does not work unless the user has register_globals enabled.
> This particular problem causes problems for anyone using php software that
was
> written before this limitation was added. In fact the sessions tests
inside
> ext/session/tests fail for that very reason.

correct me if i am wrong, but with $_SESSION you dont even need to have
session_register() anymore, as for the behaviour of session_register(), if
it held up to the latest php version, i dont see any sense of changing its
functionality now, it's a less logical step than to encourage people to use
$_SESSION.

personally i think session_register() should have been removed after the
creation of $_SESSION, but i see the logics in backwards compatibility, but
for the future of php (php5.0.0) maybe some functions like this one should
acctually be removed in favour of $_SESSION addressing.

regards,
black


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Sascha Schumann]

> believe should be addressed. The problem being that session_register()
> function does not work unless the user has register_globals enabled.

I think there is a misunderstanding with regard to how
sessions interact with global variables.

The session extension makes use of globals only, if
register_globals is set.

Your patch creates references between track and global
variables, even if register_globals is not enabled.

Prior to 4.3, the session module did not create any
references upon session_register() in the register_globals=1
case.  This however is necesssary to make get_session_var()
reliably work and avoid any lookups in the global sym table.

Because this behaviour is new, I don't see how existing
applications could break.

- Sascha


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] REVIEW: Imminent session patch

[Ilia A.]

On October 2, 2002 05:13 am, Sascha Schumann wrote:
> Could some kind soul with a thorough understanding of the
> engine details please review the attached patch?
>
> Changes:
>
> - Fix two simple memory leaks.
> - Fix a bug which did not initialize $_SESSION, if
>   is_array($HTTP_SESSION_VARS) was true.
>
> - Sascha

The patch does solve the problems it sets out to fix, most important of which 
in my opinion is the memory leak. There is however another problem, which I 
believe should be addressed. The problem being that session_register() 
function does not work unless the user has register_globals enabled.
This particular problem causes problems for anyone using php software that was 
written before this limitation was added. In fact the sessions tests inside 
ext/session/tests fail for that very reason.
To this affect, I have made a few modifications to the patch written by Sascha 
that address this problem. After the patch is applied, register_session() 
works regardless of whether or not register_globals are enabled or disabled.

Ilia

Index: session.c
===
RCS file: /repository/php4/ext/session/session.c,v
retrieving revision 1.323
diff -u -3 -p -u -r1.323 session.c
--- session.c	1 Oct 2002 11:59:45 -	1.323
+++ session.c	2 Oct 2002 14:48:27 -
@@ -241,6 +241,7 @@ typedef struct {
 void php_add_session_var(char *name, size_t namelen TSRMLS_DC)
 {
 	zval **sym_track = NULL;
+	zval **sym_global = NULL;
 	
 	zend_hash_find(Z_ARRVAL_P(PS(http_session_vars)), name, namelen + 1, 
 			(void *) &sym_track);
@@ -249,29 +250,25 @@ void php_add_session_var(char *name, siz
 	 * Set up a proper reference between $_SESSION["x"] and $x.
 	 */
 
-	if (PG(register_globals)) {
-		zval **sym_global = NULL;
-		
-		zend_hash_find(&EG(symbol_table), name, namelen + 1, 
-(void *) &sym_global);
+	zend_hash_find(&EG(symbol_table), name, namelen + 1, (void *) &sym_global);
 
-		if (sym_global == NULL && sym_track == NULL) {
-			zval *empty_var;
+	if (sym_global == NULL && sym_track == NULL) {
+		zval *empty_var;
 
-			ALLOC_INIT_ZVAL(empty_var);
-			zend_set_hash_symbol(empty_var, name, namelen, 1, 2, Z_ARRVAL_P(PS(http_session_vars)), &EG(symbol_table));
-		} else if (sym_global == NULL) {
-			zend_set_hash_symbol(*sym_track, name, namelen, 1, 1, &EG(symbol_table));
-		} else if (sym_track == NULL) {
-			zend_set_hash_symbol(*sym_global, name, namelen, 1, 1, Z_ARRVAL_P(PS(http_session_vars)));
-		}
-	} else {
-		if (sym_track == NULL) {
-			zval *empty_var;
-	
-			ALLOC_INIT_ZVAL(empty_var);
-			zend_set_hash_symbol(empty_var, name, namelen, 0, 1, Z_ARRVAL_P(PS(http_session_vars)));
-		}
+		ALLOC_INIT_ZVAL(empty_var); /* this sets refcount to 1 */
+		ZVAL_DELREF(empty_var); /* our module does not maintain a ref */
+		/* The next call will increase refcount by NR_OF_SYM_TABLES==2 */
+		if (PG(register_globals)) {
+			zend_set_hash_symbol(empty_var, name, namelen, 1, 2,
+Z_ARRVAL_P(PS(http_session_vars)), &EG(symbol_table));
+		} else {
+			zend_set_hash_symbol(empty_var, name, namelen, 1, 1,
+Z_ARRVAL_P(PS(http_session_vars)));
+		}	
+	} else if (sym_global == NULL) {
+		zend_set_hash_symbol(*sym_track, name, namelen, 1, 1, &EG(symbol_table));
+	} else if (sym_track == NULL) {
+		zend_set_hash_symbol(*sym_global, name, namelen, 1, 1, Z_ARRVAL_P(PS(http_session_vars)));
 	}
 }
 
@@ -465,23 +462,16 @@ break_outer_loop:
 
 static void php_session_track_init(TSRMLS_D)
 {
-	zval **old_vars = NULL;
+	/* Unconditionally destroy existing arrays -- possible dirty data */
+	zend_hash_del(&EG(symbol_table), "HTTP_SESSION_VARS", 
+			sizeof("HTTP_SESSION_VARS"));
+	zend_hash_del(&EG(symbol_table), "_SESSION", sizeof("_SESSION"));
 
-	if (zend_hash_find(&EG(symbol_table), "HTTP_SESSION_VARS", sizeof("HTTP_SESSION_VARS"), (void **)&old_vars) == SUCCESS && Z_TYPE_PP(old_vars) == IS_ARRAY) {
-		PS(http_session_vars) = *old_vars;
-		zend_hash_clean(Z_ARRVAL_P(PS(http_session_vars)));
-	} else {
-		if(old_vars) {
-			zend_hash_del(&EG(symbol_table), "HTTP_SESSION_VARS", sizeof("HTTP_SESSION_VARS"));
-			zend_hash_del(&EG(symbol_table), "_SESSION", sizeof("_SESSION"));
-		}
-		MAKE_STD_ZVAL(PS(http_session_vars));
-		array_init(PS(http_session_vars));
-		PS(http_session_vars)->refcount = 2;
-		PS(http_session_vars)->is_ref = 1;
-		zend_hash_update(&EG(symbol_table), "HTTP_SESSION_VARS", sizeof("HTTP_SESSION_VARS"), &PS(http_session_vars), sizeof(zval *), NULL);
-		zend_hash_update(&EG(symbol_table), "_SESSION", sizeof("_SESSION"), &PS(http_session_vars), sizeof(zval *), NULL);
-	}
+	MAKE_STD_ZVAL(PS(http_session_vars));
+	array_init(PS(http_session_vars));
+		
+	ZEND_SET_GLOBAL_VAR("HTTP_SESSION_VARS", PS(http_session_vars));
+	ZEND_SET_GLOBAL_VAR("_SESSION", PS(http_session_vars));
 }
 
 static char *php_session_encode(int *newlen TSRMLS_DC)



-- 
PHP Development Mailing List 
To unsubscribe, visit: http://w

[PHP-DEV] REVIEW: Imminent session patch

[Sascha Schumann]

Could some kind soul with a thorough understanding of the
engine details please review the attached patch?

Changes:

- Fix two simple memory leaks.
- Fix a bug which did not initialize $_SESSION, if
  is_array($HTTP_SESSION_VARS) was true.

- Sascha


Index: session.c
===
RCS file: /repository/php4/ext/session/session.c,v
retrieving revision 1.323
diff -u -r1.323 session.c
--- session.c   1 Oct 2002 11:59:45 -   1.323
+++ session.c   2 Oct 2002 09:10:37 -
@@ -258,7 +258,9 @@
if (sym_global == NULL && sym_track == NULL) {
zval *empty_var;
 
-   ALLOC_INIT_ZVAL(empty_var);
+   ALLOC_INIT_ZVAL(empty_var); /* this sets refcount to 1 */
+   ZVAL_DELREF(empty_var); /* our module does not maintain a ref 
+*/
+   /* The next call will increase refcount by NR_OF_SYM_TABLES==2 
+*/
zend_set_hash_symbol(empty_var, name, namelen, 1, 2, 
Z_ARRVAL_P(PS(http_session_vars)), &EG(symbol_table));
} else if (sym_global == NULL) {
zend_set_hash_symbol(*sym_track, name, namelen, 1, 1, 
&EG(symbol_table));
@@ -270,7 +272,7 @@
zval *empty_var;

ALLOC_INIT_ZVAL(empty_var);
-   zend_set_hash_symbol(empty_var, name, namelen, 0, 1, 
Z_ARRVAL_P(PS(http_session_vars)));
+   ZEND_SET_SYMBOL_WITH_LENGTH(Z_ARRVAL_P(PS(http_session_vars)), 
+name, namelen+1, empty_var, 1, 0);
}
}
 }
@@ -465,23 +467,16 @@
 
 static void php_session_track_init(TSRMLS_D)
 {
-   zval **old_vars = NULL;
+   /* Unconditionally destroy existing arrays -- possible dirty data */
+   zend_hash_del(&EG(symbol_table), "HTTP_SESSION_VARS", 
+   sizeof("HTTP_SESSION_VARS"));
+   zend_hash_del(&EG(symbol_table), "_SESSION", sizeof("_SESSION"));
 
-   if (zend_hash_find(&EG(symbol_table), "HTTP_SESSION_VARS", 
sizeof("HTTP_SESSION_VARS"), (void **)&old_vars) == SUCCESS && Z_TYPE_PP(old_vars) == 
IS_ARRAY) {
-   PS(http_session_vars) = *old_vars;
-   zend_hash_clean(Z_ARRVAL_P(PS(http_session_vars)));
-   } else {
-   if(old_vars) {
-   zend_hash_del(&EG(symbol_table), "HTTP_SESSION_VARS", 
sizeof("HTTP_SESSION_VARS"));
-   zend_hash_del(&EG(symbol_table), "_SESSION", 
sizeof("_SESSION"));
-   }
-   MAKE_STD_ZVAL(PS(http_session_vars));
-   array_init(PS(http_session_vars));
-   PS(http_session_vars)->refcount = 2;
-   PS(http_session_vars)->is_ref = 1;
-   zend_hash_update(&EG(symbol_table), "HTTP_SESSION_VARS", 
sizeof("HTTP_SESSION_VARS"), &PS(http_session_vars), sizeof(zval *), NULL);
-   zend_hash_update(&EG(symbol_table), "_SESSION", sizeof("_SESSION"), 
&PS(http_session_vars), sizeof(zval *), NULL);
-   }
+   MAKE_STD_ZVAL(PS(http_session_vars));
+   array_init(PS(http_session_vars));
+   
+   ZEND_SET_GLOBAL_VAR("HTTP_SESSION_VARS", PS(http_session_vars));
+   ZEND_SET_GLOBAL_VAR("_SESSION", PS(http_session_vars));
 }
 
 static char *php_session_encode(int *newlen TSRMLS_DC)


-- 
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php