WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 2 weeks ago

#37082 reopened enhancement

Remove (most) uses of create_function() from core

Reported by: sgolemon Owned by: rmccue
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: General Keywords: needs-patch
Focuses: docs Cc:

Description

There are currently five uses of create_function() in WP core. Four of them may be trivially refactored into simpler, more secure, and (marginally) more performant versions which do not use create_function() while still retaining PHP 5.2.0 compatability.

The fifth use, in wp-includes/pomo/translations.php relies on arbitrary code evaluation and can't be trivially refactored, so it's been left out of this issue.

Attachments (6)

0001-Remove-two-unnecessary-uses-of-create_function-in-PO.patch (2.0 KB) - added by sgolemon 10 months ago.
0001-Remove-two-unnecessary-uses-of-create_function-in-PO.patch
0002-Remove-unnecessary-use-of-create_function-in-PO-read.patch (655 bytes) - added by sgolemon 10 months ago.
0002-Remove-unnecessary-use-of-create_function-in-PO-read.patch
0003-Remove-unnecessary-use-of-create_function-in-PO-prep.patch (1.2 KB) - added by sgolemon 10 months ago.
0003-Remove-unnecessary-use-of-create_function-in-PO-prep.patch
0004-Remove-unnecessary-use-of-create_function-in-AtomFee.patch (1.5 KB) - added by sgolemon 10 months ago.
0004-Remove-unnecessary-use-of-create_function-in-AtomFee.patch
37082.diff (4.7 KB) - added by rmccue 3 months ago.
Updated and consolidated patch
37082.2.diff (3.4 KB) - added by desrosj 6 days ago.
Replaces create_function() calls in unit tests.

Download all attachments as: .zip

Change History (23)

@sgolemon
10 months ago

0001-Remove-two-unnecessary-uses-of-create_function-in-PO.patch

@sgolemon
10 months ago

0002-Remove-unnecessary-use-of-create_function-in-PO-read.patch

@sgolemon
10 months ago

0003-Remove-unnecessary-use-of-create_function-in-PO-prep.patch

@sgolemon
10 months ago

0004-Remove-unnecessary-use-of-create_function-in-AtomFee.patch

#1 follow-up: @sgolemon
10 months ago

On the fourth usage, I have some concerns about the current default implementations, specifically regarding unfiltered/unsanitized output.

Given that these methods are designed to produce XML declaration pairs, I'd like to apply a label/string/uri sanitization such as the following:

public static function defaultMapAttrsFunc($k, $v) {
    return preg_replace('/[0-9A-Za-z_-]+/', "", $k) . '="' . filter_var($v, FILTER_SANITIZE_ENCODED) . '"';
}

and similar in defaultMapXmlnsFunc, of course.

Last edited 9 months ago by ocean90 (previous) (diff)

#2 in reply to: ↑ 1 @rmccue
10 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.6

Welcome to Trac, and thanks for the patches!

Replying to sgolemon:

On the fourth usage, I have some concerns about the current default implementations, specifically regarding unfiltered/unsanitized output.

