Heya,
This bug was closed as Bogus http://bugs.php.net/bug.php?id=51173 and
Pierre told me to bring the discussion here since I was ranting on irc.
Johannes argued that the fact unserialize doesn't check the access level
of properties before generating object is good because it allows hackish
features, and that this is intended behavior. Great, but I don't get
what it ultimately allows you to do.
To show this I have expanded my reproduce code and reversed the example:
Run this first to populate the session with an object with a protected
property.
session_start()
;class testClass {
protected $property;
public function __construct($val) {
$this->property = $val;
}
public function getProperty() {
return $this->property;
}
}$_SESSION['obj'] = new testClass('value');
After that you run this script, where the property has been changed to
public, the object that is unserialized from the session has become
completely unusable.
session_start()
;class testClass {
public $property;
public function __construct($val) {
$this->property = $val;
}
public function getProperty() {
return $this->property;
}
}if (!isset($_SESSION['obj'])) {
$_SESSION['obj'] = new testClass('value');
}var_dump($_SESSION['obj']);
var_dump($_SESSION['obj']->property);
var_dump($_SESSION['obj']->getProperty());
If you don't want to run it, the output is the following :
object(testClass)[1]
public 'property' => null
protected 'property' => string 'value' (length=5)
null
null
The "value" you had stored in the object when you serialized it is now
gone and it's impossible to retrieve it, either from inside or outside
the object. If you do it reversed (switch public to protected),
accessing value from outside the object results in a fatal error, even
though there is a "public 'property' => string 'value'" in the var_dump
output.
Imo unserialize should check, when applying public or protected values,
if either exists on the object, and apply it to the one that exists.
Sure it's gonna cost some performance, but at least changing the
prototype of your class while stuff is running isn't going to kill your
code anymore. For private it's a bit different since private vars belong
to every "class" and not the object as a whole, I guess those should be
unserialized back in their place. I don't see any potential BC break
since doing this atm breaks your code, but people always amazed me with
their ability to create code that relies on broken behavior, so maybe I
overlooked something ?
Cheers,
Jordi
Imo unserialize should check, when applying public or protected values,
if either exists on the object, and apply it to the one that exists.
Sure it's gonna cost some performance, but at least changing the
prototype of your class while stuff is running isn't going to kill your
code anymore.
This seems like a corner case and one that a conversion script should
handle. Considering that serialize and unserialize are called for
every single web-request, degrading the performance of unserialize is
not something that should be done lightly.
--
Herman Radtke
hermanradtke@gmail.com | http://hermanradtke.com
Imo unserialize should check, when applying public or protected values,
if either exists on the object, and apply it to the one that exists.
Sure it's gonna cost some performance, but at least changing the
prototype of your class while stuff is running isn't going to kill your
code anymore.This seems like a corner case and one that a conversion script should
handle. Considering that serialize and unserialize are called for
every single web-request, degrading the performance of unserialize is
not something that should be done lightly.--
Herman Radtke
hermanradtke@gmail.com | http://hermanradtke.com--
As serializing reports protected and private properties, would it not
be right that unserialize should work within the private scope of an
object and then follow the same rules as userland code.
It would guess that unserialize really doesn't need to know the
visibility or at least shouldn't be creating properties based upon
those visibilities but rather applying the values to the existing
properties.
--
Richard Quadling
"Standing on the shoulders of some very clever giants!"
EE : http://www.experts-exchange.com/M_248814.html
EE4Free : http://www.experts-exchange.com/becomeAnExpert.jsp
Zend Certified Engineer : http://zend.com/zce.php?c=ZEND002498&r=213474731
ZOPA : http://uk.zopa.com/member/RQuadling
Imo unserialize should check, when applying public or protected values,
if either exists on the object, and apply it to the one that exists.
Sure it's gonna cost some performance, but at least changing the
prototype of your class while stuff is running isn't going to kill your
code anymore.This seems like a corner case and one that a conversion script should
handle. Considering that serialize and unserialize are called for
every single web-request, degrading the performance of unserialize is
not something that should be done lightly.
Well.. the problem is you can't use __wakeup() for it, because at that
point it's too late to read the values, it could work if you got the
properties as an array or something as __wakeup($data), but that's not
the case. So the only approach would be to edit the scripts manually,
and that's the last thing I want to do honestly, in the playing with
fire category serialized strings are pretty high ranked.
Patching __wakeup handling could be a non-costly option I suppose, but
that's just another hack on top of the hackish hacks, and I don't want
to be the cause of it, so if unserialize can't be fixed I'd rather
have nothing.
Cheers,
Jordi
Hi,
If visibility is an issue why not just use json_enode/decode for this
case then?it doesn't seem like a typical enough problem to be solved
low-level and json seems fast enough for the job ;-)
On Sun, Feb 28, 2010 at 8:03 PM, Herman Radtke
hermanradtke@gmail.com wrote:Imo unserialize should check, when applying public or protected
values,
if either exists on the object, and apply it to the one that exists.
Sure it's gonna cost some performance, but at least changing the
prototype of your class while stuff is running isn't going to kill
your
code anymore.This seems like a corner case and one that a conversion script should
handle. Considering that serialize and unserialize are called for
every single web-request, degrading the performance of unserialize is
not something that should be done lightly.Well.. the problem is you can't use __wakeup() for it, because at that
point it's too late to read the values, it could work if you got the
properties as an array or something as __wakeup($data), but that's not
the case. So the only approach would be to edit the scripts manually,
and that's the last thing I want to do honestly, in the playing with
fire category serialized strings are pretty high ranked.Patching __wakeup handling could be a non-costly option I suppose, but
that's just another hack on top of the hackish hacks, and I don't want
to be the cause of it, so if unserialize can't be fixed I'd rather
have nothing.Cheers,
Jordi
Hi,
If visibility is an issue why not just use json_enode/decode for this
case then?it doesn't seem like a typical enough problem to be solved
low-level and json seems fast enough for the job ;-)
Hi,
I don't know whether it should be fixed or not, but it definitely shouldn't
have received resolution "Bogus" in the bugbase.
"Bogus" means the issue doesn't exist as described, is not caused by PHP, or
it's part of the expected, intended, documented behavior of the language. In
this case the issue is an undocumented edge case, it's counterintuitive, and
it's reproducible with the provided code. If it not feasible to fix due to
performance reason, the resolution should be "Won't Fix".
Marking a real issue as a bogus one deprives people who work on PHP from
having a handy log of issues that could be reviewed and possibly fixed at a
later point as the egine changes and a solution no longer is a BC or
performance issue. It's also insulting to the submitter, who took from his
time to file an actual problem and was basically dismissed as uninformed on
the issue.
Regards,
Stan Vassilev
If visibility is an issue why not just use json_enode/decode for this
case then?it doesn't seem like a typical enough problem to be solved
low-level and json seems fast enough for the job ;-)
Honestly I personally don't care, it won't happen to me again, I know it
now, but I want it to be fixed for the benefit of others. Sure it's a
rare case, but if you're gonna pretend it's ok that objects have ghost
properties set to them that are unreadable, I guess there is no point in
discussing this further.
Should I btw remind you that the following results in a Fatal error:
Cannot redeclare Test::$foo ?
class Test {
protected $foo;
public $foo;
}
So it's illegal, but let's do it anyway because in php core it's funny
to do weird stuff that doesn't make any sense?
And in any case json_encode doesn't work the same at all since it
doesn't encode protected properties, and doesn't store the object type,
so it'd be much more work.
Cheers,
Jordi
Java has a transient keyword to skip serialising a property and I have
a patch against 5.3 on http://whisky.macvicar.net/patches/
It might make it in to 5.4/6/next once I get some more free time.
Scott
If visibility is an issue why not just use json_enode/decode for this
case then?it doesn't seem like a typical enough problem to be solved
low-level and json seems fast enough for the job ;-)Honestly I personally don't care, it won't happen to me again, I
know it
now, but I want it to be fixed for the benefit of others. Sure it's a
rare case, but if you're gonna pretend it's ok that objects have ghost
properties set to them that are unreadable, I guess there is no
point in
discussing this further.Should I btw remind you that the following results in a Fatal error:
Cannot redeclare Test::$foo ?class Test {
protected $foo;
public $foo;
}So it's illegal, but let's do it anyway because in php core it's funny
to do weird stuff that doesn't make any sense?And in any case json_encode doesn't work the same at all since it
doesn't encode protected properties, and doesn't store the object
type,
so it'd be much more work.Cheers,
Jordi