Serializing exceptions

9 years ago by Juan Basso — view source

unread

Maybe you can implement the __sleep method and just return the documented
attributes (message, code, file, line).

Not perfect, but probably more useful than throw an exception, specially if
the exception is something that is the attribute of some object that is
being serialized.

Juan Basso

Hi!

Looking into some issue, I've discovered that, to my surprise,
Exceptions are serializable. Except that it doesn't always work of
course (e.g. see http://stackoverflow.com/q/9747813/214196) because
exceptions contain backtraces, and those can contain non-serializable
objects. So in reality, you never know if you can serialize it or not.

So, I wonder - would it be ok to make exceptions not serializable at
all? I think that would prevent major WTF factor when people try it and
it randomly fails.

Thoughts?

Stas Malyshev
smalyshev@gmail.com

9 years ago by Pierre Joye — view source

unread

Maybe you can implement the __sleep method and just return the documented
attributes (message, code, file, line).

Not perfect, but probably more useful than throw an exception, specially if
the exception is something that is the attribute of some object that is
being serialized.

Well, can someone explain me why some code would need to serialize
exception in the 1st place?

Cheers,

Pierre

@pierrejoye | http://www.libgd.org

9 years ago by Robert Stoll — view source

unread

-----Ursprüngliche Nachricht-----
Von: Pierre Joye [mailto:pierre.php@gmail.com]
Gesendet: Montag, 23. März 2015 06:57
An: Juan Basso
Cc: Stanislav Malyshev; PHP Internals
Betreff: Re: [PHP-DEV] Serializing exceptions

Maybe you can implement the __sleep method and just return the
documented attributes (message, code, file, line).

Not perfect, but probably more useful than throw an exception,
specially if the exception is something that is the attribute of some
object that is being serialized.

Well, can someone explain me why some code would need to serialize exception in the 1st place?

Cheers,

Pierre

@pierrejoye | http://www.libgd.org

--

Maybe some user defined mechanism for remote exception handling

9 years ago by Stanislav Malyshev — view source

unread

Hi!

Maybe you can implement the __sleep method and just return the
documented attributes (message, code, file, line).

That would be an option, but before going there, my question is - does
anybody need it, really?

Stas Malyshev
smalyshev@gmail.com

9 years ago by Christian Stoller — view source

unread

From: Stanislav Malyshev [mailto:smalyshev@gmail.com], Sent: Monday, March 23, 2015 7:45 AM

Hi!

Maybe you can implement the __sleep method and just return the
documented attributes (message, code, file, line).

That would be an option, but before going there, my question is - does
anybody need it, really?

Hi.

Symfony2 stores an AuthenticationException in the session,
when login credentials are invalid.

I think, exceptions should be serializable. One could strip
the objects from the backtrace and document it like that.

Christian

9 years ago by Dennis Birkholz — view source

unread

Hi,

Am 23.03.2015 um 06:21 schrieb Stanislav Malyshev:

Looking into some issue, I've discovered that, to my surprise,
Exceptions are serializable. Except that it doesn't always work of
course (e.g. see http://stackoverflow.com/q/9747813/214196) because
exceptions contain backtraces, and those can contain non-serializable
objects. So in reality, you never know if you can serialize it or not.

So, I wonder - would it be ok to make exceptions not serializable at
all? I think that would prevent major WTF factor when people try it and
it randomly fails.

Thoughts?

I think we should discuss if (un)serialization is a first-class
operation in the language and if so, we should try to make everything
serializable. Currently, we introduce more and more unserializable
language features (closures, anonymous classes) which makes it more and
more dangerous to serialize something that could contain any of these
(or any of a number of non-serializable internal classes: mysqli/pdo, etc.)
If we don't want serialization as a first-class operation, we should
make objects not serializable by default unless they implement
serializable or __sleep()/__wakeup() and add an is_serializable() method
to check if an object is serializable. But currently serialization gets
more and more unreliable/prone to runtime errors.

Greets
Dennis

9 years ago by Stanislav Malyshev — view source

unread

Hi!

I think we should discuss if (un)serialization is a first-class
operation in the language and if so, we should try to make everything
serializable. Currently, we introduce more and more unserializable

I don't think we can, not unless we can serialize PHP code (like Java
can have JARs with bytecode). Otherwise, something like closure is
inherently not serializable. Moreover, something resource-bound is not
serializable either since it depends on something external.

If we don't want serialization as a first-class operation, we should

I'm not sure what you mean by "first-class operation". We certainly want
most objects be serializable, and they are. But some are not, and would
never be. The question is - is Exception one of those? I think yes, but
I'd like to hear if somebody says otherwise.

make objects not serializable by default unless they implement
serializable or __sleep()/__wakeup() and add an is_serializable() method
to check if an object is serializable. But currently serialization gets
more and more unreliable/prone to runtime errors.

That's because implementers of classes are not careful to account for
serialization. However, I don't think we need any drastic changes here.

Stas Malyshev
smalyshev@gmail.com

9 years ago by Rowan Collins — view source

unread

Stanislav Malyshev wrote on 23/03/2015 05:21:

Hi!

Looking into some issue, I've discovered that, to my surprise,
Exceptions are serializable. Except that it doesn't always work of
course (e.g. see http://stackoverflow.com/q/9747813/214196) because
exceptions contain backtraces, and those can contain non-serializable
objects. So in reality, you never know if you can serialize it or not.

So, I wonder - would it be ok to make exceptions not serializable at
all? I think that would prevent major WTF factor when people try it and
it randomly fails.

Thoughts?

I came upon exactly this situation some time ago, and posted to the list
about it: http://marc.info/?l=php-internals&m=138118333112741&w=2

The options I presented there are:

Mark all exceptions as non-serializable, as you suggest, removing the
surprise.
Don't capture args in exceptions backtraces, with the additional
benefit of destructors firing more predictably.
Strip args out of the backtrace when serializing.
Have a function to strip the backtrace ready to serialize, and
prevent serialization if it hasn't been called.

Option 1 is the simplest, but just as many have questioned the need to
serialize exceptions, I would question the need for their backtraces to
contain every argument passed to every function in the stack, so would
tend towards option 2. AFAIK, no unserializable information is stored
outside the 'args' element of the backtrace items.

Regards,

Rowan Collins
[IMSoP]

9 years ago by Stas Malyshev — view source

unread

Hi!

Looking into some issue, I've discovered that, to my surprise,

Exceptions are serializable. Except that it doesn't always work of
course (e.g. see http://stackoverflow.com/q/9747813/214196) because
exceptions contain backtraces, and those can contain non-serializable
objects. So in reality, you never know if you can serialize it or not.

So, I wonder - would it be ok to make exceptions not serializable at
all? I think that would prevent major WTF factor when people try it and
it randomly fails.

Since discussion on this did not lead to a definite conclusion, but I did
not hear from anybody that they need serialized exceptions, and we keep
getting bug reports about exception serialization and various issues
triggered by it, I propose this change:
https://github.com/php/php-src/pull/1442

Since it is kind of BC break (even though I assume it breaks something that
needed not to be allowed in the first place) I'd like to merge it in 7.0.0.
Please object if you think this should not be done and explain why.
Otherwise I intend to merge it after a suitable wait and after adding
necessary tests/docs.

Thanks,

Stas Malyshev
smalyshev@gmail.com

9 years ago by Rowan Collins — view source

unread

Stas Malyshev wrote on 27/07/2015 08:08:

Since discussion on this did not lead to a definite conclusion, but I did
not hear from anybody that they need serialized exceptions, and we keep
getting bug reports about exception serialization and various issues
triggered by it, I propose this change:
https://github.com/php/php-src/pull/1442

I agree that this is better than the status quo - although I would still
prefer stripping out the 'args' from the exception backtrace, which has
other benefits for predictable destructors etc.

Out of interest, is there any way to over-ride a denied serialization
like this, using __sleep or Serializable? At the moment, you can create
serializable sub-classes of exception, and retain a summarised copy of
the backtrace: http://3v4l.org/IHPI5

It's not a big deal either way - in the rare cases where serialization
of exception data is necessary, you could just write a wrapper to
extract the public details - but I'm interested if my current hack will
be adaptable if this change is applied.

Regards,

Rowan Collins
[IMSoP]

9 years ago by Nikita Popov — view source

unread

Hi!

Looking into some issue, I've discovered that, to my surprise,

Exceptions are serializable. Except that it doesn't always work of
course (e.g. see http://stackoverflow.com/q/9747813/214196) because
exceptions contain backtraces, and those can contain non-serializable
objects. So in reality, you never know if you can serialize it or not.

So, I wonder - would it be ok to make exceptions not serializable at
all? I think that would prevent major WTF factor when people try it and
it randomly fails.

Since discussion on this did not lead to a definite conclusion, but I did
not hear from anybody that they need serialized exceptions, and we keep
getting bug reports about exception serialization and various issues
triggered by it, I propose this change:
https://github.com/php/php-src/pull/1442

Since it is kind of BC break (even though I assume it breaks something that
needed not to be allowed in the first place) I'd like to merge it in 7.0.0.
Please object if you think this should not be done and explain why.
Otherwise I intend to merge it after a suitable wait and after adding
necessary tests/docs.

-1 on this. If there is no technical problem with serializing the Exception
class itself, it should be possible to serialize it. It can always happen
that an object contains some not-serializable member, this is nothing
specific to exceptions. I don't see the point of this change.

Also, Christian Stoller's mail pointed out that Symfony uses serialized
exceptions.

Nikita

9 years ago by Rowan Collins — view source

unread

Nikita Popov wrote on 28/07/2015 14:07:

-1 on this. If there is no technical problem with serializing the Exception
class itself, it should be possible to serialize it. It can always happen
that an object contains some not-serializable member, this is nothing
specific to exceptions. I don't see the point of this change.

Also, Christian Stoller's mail pointed out that Symfony uses serialized
exceptions.

The problem is that exceptions unpredictably contain non-serializable
members, because it depends on what's happened at arbitrary points in
the backtrace. There is nothing a user catching an exception can do to
predict or prevent it happening, unless (as I have repeatedly suggested)
there is some way to strip the 'args' from the backtrace items.

9 years ago by Stanislav Malyshev — view source

unread

Hi!

-1 on this. If there is no technical problem with serializing the
Exception class itself, it should be possible to serialize it. It can
always happen that an object contains some not-serializable member, this
is nothing specific to exceptions. I don't see the point of this change.

The point is exactly that Exception contains non-serializable members
that makes successfully serializing and restoring them impossible. You
could get back something that looks like an Exception, but not the
original one - namely, you can not store and restore backtraces.

Also, Christian Stoller's mail pointed out that Symfony uses serialized
exceptions.

I must have missed it - what Symphony uses serialized exceptions for?

--
Stas Malyshev
smalyshev@gmail.com

9 years ago by Marco Pivetta — view source

unread

This sort of change would be a major BC break for 8.x or similar. I also
don't see security implications, tbh.

Hi!

-1 on this. If there is no technical problem with serializing the
Exception class itself, it should be possible to serialize it. It can
always happen that an object contains some not-serializable member, this
is nothing specific to exceptions. I don't see the point of this change.

The point is exactly that Exception contains non-serializable members
that makes successfully serializing and restoring them impossible. You
could get back something that looks like an Exception, but not the
original one - namely, you can not store and restore backtraces.

Also, Christian Stoller's mail pointed out that Symfony uses serialized
exceptions.

I must have missed it - what Symphony uses serialized exceptions for?

--
Stas Malyshev
smalyshev@gmail.com

9 years ago by Rowan Collins — view source

unread

This sort of change would be a major BC break for 8.x or similar.

I think that was the point of trying to squeeze it into 7.0

9 years ago by Marco Pivetta — view source

unread

New BC breaks in Beta? Meh.

This sort of change would be a major BC break for 8.x or similar.

I think that was the point of trying to squeeze it into 7.0

9 years ago by Stanislav Malyshev — view source

unread

Hi!

New BC breaks in Beta?

Why not? What's the problem with it? Beta is to identify issues with
current code, serialized exceptions is an issue (as in, they don't work
and lead to security problems and generally pointless). Dragging this
known problem around for years makes no sense.

Stas Malyshev
smalyshev@gmail.com

9 years ago by Anatol Belski — view source

unread

Hi Stas,

-----Original Message-----
From: Stanislav Malyshev [mailto:smalyshev@gmail.com]
Sent: Tuesday, July 28, 2015 10:51 PM
To: Marco Pivetta ocramius@gmail.com; Rowan Collins
rowan.collins@gmail.com
Cc: Nikita Popov nikita.ppv@gmail.com; PHP Internals List
internals@lists.php.net
Subject: Re: [PHP-DEV] Re: Serializing exceptions

Hi!

New BC breaks in Beta?

Why not? What's the problem with it? Beta is to identify issues with current
code, serialized exceptions is an issue (as in, they don't work and lead to security
problems and generally pointless). Dragging this known problem around for
years makes no sense.
I was looking through and seems that situation is not that bad. Reading http://fabien.potencier.org/php-serialization-stack-traces-and-exceptions.html it might be not that meaningful to break symphony. Having upset users is logic as long as we don't offer a solution, while the functionality itself seems to make sense. And after all serialization looks useful in some cases (logging at least). Maybe we should go more fine tuning way, maybe ignoring the thing that cannot be serialized. Say, serializing only a white list in this case?

Regards

Anatol

9 years ago by Stanislav Malyshev — view source

unread

Hi!

This sort of change would be a major BC break for 8.x or similar.

How is it a major BC break? You make it sound like serializing
exceptions is something no application can do without. I have yet to see
a single case where it's useful (yes, I've read the Symphony comment but
I'm not sure why they're doing it and if it's indeed something that
should be done and not an ugly hack like unserializing fake internal
objects).

I also don't see security implications, tbh.

I don't want to discuss it in detail yet, but check out currently open
or recently fixed security issues and see how many of them relate to
serialized exceptions and consequences of that.

Stas Malyshev
smalyshev@gmail.com

9 years ago by Stephen Coakley — view source

unread

Hi!

This sort of change would be a major BC break for 8.x or similar.

How is it a major BC break? You make it sound like serializing
exceptions is something no application can do without. I have yet to see
a single case where it's useful (yes, I've read the Symphony comment but
I'm not sure why they're doing it and if it's indeed something that
should be done and not an ugly hack like unserializing fake internal
objects).

I also don't see security implications, tbh.

I don't want to discuss it in detail yet, but check out currently open
or recently fixed security issues and see how many of them relate to
serialized exceptions and consequences of that.

Stas Malyshev
smalyshev@gmail.com

Serializing exceptions can be useful in parallel code using multiple
processes or threads. I have been working on a concurrency library for a
week or two and I serialize exceptions (excluding stack trace arguments)
to send them back to the calling process to aid in debugging process
failures.

I agree there aren't too many use cases, but there are a few. Of course,
exceptions aren't consistently serializable, which is still a problem
that should be resolved in some way.

--
Stephen Coakley

9 years ago by Stanislav Malyshev — view source

unread

Hi!

week or two and I serialize exceptions (excluding stack trace arguments)
to send them back to the calling process to aid in debugging process
failures.

But then you don't need to serialize Exception. You need to send the
text representation of Exception, for humans to look at, not the live
Exception object. Sending the actual object would be next to impossible
anyway (i.e. how you send over the live DB connection on DB query
exception?).

--
Stas Malyshev
smalyshev@gmail.com

9 years ago by Stephen Coakley — view source

unread

Hi!

week or two and I serialize exceptions (excluding stack trace arguments)
to send them back to the calling process to aid in debugging process
failures.

But then you don't need to serialize Exception. You need to send the
text representation of Exception, for humans to look at, not the live
Exception object. Sending the actual object would be next to impossible
anyway (i.e. how you send over the live DB connection on DB query
exception?).

But the text representation includes the exception message, code, and
the stack trace. That's pretty much the whole thing. But you are right,
I don't want (and couldn't transfer) the live objects related to the
stack trace, if that's what you're referring to. That's why I would omit
the arguments in each stack trace item during serialization, because who
knows what it could be.

I just think that serializing exceptions is a buggy feature right now,
and we should fix the feature instead of throwing it out.

--
Stephen Coakley

9 years ago by Ferenc Kovacs — view source

unread

On Tue, Jul 28, 2015 at 6:40 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:

Hi!

-1 on this. If there is no technical problem with serializing the
Exception class itself, it should be possible to serialize it. It can
always happen that an object contains some not-serializable member, this
is nothing specific to exceptions. I don't see the point of this change.

The point is exactly that Exception contains non-serializable members
that makes successfully serializing and restoring them impossible. You
could get back something that looks like an Exception, but not the
original one - namely, you can not store and restore backtraces.

Personally I feel the restoring them impossible argument weak, consider
that we allow stuff like serializing resources without even a notice.
Based on my own experiences where I had to fix multiple apps when we
introduced the unserializable Closure (mostly error logger and debugging
tools) which got passed as argument in the backtrace I would prefer if we
could remove that restriction.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

9 years ago by Stanislav Malyshev — view source

unread

Hi!

Personally I feel the restoring them impossible argument weak, consider
that we allow stuff like serializing resources without even a notice.

Not sure what you mean by that. If you try to serialize resource, you
just get an integer. Not ideal, as a remanant of the times in PHP where
the approach was "if it doesn't make sense, do whatever and hope the
user is ok with that", but certainly it's not "serializing resources".
It's "ignoring resources when serializing and producing integers
instead". Replacing Exceptions with integers probably won't work that
well :)