You're totally right that the code in here sucks, but it's actually no longer used in core, and only provided for backwards compatibility purposes (AtomPub was removed in 3.5 in #21866), so we don't really need to fix it.

(Also, the filter extension can be disabled, so we can't use filter_var anyway.)

I think 0004-Remove-unnecessary-use-of-create_function-in-AtomFee.patch should be fine just to remove the create_function calls. I'm not entirely sure how the POMO library stuff is handled, I believe it's managed externally in GlotPress. Pinging @ocean90 on that one.

The code all looks good-to-go though, pending the logistical bits of how we actually merge it.

Sidenote: Apart from these usages of create_function, the only other one I see is in Translations::make_plural_form_function(), which looks absolutely crazy to me. Plural-Forms in gettext is actually a full C-like expression, so we either need to write our own parser or continue using this eval-ish hack. This concerns me, but seems like that'll need to stay that way.

#3 @sgolemon
10 months ago

I think 0004-Remove-unnecessary-use-of-create_function-in-AtomFee.patch​ should be fine just to remove the create_function calls.

Right, in terms of addressing the issue of create_function() usage (which should be avoided wherever possible), these patches are focused on JUST that change (I subscribe to the one-diff-one-change rule). All my other comments about 0004 were related to "things that should probably be addressed in followup diffs at some point".

(Also, the filter extension can be disabled, so we can't use filter_var anyway.)

Ugh, fair point. Still, for what that default implementation is trying to do, a simple str_replace will go a long way towards improving things.

public static function defaultMapAttrsFunc($k, $v) {
  $k = preg_replace('/[^0-9A-Za-z_-]+/', "", $k);
  if (function_exists('filter_var')) {
    $v = filter_var($v, FILTER_SANITIZE_ENCODED);
  } else {
    // Absolute minimal safeguard, there are known browser bugs,
    // especially in old versions of IE which render this ineffective,
    // but I cringe at doing nothing at all.
   $v = str_replace('"', '%22', $v);
  }
  return "$k=\"$v\"";
}

But again, something for a followup and perhaps not even necessary to fix depending on how the API is used by the modules which depend on it. Just something I thought I'd mention while I was in the neighborhood.

Translations::make_plural_form_function(), which looks absolutely crazy to me.

Yep, that was the usage I mentioned in the OP, and it's clearly just a disguised eval(). It's complex enough that any work done on it deserves its own ticket, and certainly needs someone who understands WP's internals better than I.

BTW, for context, these create_function() uses came up during a hackathon at Facebook months ago (looking for opportunities to improve HHVM's ability to run WP). I informally handed these fixes off to a coworker who had closer ties to WP, but I guess he never got around to passing them on, so I recreated the (pretty simple) diffs. Unsurprisingly, these made no practical difference to performance, but the avoidance of create_function() was worth it in terms of correctness.

#4 @ocean90
10 months ago

  • Owner set to rmccue
  • Status changed from new to assigned

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


9 months ago

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


9 months ago

#7 @ocean90
9 months ago

  • Keywords upstream added
  • Milestone changed from 4.6 to Future Release
  • Version trunk deleted

#8 @rmccue
5 months ago

  • Keywords 4.8-early added

I'm going to tackle this in 4.8; it's alas too late for 4.7 now, and we totally slipped 4.6. Sorry about that, that's on me.

#9 @rmccue
3 months ago

  • Keywords 4.8-early removed
  • Milestone changed from Future Release to 4.8

@rmccue
3 months ago

Updated and consolidated patch

#10 @rmccue
3 months ago

Updated the patch against trunk, and edited some of the docs and naming to be a little more WordPress-y.

#11 @rmccue
3 months ago

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

In 39591:

General: Remove most uses of create_function()

create_function() is equivalent to eval(), and most of our uses can be refactored. This is simpler, more secure, and slightly more performant.

Props sgolemon.
Fixes #37082.

#12 @rmccue
3 months ago

In 39592:

General: Correctly detect trailing newline when prepending.

We need to check that the final line is actually an artifact of explode(), not just an empty input string.

See #37082.

#13 @johnbillion
3 months ago

  • Focuses docs added
  • Keywords needs-patch added; has-patch upstream removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Those new methods need @since docs.

#14 @ocean90
3 months ago

@johnbillion These are external libraries, I don't think we should use WP versions for them.

#15 @knutsp
8 weeks ago

PHP 7.2 will deprecate create_function https://wiki.php.net/rfc/deprecations_php_7_2#create_function

So if core is to run on PHP 7.2 without deprecation warnings from PHP I guess we need a plan to remove the last one, too. A new ticket only for this or collect some more preparations for PHP 7.2?

#16 @sgolemon
8 weeks ago

7.2 is keeping eval, so that last usage of create_function() could be migrated to be 7.2 safe easily enough. It'd also be a much more honest admission of what that function does (which is terrible).

#17 @ocean90
2 weeks ago

We also have to update tests which are using create_function(): https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/209970096

@desrosj
6 days ago

Replaces create_function() calls in unit tests.

Note: See TracTickets for help on using tickets.