Make WordPress Core

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#37082 closed enhancement (fixed)

Remove (most) uses of create_function() from core

Reported by: sgolemon's profile sgolemon Owned by: rmccue's profile 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 8 years 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 8 years 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 8 years 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 8 years ago.
0004-Remove-unnecessary-use-of-create_function-in-AtomFee.patch
37082.diff (4.7 KB) - added by rmccue 7 years ago.
Updated and consolidated patch
37082.2.diff (3.4 KB) - added by desrosj 7 years ago.
Replaces create_function() calls in unit tests.

Download all attachments as: .zip

Change History (29)

@sgolemon
8 years ago

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

@sgolemon
8 years ago

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

@sgolemon
8 years ago

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

@sgolemon
8 years ago

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

#1 follow-up: @sgolemon
8 years 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 8 years ago by ocean90 (previous) (diff)

#2 in reply to: ↑ 1 @rmccue
8 years 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
8 years 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
8 years ago

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

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


8 years ago

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


8 years ago

#7 @ocean90
8 years ago

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

#8 @rmccue
7 years 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
7 years ago

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

@rmccue
7 years ago

Updated and consolidated patch

#10 @rmccue
7 years ago

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

#11 @rmccue
7 years 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
7 years 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
7 years 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
7 years ago

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

#15 @knutsp
7 years 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
7 years 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
7 years ago

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

@desrosj
7 years ago

Replaces create_function() calls in unit tests.

#18 @johnbillion
7 years ago

In 40392:

Build/Test tools: Remove occurrences of create_function() in unit tests.

Props desrosj

See #37082

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


7 years ago

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


7 years ago

#21 @desrosj
7 years ago

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

There is one remaining create_function() call in core within translations.php. This one is a long term fix already accounted for in a different ticket. Going to close this out.

#22 @ocean90
7 years ago

#38588 was marked as a duplicate.

#23 @ayeshrajans
7 years ago

#41562 is an attempt to fix the remaining create_function() call.

Note: See TracTickets for help on using tickets.