# Unserialize is broken

- Thread: https://externals.io/message/47061
- Original archive: https://news-web.php.net/php.internals/47061
- Started by Jordi Boggiano on 2010-02-28T16:19:59+00:00
- Messages: 8
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #47061 — Jordi Boggiano — 2010-02-28T16:19:59+00:00

URL: https://externals.io/message/47061#47061

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

---

## #47062 — Herman Radtke — 2010-02-28T19:03:06+00:00

URL: https://externals.io/message/47061#47062 · In reply to: #47061

> 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

---

## #47063 — Richard Quadling — 2010-02-28T19:35:15+00:00

URL: https://externals.io/message/47061#47063 · In reply to: #47062

> > […]
>
> 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

---

## #47064 — Jordi Boggiano — 2010-02-28T20:00:05+00:00

URL: https://externals.io/message/47061#47064 · In reply to: #47062

> > […]
>
> 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

---

## #47066 — Tjerk Meesters — 2010-03-01T09:31:54+00:00

URL: https://externals.io/message/47061#47066 · In reply to: #47064

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:
> > […]
>
> 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

---

## #47068 — Stan Vassilev — 2010-03-01T10:02:01+00:00

URL: https://externals.io/message/47061#47068 · In reply to: #47066

> 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

---

## #47069 — Jordi Boggiano — 2010-03-01T10:10:46+00:00

URL: https://externals.io/message/47061#47069 · In reply to: #47066

> 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

---

## #47073 — Scott MacVicar — 2010-03-01T17:11:13+00:00

URL: https://externals.io/message/47061#47073 · In reply to: #47069

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



> 
> > […]
>
> 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