php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #75056 Unintuitive gotcha - Exception maintains handles to stack objects
Submitted: 2017-08-09 15:05 UTC Modified: 2017-08-12 23:19 UTC
Votes:2
Avg. Score:5.0 ± 0.0
Reproduced:2 of 2 (100.0%)
Same Version:1 (50.0%)
Same OS:1 (50.0%)
From: nachms+php at gmail dot com Assigned:
Status: Open Package: *General Issues
PHP Version: 5.6.31 OS:
Private report: No CVE-ID:
View Add Comment Developer Edit
Anyone can comment on a bug. Have a simpler test case? Does it work for you on a different platform? Let us know!
Just going to say 'Me too!'? Don't clutter the database with that please — but make sure to vote on the bug!
Your email address:
MUST BE VALID
Solve the problem:
31 + 50 = ?
Subscribe to this entry?

 
 [2017-08-09 15:05 UTC] nachms+php at gmail dot com
Description:
------------
Consider the attached code, it outputs:
The throw did not destruct
destructing
--------------

This is somewhat surprising at first, but is due to exceptions containing a backtrace of all function parameters keeping objects alive somewhat unintuitively.

This really means that to have sane destruction behavior with exception handling, one must always unset the Exception at the end of a catch. Which is usually fine...

However, if one needs to rethrow from an exception handler, this means objects that need to be destructed as early as possible are now long-lived through many nested levels. There is no built-in way in PHP to remove these handles from the exception's backtrace.

With the way PHP currently works, we've found that an exception to handle some occasional error could in turn generate a whole chain reaction of problems simply because objects are now living far outside their scope, holding onto resources they ideally should not.

We're not sure of the best solution here, but it would seem that in order to handle these kinds of cases, PHP needs to offer a way to catch exceptions without keeping handles to stack objects. Perhaps some other way to catch, or some way to tell Exceptions to drop object handles from their backtrace before further handling them.

Test script:
---------------
<?php

$destructed = false;
class c
{
  function __destruct()
  {
    global $destructed;
    $destructed = true;
    echo 'destructing', "\n";
  }
}

function thrower($c) { throw(new Exception('oops')); }

function test()
{
  $c = new c();
  thrower($c);
}

try { test(); }
catch(Exception $e)
{
  echo 'The throw ', ($destructed ? 'destructed' : 'did not destruct'), "\n";
}


Expected result:
----------------
One would intuitively expect:
destructing
The throw destructed


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-08-09 17:02 UTC] stas@php.net
-Package: PHP Language Specification +Package: *General Issues
 [2017-08-11 20:42 UTC] rowan dot collins at gmail dot com
This was actually one of the first topics I raised on the PHP Internals mailing list (which is generally a better place for open-ended discussions like this - http://php.net/mailing-lists.php). That was nearly 4 years ago, and didn't get much response; it then came up again a couple of years later, in the context of serialization errors (which are caused by the same data). See these threads in the archives:

- http://marc.info/?t=138118341600002&r=1&w=2
- http://marc.info/?t=142708828500001&r=1&w=2
- http://marc.info/?t=143798102800002&r=1&w=2

The peculiar thing is that most of the information stored for the backtrace is already reduced to strings (class names and the like), but the *arguments* passed to functions in the trace are preserved as full object references. My inclination remains that removing these object references would solve both the destructor and serialization problems, and I'm going to raise it on the list again. I invite you to join the discussion there.
 [2017-08-12 23:19 UTC] nachms+php at gmail dot com
I'm not familiar with the mailing list, but if you start a discussion there, I can try to join in.
----------------------
While I agree that serialization of exceptions can be very useful for keeping track of errors that occurred and other things, this aspect of holding onto objects can cause some really really bad issues. Take this example:

foreach ($somethings as $something)
{
  echo 'Processing ', $something, "\n";
  try { do_something($something); }
  catch (Exception $e) { echo 'Error occurred with ', $something, "\n"; }
}

At first glance this code looks perfectly fine, right? But if somewhere inside do_something() there's an object controlling resources that ends up part of a call frame, no matter how nested, if there's something which ends up creating a cursor on a database or has an open network handle, and then encounters an error, that cursor or network handle will remain open till the next error occurs! No destructors will fire.

Holding open a database cursor indefinitely can cause further database actions to go wrong. Leaving open a network handle may have security ramifications, or lead to file descriptor starvation. Correct code is actually:

foreach ($somethings as $something)
{
  echo 'Processing ', $something, "\n";
  try { do_something($something); }
  catch (Exception $e)
  {
    echo 'Error occurred with ', $something, "\n";
    unset($e);
  }
}

Without the unset, the Exception lives throughout the duration of the loop since PHP variables have function lifetime not block scope lifetime. The Exception will not die till the next error occurs and the Exception variable in the catch ($e) is overwritten. The Exception in turn keeps resources locked, and living far outside their scope, thereby breaking encapsulation. Typically you expect stack unwinding to enforce function lifetime, but with Exceptions, many variables not only have function lifetime but Exception lifetime!

We ran into this issue the other day, where the lack of an unset had severe ramifications, and it took a team of our developers over 5 hours to track down the bug. I don't think it's the kind of thing any developer is expecting. We've since updated our PHP guidelines to always put an unset into exception handling code where the Exception is not then further used beyond. For cases where there's some master object keeping track of Exceptions for some reason, it's not actually safe for the loop to continue, because desctructors not firing to release managed resources prevents another iteration from working properly.

Basically we discovered with PHP that Exception tracking + loops + resource objects that get passed around is a recipe for disaster, and there does not seem to be any viable solutions to do this in PHP as it currently works. This prevents us from using PHP for various long lived tasks.

Typically you want exception handling to make error handling easier and prevent bugs. But it seems with PHP's designs, once you realize how it works, it can make error handling much harder and create all kinds of bugs if not dealt with properly.
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Aug 29 15:01:52 2017 UTC