Based on my own experiences where I had to fix multiple apps when we
introduced the unserializable Closure (mostly error logger and debugging
tools) which got passed as argument in the backtrace I would prefer if
we could remove that restriction.

I don't see how we can really remove the underlying problem - Exceptions
contain backtraces, which means serializing them tries to serialize a
ton of stuff that may be not only not serializable but outright
dangerous to carry around - such as keys, passwords, etc. Given how many
problems we have had with serialization of complex objects lately, and
given that I still see absolutely no practical use of actually
serializing exceptions I would rather remove it and reduce the
vulnerable surface than keep dealing with dozens of issues that continue
to pop up from that.

BTW what you mean by "unserializable Closure"? As far as I know you can
not serialize Closure.

--
Stas Malyshev
smalyshev@gmail.com

9 years ago by Ferenc Kovacs — view source

unread

On Fri, Jul 31, 2015 at 9:56 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:

Hi!

Personally I feel the restoring them impossible argument weak, consider
that we allow stuff like serializing resources without even a notice.

Not sure what you mean by that. If you try to serialize resource, you
just get an integer. Not ideal, as a remanant of the times in PHP where
the approach was "if it doesn't make sense, do whatever and hope the
user is ok with that", but certainly it's not "serializing resources".

your argument was that we shouldn't allow serialization of such items where
after the unserliazion you won't get the same thing back.
this is where my point about we allow serializing resources just fine was
aimed at.

