WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 20 months ago

Last modified 19 months ago

#45895 closed defect (bug) (wontfix)

maybe_unserialize: handle exceptions thrown during unserialization

Reported by: bluefuton Owned by: desrosj
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

In PHP 7.2+, it's possible for unserialize() to fail with an uncaught exception when handed a serialized SimpleXMLElement object, like this:

$input = 'O:16:"SimpleXMLElement":0:{}';
$output = null;

try {
  $output = @unserialize( $input );
} catch ( Exception $e ) {
  var_dump( $e );
}

var_dump( $output );

The maybe_unserialize() function does not currently include any exception handling.

The attached patch handles the scenario where unserialize() throws an exception, and also adds unit tests for maybe_unserialize().

Attachments (2)

maybe_unserialize_handle_exception.patch (1.9 KB) - added by bluefuton 22 months ago.
Patch adding exception handling to maybe_unserialize() and unit tests
example.php (176 bytes) - added by bluefuton 22 months ago.
Example showing the output of unserialize() with an empty serialized SimpleXMLElement object

Download all attachments as: .zip

Change History (10)

@bluefuton
22 months ago

Patch adding exception handling to maybe_unserialize() and unit tests

@bluefuton
22 months ago

Example showing the output of unserialize() with an empty serialized SimpleXMLElement object

#1 @desrosj
22 months ago

  • Milestone changed from Awaiting Review to 5.2
  • Owner set to desrosj
  • Status changed from new to reviewing

Hey @bluefuton, thanks for this, and welcome to Trac!

This looks good at first glance, but I'd like to review it further.

I'm putting this in 5.2, because the list of bugs in 5.1 is currently at 216. If that can be scrubbed down with enough time before RC (slated for February 7), we can look at getting this 5.1 in as well.

#2 @desrosj
20 months ago

  • Keywords reporter-feedback added

@bluefuton I started digging into this a bit deeper, but I have been unable to trigger an exception with your example input. Also, maybe_unserialize_handle_exception.patch is causing failures elsewhere in the test suite.

I have tested with PHP versions all the way back to 5.6. Each version returns a false boolean when passing your test input value to unserialize().

If I remove error suppression (@), I see the following error in PHP < 7.3, but I still get a false boolean:

Warning: Erroneous data format for unserializing 'SimpleXMLElement' in /path/to/file
Notice: unserialize(): Error at offset 27 of 28 bytes in /path/to/file

Can you provide some more information to help me see what I am missing?

#3 @desrosj
20 months ago

  • Keywords needs-refresh added

Looking into it some more, I can't find any documentation anywhere that notes unserialize() would ever throw an exception. But, an exception is thrown when attempting to serialize a SimpleXMLElement.

add_action( 'init', function() {
	$xml = new SimpleXMLElement( '<xml></xml>' );

	$output = null;

	try {
		$output = maybe_serialize( $xml );
	} catch ( Exception $e ) {
		var_dump( $e );
	}

	var_dump( $output );
});

It also seems that attempting to serialize a closure function will cause an exception.

add_action( 'init', function() {
	$closure_function = function(){
		$test = 'some string';
	};

	$output = null;

	try {
		$output = maybe_serialize( $closure_function );
	} catch ( Exception $e ) {
		var_dump( $e );
	}

	var_dump( $output );
});

Can you confirm that this is what you experienced, @bluefuton?

#4 @desrosj
20 months ago

  • Keywords close 2nd-opinion added; needs-refresh removed

I returned to this today, and after thinking it through some more, I am marking as a candidate to close as a wontfix pending a second opinion.

I don't think we should prevent an exception when attempting to serialize a closure or SimpleXMLElement for a few reasons.

First, if the function was changed to just return the data if it was not able to be serialized, it would then be failing silently and the developer would not have any way to know why the data was not serialized unless the function's return value was changed to allow a WP_Error or false. But, that is not really an option because this function is so old.

Instead of returning a value to indicate an error, a _doing_it_wrong() could be called. But at that point, Core would be doing the exact same thing as the exception. It also would circumvent any custom exception handling configured for a site using set_exception_handler().

There are also very few occurrences of try {} catch{} in Core. The main exceptions are bundled external libraries (such as PHPMailer or getID3), and a few instances where exceptions could be thrown for unknown or unpredictable reasons (such as with the Imagick image editor).

#5 @earnjam
20 months ago

I'd just add to that, why would you serialize a SimpleXMLElement?

If you need it in string format, couldn't you just convert it back to XML using SimpleXMLElement::asXML?

http://php.net/manual/en/simplexmlelement.asxml.php

#6 @desrosj
20 months ago

  • Keywords reporter-feedback close 2nd-opinion removed
  • Milestone 5.2 deleted
  • Resolution set to invalid
  • Status changed from reviewing to closed
  • Version 5.1 deleted

I'm going to close this one out. @bluefuton if you still feel that this should be addressed, feel free to reopen with more details. Thanks again for this report!

#7 @desrosj
20 months ago

  • Resolution changed from invalid to wontfix

#8 @bluefuton
19 months ago

Thanks for looking into this @desrosj!

I understand your wontfix decision here, but just wanted to add some additional details in case someone returns to this one.

The exception I saw was thrown in PHP 7.2.13 and it was definitely during unserialization. Using the code in the description, I received:

object(Exception)#1 (7) {
  ["message":protected]=>
  string(52) "Unserialization of 'SimpleXMLElement' is not allowed"
  ["string":"Exception":private]=>
  string(0) ""
  ["code":protected]=>
  int(0)
  ["file":protected]=>
  string(25) "/home/wpcom/test-php7.php"
  ["line":protected]=>
  int(7)
  ["trace":"Exception":private]=>
  array(1) {
    [0]=>
    array(4) {
      ["file"]=>
      string(25) "/home/wpcom/test-php7.php"
      ["line"]=>
      int(7)
      ["function"]=>
      string(11) "unserialize"
      ["args"]=>
      array(1) {
        [0]=>
        string(28) "O:16:"SimpleXMLElement":0:{}"
      }
    }
  }
  ["previous":"Exception":private]=>
  NULL
}
NULL

We encountered this on WordPress.com in a place where we unserialize post meta. One site had an empty SimpleXMLElement serialized in post meta (apparently created by a site importer). We now handle the exception outside of maybe_serialize in that scenario.

Note: See TracTickets for help on using tickets.