Remove (most) uses of create_function() from core

[sgolemon]

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.

[sgolemon]

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

[sgolemon]

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

[sgolemon]

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

[sgolemon]

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

[sgolemon]

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.

[rmccue]

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.

[sgolemon]

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.

[rmccue]

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.

[rmccue]

Updated and consolidated patch

[rmccue]

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

[rmccue]

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.

[rmccue]

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.

[johnbillion]

Those new methods need @since docs.

[ocean90]

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

[knutsp]

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?

[sgolemon]

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).

[ocean90]

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

[desrosj]

Replaces create_function() calls in unit tests.

[johnbillion]

In 40392:

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

Props desrosj

See #37082

[desrosj]

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.

[ocean90]

#38588 was marked as a duplicate.

[ayeshrajans]

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