It's "ignoring resources when serializing and producing integers
instead". Replacing Exceptions with integers probably won't work that
well :)

nobody suggested replacing Exceptions with integers, but serializing
Closure to "Closure #1" or similar (albeit we could even do a better job,
similarly how java8 supports lambda serialization).

Based on my own experiences where I had to fix multiple apps when we
introduced the unserializable Closure (mostly error logger and debugging
tools) which got passed as argument in the backtrace I would prefer if
we could remove that restriction.

I don't see how we can really remove the underlying problem - Exceptions
contain backtraces, which means serializing them tries to serialize a
ton of stuff that may be not only not serializable but outright
dangerous to carry around - such as keys, passwords, etc.

var_export() shares the same problem, should we then also try to remove
__toString() from closures?
or try for looking for passwords or credit card data in the variable?
I think that your argument is really streched. I'm not saying that we
should give more rope for the developers to hang themselves but the current
situations sucks (you can serialize basically anything but Closure) and I
think that your suggested way of solving the problem would create a medium
sized BC break for little gain as it would only fix a specific scenario
(people like in your SO link in the opening post would still try to
debug_backtrace() from their logger/debugger which would trigger the same
problem that the Closure arg can't be serialized).

Given how many
problems we have had with serialization of complex objects lately, and
given that I still see absolutely no practical use of actually
serializing exceptions I would rather remove it and reduce the
vulnerable surface than keep dealing with dozens of issues that continue
to pop up from that.

that's true, but you would only remove a single attack vector via removing
an otherwise useful feature
those issues would be still present, still reported, we should still fix
then, even if you can't trigger them via an exception logger but a session
unserialized.

BTW what you mean by "unserializable Closure"? As far as I know you can
not serialize Closure.

I mean exactly that and btw. that was the original problem in the
Stackoverflow post you cited in your opening post (
http://stackoverflow.com/questions/9747813/storing-a-php-exception-object-in-a-database
).

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

9 years ago by Stanislav Malyshev — view source

unread

Hi!

this is where my point about we allow serializing resources just fine
was aimed at.

It's not "just fine". It's "it was there before we got smarter so now we
can't change it without breaking too many things". But I think we did
get smarter and no longer see silently putting whatever string in place
of object while serializing as an acceptable method of serializing.

nobody suggested replacing Exceptions with integers, but serializing
Closure to "Closure #1" or similar (albeit we could even do a better
job, similarly how java8 supports lambda serialization).

It's not "serialization" in any meaningful sense. It's replacing an
object with a useless string value, silently, hoping the user didn't
actually need it. If you didn't actually need it, why waste space
serializing it at all? And why not to tell the user we can't actually
serialize it?

var_export() shares the same problem, should we then also try to remove
__toString() from closures?

toString is not serialization. toString is producing human-readable
string representation - its very definition implies you lose all object
identity and receive only string. It is completely different thing from
serialization. Serialization is supposed to preserve the object so it
can later be restored. Replacing object with toString() is in no way
serialization.

should give more rope for the developers to hang themselves but the
current situations sucks (you can serialize basically anything

And we have tons of problems with serializing "basically anything",
including a smorgasbord of security issues, because of that. Because of
trying to unserialize objects that weren't supposed to be even
serializable.

Closure) and I think that your suggested way of solving the problem
would create a medium sized BC break for little gain as it would only

The gain is that we get rid of those issues and also get people to
understand what serialization means (as I said it's not "put an object,
get a random string out" and it's supposed to be two-way, not one-way).
Yes, that means some objects could not be serialized. They should not
be serialized, and the fail should be explicit, not in the form
"suddenly I get string instead of object", or, even worse, "my app
crashes randomly when I unserialize complex objects because somebody
stored unexpected object in a property".

that's true, but you would only remove a single attack vector via
removing an otherwise useful feature

I disagree with serializing exceptions being "useful feature", and
"removing a single attack vector" is one attack vector less. Its not
like we can either remove all attack vectors at once (which as we both
know is impossible) or never remove any of them. Removing them as we
identify them is how we can do it. If we can remove a whole class of
them instead of trying to fix a feature of no practical use that already
proven to be source of trouble, it looks like a win to me.

BTW what you mean by "unserializable Closure"? As far as I know you can
not serialize Closure.
I mean exactly that and btw. that was the original problem in the

Well, if you mean serializing closure then you still can't do that. And
I see no reason why you should be doing that.

Stackoverflow post you cited in your opening post
(http://stackoverflow.com/questions/9747813/storing-a-php-exception-object-in-a-database).

I know. Serializing exceptions and storing them in the DB is not a
good way to do logging. And us taking stance "well, just serialize stuff
and we'll throw in whatever strings there instead of objects" encourages
this wrong behavior. That's exactly what we need to fix.

--
Stas Malyshev
smalyshev@gmail.com

9 years ago by Ferenc Kovacs — view source

unread

On Sat, Aug 1, 2015 at 1:10 AM, Stanislav Malyshev smalyshev@gmail.com
wrote:

Hi!

this is where my point about we allow serializing resources just fine
was aimed at.

It's not "just fine". It's "it was there before we got smarter so now we
can't change it without breaking too many things". But I think we did
get smarter and no longer see silently putting whatever string in place
of object while serializing as an acceptable method of serializing.

nobody suggested replacing Exceptions with integers, but serializing
Closure to "Closure #1" or similar (albeit we could even do a better
job, similarly how java8 supports lambda serialization).

It's not "serialization" in any meaningful sense. It's replacing an
object with a useless string value, silently, hoping the user didn't
actually need it. If you didn't actually need it, why waste space
serializing it at all? And why not to tell the user we can't actually
serialize it?

var_export() shares the same problem, should we then also try to remove
__toString() from closures?

toString is not serialization. toString is producing human-readable
string representation - its very definition implies you lose all object
identity and receive only string. It is completely different thing from
serialization. Serialization is supposed to preserve the object so it
can later be restored. Replacing object with toString() is in no way
serialization.

should give more rope for the developers to hang themselves but the
current situations sucks (you can serialize basically anything

And we have tons of problems with serializing "basically anything",
including a smorgasbord of security issues, because of that. Because of
trying to unserialize objects that weren't supposed to be even
serializable.

Closure) and I think that your suggested way of solving the problem
would create a medium sized BC break for little gain as it would only

The gain is that we get rid of those issues and also get people to
understand what serialization means (as I said it's not "put an object,
get a random string out" and it's supposed to be two-way, not one-way).
Yes, that means some objects could not be serialized. They should not
be serialized, and the fail should be explicit, not in the form
"suddenly I get string instead of object", or, even worse, "my app
crashes randomly when I unserialize complex objects because somebody
stored unexpected object in a property".

that's true, but you would only remove a single attack vector via
removing an otherwise useful feature

I disagree with serializing exceptions being "useful feature", and
"removing a single attack vector" is one attack vector less. Its not
like we can either remove all attack vectors at once (which as we both
know is impossible) or never remove any of them. Removing them as we
identify them is how we can do it. If we can remove a whole class of
them instead of trying to fix a feature of no practical use that already
proven to be source of trouble, it looks like a win to me.
BTW what you mean by "unserializable Closure"? As far as I know you
can
not serialize Closure.
I mean exactly that and btw. that was the original problem in the
Well, if you mean serializing closure then you still can't do that. And
I see no reason why you should be doing that.

Stackoverflow post you cited in your opening post
(
http://stackoverflow.com/questions/9747813/storing-a-php-exception-object-in-a-database
).

I know. Serializing exceptions and storing them in the DB is not a
good way to do logging. And us taking stance "well, just serialize stuff
and we'll throw in whatever strings there instead of objects" encourages
this wrong behavior. That's exactly what we need to fix.

--
Stas Malyshev
smalyshev@gmail.com

for the record I won't reply to you until tomorrow, either you don't read
what I'm writing or I'm too tired to properly articulate my point.
if any chance it was the first, then please re-read my first email in this
thread, thanks!

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

9 years ago by Rowan Collins — view source

unread

Hi!

Personally I feel the restoring them impossible argument weak,
consider
that we allow stuff like serializing resources without even a notice.

Not sure what you mean by that. If you try to serialize resource, you
just get an integer. Not ideal, as a remanant of the times in PHP where
the approach was "if it doesn't make sense, do whatever and hope the
user is ok with that", but certainly it's not "serializing resources".
It's "ignoring resources when serializing and producing integers
instead". Replacing Exceptions with integers probably won't work that
well :)

Based on my own experiences where I had to fix multiple apps when we
introduced the unserializable Closure (mostly error logger and
debugging
tools) which got passed as argument in the backtrace I would prefer
if
we could remove that restriction.

I don't see how we can really remove the underlying problem -
Exceptions
contain backtraces, which means serializing them tries to serialize a
ton of stuff that may be not only not serializable but outright
dangerous

As I have pointed out several times, it is only the 'args' section of the backtrace that ever contains unserializable items. The solution is simply not to include that argument - equivalent to setting DEBUG_BACKTRACE_IGNORE_ARGS in a debug_backtrace() call. IIRC the object of called methods is already excluded (equivalent to masking out DEBUG_PROVIDE_OBJECT) so what's left is all strings.

I would have thought that genuine use cases for extracting arbitrary arguments out of an exceptions backtrace would be pretty rare.

Regards,

Rowan Collins

9 years ago by Stanislav Malyshev — view source

unread

Hi!

As I have pointed out several times, it is only the 'args' section of
the backtrace that ever contains unserializable items. The solution

previous could too. In fact, right now, since you can unserialize
exceptions, previous can contain literally anything and so can any other
members. Also, user code can modify any protected properties too.

DEBUG_BACKTRACE_IGNORE_ARGS in a debug_backtrace() call. IIRC the
object of called methods is already excluded (equivalent to masking
out DEBUG_PROVIDE_OBJECT) so what's left is all strings.

I'm not sure how you arrived at the conclusion that all arguments in
backtrace are strings. Arguments can be of any type. When printed, they
are converted to strings, but they are not strings when stored.

--
Stas Malyshev
smalyshev@gmail.com

9 years ago by Rowan Collins — view source

unread

Hi!

As I have pointed out several times, it is only the 'args' section of
the backtrace that ever contains unserializable items. The solution

previous could too. In fact, right now, since you can unserialize
exceptions, previous can contain literally anything and so can any
other
members. Also, user code can modify any protected properties too.

By that logic, no object should be Serializable, because attempting to do so might recurse to a property that can't be. That doesn't make any sense; what we're talking about here are the native properties of a standard exception, not random data stuffed into the data by a user.

DEBUG_BACKTRACE_IGNORE_ARGS in a debug_backtrace() call. IIRC the
object of called methods is already excluded (equivalent to masking
out DEBUG_PROVIDE_OBJECT) so what's left is all strings.

I'm not sure how you arrived at the conclusion that all arguments in
backtrace are strings. Arguments can be of any type. When printed, they
are converted to strings, but they are not strings when stored.

I'll have to recheck when I have more time, and something better than a phone to type on, but from memory, the backtrace which can be retrieved from an exception includes the same information as debug_backtrace(false), that is:

function: string
line: integer
file: string
class: string
type: string
args: array

Of these 6 items, it is only 'args' that can contain an object of any kind, so without this item, the data would be serializable. This would be equivalent to debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS).

If any of these items (other than args) are stored in memory in a different form (which doesn't seem likely to me), that form is completely inaccessible to the user, so converting to string during serialization would effectively be lossless. (Or, pre-converting would have zero BC break.) Similarly, if additional details are stored, those details are inaccessible, so removing them has no impact on any existing (userland) code.

Regards,

Rowan Collins
[IMSoP]

9 years ago by Rowan Collins — view source

unread

DEBUG_BACKTRACE_IGNORE_ARGS in a debug_backtrace() call. IIRC the
object of called methods is already excluded (equivalent to masking
out DEBUG_PROVIDE_OBJECT) so what's left is all strings.
I'm not sure how you arrived at the conclusion that all arguments in
backtrace are strings. Arguments can be of any type. When printed, they
are converted to strings, but they are not strings when stored.
I'll have to recheck when I have more time, and something better than a phone to type on, but from memory, the backtrace which can be retrieved from an exception includes the same information as debug_backtrace(false), that is:

function: string

line: integer

file: string

class: string

type: string

args: array

Of these 6 items, it is only 'args' that can contain an object of any kind, so without this item, the data would be serializable. This would be equivalent to debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS).

If any of these items (other than args) are stored in memory in a different form (which doesn't seem likely to me), that form is completely inaccessible to the user, so converting to string during serialization would effectively be lossless. (Or, pre-converting would have zero BC break.) Similarly, if additional details are stored, those details are inaccessible, so removing them has no impact on any existing (userland) code.

I just double-checked this. You can see that there is not even an
internal reference to objects which were $this in the stack trace,
because the object can be garbage collected separately from the
exception. http://3v4l.org/hOYb9

So, as I said, no object will be implicitly attached to the exception
anywhere other than the 'args' key of the backtrace. Obviously, people
can attach object references wherever they like, but without this
implicit gathering of objects from the entire environment,
"serialize(new Exception)" would be a safe operation.

Slightly abbreviated version:

class Example {
private $label;
public function __construct($label) {
$this->label = $label;
}
public function __sleep() {
echo "Attempt to serialize object with label $this->label\n";
return array('label');
}
public function __destruct() {
echo "Refcount reached zero for object with label $this->label\n";
}
public function throwSomething($unused_parameter) {
throw new Exception;
}
}

$target = new Example('Target of method call');
$parameter = new Example('Parameter passed but never actually used');
try {
$target->throwSomething($parameter);
}
catch ( Exception $e ) {
// Serialize exception, will attempt to serialize $parameter but
not $target
var_dump(serialize($e));
// Destroy $target, as there are no other references to it
unset($target);
// Attempt to do the same for $parameter, but Exception holds a
reference
unset($parameter);
}
echo "-- PHP process cleanup begins here --\n";
// $parameter will have its destructor fired once $e goes out of scope
and relinquishes its reference

Regards,

--
Rowan Collins
[IMSoP]

9 years ago by Stephen Coakley — view source

unread

Hi!

Looking into some issue, I've discovered that, to my surprise,

Exceptions are serializable. Except that it doesn't always work of
course (e.g. see http://stackoverflow.com/q/9747813/214196) because
exceptions contain backtraces, and those can contain non-serializable
objects. So in reality, you never know if you can serialize it or not.

So, I wonder - would it be ok to make exceptions not serializable at
all? I think that would prevent major WTF factor when people try it and
it randomly fails.

Since discussion on this did not lead to a definite conclusion, but I did
not hear from anybody that they need serialized exceptions, and we keep
getting bug reports about exception serialization and various issues
triggered by it, I propose this change:
https://github.com/php/php-src/pull/1442

Since it is kind of BC break (even though I assume it breaks something that
needed not to be allowed in the first place) I'd like to merge it in 7.0.0.
Please object if you think this should not be done and explain why.
Otherwise I intend to merge it after a suitable wait and after adding
necessary tests/docs.

Thanks,

I'm serializing exceptions in a current project, and I would much prefer
losing args (the only part not always serializable) from the trace
than not being able to serialize the exception at all.

Making exceptions not serializable will just result in another userland
wrapper, like a SuperException, that lets you serialize and unserialize
with eval()'s. I think allowing the serialize to drop args in the trace
and just include a warning in the docs about the serialization being lossy.

My 2 cents.

--
Stephen Coakley

Serializing exceptions

Thoughts?

Thoughts?

Cheers,

Cheers,

That would be an option, but before going there, my question is - does anybody need it, really?

That's because implementers of classes are not careful to account for serialization. However, I don't think we need any drastic changes here.

Regards,

Thanks,

Regards,

Why not? What's the problem with it? Beta is to identify issues with current code, serialized exceptions is an issue (as in, they don't work and lead to security problems and generally pointless). Dragging this known problem around for years makes no sense.

I don't want to discuss it in detail yet, but check out currently open or recently fixed security issues and see how many of them relate to serialized exceptions and consequences of that.

I don't want to discuss it in detail yet, but check out currently open or recently fixed security issues and see how many of them relate to serialized exceptions and consequences of that.

Regards,

Regards,

That would be an option, but before going there, my question is - does
anybody need it, really?

That's because implementers of classes are not careful to account for
serialization. However, I don't think we need any drastic changes here.

Why not? What's the problem with it? Beta is to identify issues with
current code, serialized exceptions is an issue (as in, they don't work
and lead to security problems and generally pointless). Dragging this
known problem around for years makes no sense.

I don't want to discuss it in detail yet, but check out currently open
or recently fixed security issues and see how many of them relate to
serialized exceptions and consequences of that.

I don't want to discuss it in detail yet, but check out currently open
or recently fixed security issues and see how many of them relate to
serialized exceptions and consequences of that.