WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 2 months ago

Last modified 2 months ago

#46570 closed defect (bug) (fixed)

the is_serialized function couldn't tell e+n

Reported by: hoythan Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: Cc:
PR Number:

Description

is_serialized
https://developer.wordpress.org/reference/functions/is_serialized/

echo unserialize('d:1.7976931348623157E+308;') // d:1.7976931348623157E+308;

is_serialized('d:1.7976931348623157E+308;') // false

this could be wrong

516: return (bool) preg_match( "/^{$token}:[0-9.E-]+;$end/", $data );

Attachments (2)

46570.diff (472 bytes) - added by killerbishop 3 months ago.
Patch for #46570
46570.1.diff (1.3 KB) - added by donmhico 2 months ago.
Added unit test.

Download all attachments as: .zip

Change History (14)

#1 @SergeyBiryukov
6 months ago

  • Keywords needs-patch needs-unit-tests added
  • Version trunk deleted

Hi @hoythan, welcome to WordPress Trac! Thanks for the report.

Removing the trunk version, as this doesn't seem to be a regression in 5.2.

@killerbishop
3 months ago

Patch for #46570

#2 @killerbishop
3 months ago

  • Keywords has-patch added; needs-patch removed

Issue was definitely related to the regex - plus needs to be accounted for. I've attached a patch.

#3 @SergeyBiryukov
3 months ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#4 follow-up: @donmhico
3 months ago

I have a question. Instead of using regex to check if a string is in 'serialized-format', why not use PHP's native unserialize() to actually unserialize the input string and check for its output. From the PHP docs, unserialize() would return either serialized value or FALSE if the passed data is not unserializeable.

Of course, what i'm thinking is still returning only either TRUE or FALSE for the is_serialized() and not the actual unserialized value, if it's the return value of the unserialize() function. I'm guessing that returning the actual unserialized value might posed backward compat issues.

See https://www.php.net/manual/en/function.unserialize.php.

I can create a patch that will use this approach, however I'm not sure if there's any particular reason why the WP native function is_serialized() is using regex. I need some input from others first.

Last edited 3 months ago by donmhico (previous) (diff)

This ticket was mentioned in Slack in #core by donmhico. View the logs.


3 months ago

#6 @killerbishop
3 months ago

@donmhico - I don't know the full history on why this check is needed - but if client code does not always include a direct follow-up call to unserialize() - unpacking objects - especially ones with expensive __wakeup() routines - would be a reason to manually inspect this way. Another reason maybe to avoid any annoying E_NOTICE that would get triggered (though that could be suppressed).

With PHP 7.1, I suppose the class instantiation cost could be mitigated with the allowed_classes option being set to false. For older PHP version support - it would still need to have something like this routine though.

#7 in reply to: ↑ 4 @SergeyBiryukov
3 months ago

Replying to donmhico:

I have a question. Instead of using regex to check if a string is in 'serialized-format', why not use PHP's native unserialize() to actually unserialize the input string and check for its output. From the PHP docs, unserialize() would return either serialized value or FALSE if the passed data is not unserializeable.

We needed a function that can detect serialized data without doing an actual unserialize test on it. See [4382] and the discussion in #2591.

#8 @donmhico
2 months ago

@killerbishop @SergeyBiryukov - Thank you for your inputs. I'll read the links and i'll probably just create some unit tests for the patch attached.

@donmhico
2 months ago

Added unit test.

#9 @donmhico
2 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#10 @SergeyBiryukov
2 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 45754:

General: Correctly detect large floats in is_serialized().

Props killerbishop, donmhico, hoythan.
Fixes #46570.

#11 @SergeyBiryukov
2 months ago

In 45755:

Coding Standards: Fix WPCS violation in [45754].

See #46570.

#12 @donmhico
2 months ago

#9930 was marked as a duplicate.

Note: See TracTickets for help on using tickets.