Make WordPress Core

Opened 11 years ago

Closed 14 months ago

Last modified 12 months ago

#22951 closed defect (bug) (fixed)

Performance enhancements for esc_url()

Reported by: markjaquith's profile markjaquith Owned by: flixos90's profile flixos90
Milestone: 6.2 Priority: normal
Severity: normal Version: 2.8
Component: Formatting Keywords: has-patch add-to-field-guide
Focuses: performance Cc:

Description

esc_url() gets used a lot on WordPress admin pages. Sometimes 100 times or more. Nacin did some KcacheGrind measurements that had it as 7% of some pages. We can speed it up.

Most of the grind comes from wp_kses_bad_protocol().

I had a thought that we're sort of going about things backwards. We're being very careful to exclude anything harmful — bad characters, bad protocols, duplicate fake-out protocols, etc. But almost 100% of the time, the URL going through it is a http/https URL that has completely normal characters in it. We can detect that really early, and bail.

I did some tests with this approach that showed a good time savings.

Quite obviously, there's no room to compromise on security, so we'll need to be watching unit test, and maybe even writing some new ones for good measure.

Attachments (3)

improve-wp-kses-bad-protocol-performance.22951.diff (1.8 KB) - added by schlessera 7 years ago.
22951.2.diff (2.2 KB) - added by schlessera 7 years ago.
Refreshed the patch and made changes to fix broken tests
22951.patch (2.1 KB) - added by mukesh27 20 months ago.

Download all attachments as: .zip

Change History (46)

#1 @nacin
11 years ago

  • Milestone changed from Awaiting Review to 3.6

I think this would be great for 3.6.

#2 @adamsilverstein
11 years ago

  • Cc ADAMSILVERSTEIN@… added

#3 @mboynes
11 years ago

  • Cc mboynes added

#4 @alex-ye
11 years ago

  • Cc nashwan.doaqan@… added

#5 @sunnyratilal
11 years ago

  • Cc sunnyratilal5@… added

#6 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

#7 @nacin
10 years ago

  • Component changed from Performance to Formatting
  • Focuses performance added

#8 @johnbillion
9 years ago

  • Keywords needs-patch reporter-feedback added
  • Version set to 2.8

Did you ever write a patch for this Mark?

#9 @johnbillion
9 years ago

  • Milestone changed from Future Release to Awaiting Review
  • Owner set to markjaquith
  • Status changed from new to assigned

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


8 years ago

#11 @peterwilsoncc
8 years ago

  • Milestone changed from Awaiting Review to 4.7
  • Owner changed from markjaquith to schlessera

@schlessera assigning to you as requested ands putting on the 4.7 milestone. It can always be moved off if the security stuff requires additional testing.

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


7 years ago

#13 @desrosj
7 years ago

  • Keywords needs-unit-tests added

This has a solid plan from @markjaquith, needs a patch and some unit tests.

#14 @peterwilsoncc
7 years ago

@schlessera did you get a chance to look at this? It's been sitting around for a while so we can postpone if not.

thanks.

#15 @peterwilsoncc
7 years ago

  • Milestone changed from 4.7 to Future Release

#16 @schlessera
7 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch needs-unit-tests removed

@peterwilsoncc Sorry for not getting back to you sooner.

I had some time during a contributor day and made a first attempt at improving performance. Basically, I just add a regex in front of the bad protocol detection to return early for http(s) URLs. I'm sure there might be other performance enhancements at other (earlier) points in the code execution, but these will also have more potential impact on the security implications than what I've currently done.

With the change in place, the existing unit tests in group kses still pass successfully.

The regex I've taken is the @diegoperini one (with a slight modification to remove ftp) from the comparison here: https://mathiasbynens.be/demo/url-regex . It is pretty long and feature-complete, but due to the way the regex is evaluated, it is actually fast enough to provide a real improvement.

I've done some (not 100% scientific) benchmarks to compare the current version of esc_url from trunk against my modified version. For all benchmarks, both versions run on the same machine from within the same process, and only the relative change is stored, to get rid of fluctuations due to allocations of server resources as good as possible. You can see a table with the benchmark results here: https://docs.google.com/spreadsheets/d/1XK5lgAIDMlj6lQSTrelXdD5eh6wAqJuLMS1605iM618/edit?usp=sharing

The benchmark summary: In a normal use case, this change should speed up the esc_url function by 25-30%. In a best case scenario, the speed-up can be as high as 40%, while the absolute worst case scenario (basically, dealing with 100% bad protocol links) produces a slow-down of 2%, which I deem is acceptable given the very low probability of this ever being the case.

The test script I've used to run the benchmarks can be found here: https://gist.github.com/schlessera/d4dd4db71d59dfee6ee6c5b9a0ca8a33

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


7 years ago

#18 @desrosj
7 years ago

This patch still applies cleanly, but I am getting some unit test failures while testing.

@schlessera
7 years ago

Refreshed the patch and made changes to fix broken tests

#19 @schlessera
7 years ago

The test failures were due to two issues:

  • the protocol is normalized to lowercase by the full protocol check
  • http needs to be rejected when only https is allowed, and vice versa.

Both problems should be solved.

#20 @mukesh27
21 months ago

  • Keywords needs-refresh dev-feedback added; reporter-feedback removed

Hi there!

@schlessera is this ticket is in your to-do list for the upcoming version?

The 22951.2.diff patch needs to update against the Trank version.

It's a good addition if we add this so we get some good page speed as esc_url is many places to validate URL.

dev-feedback added for more eyes on this ticket.

@mukesh27
20 months ago

#21 @mukesh27
20 months ago

  • Keywords needs-refresh removed

Patch updated against trunk version.

#22 @mehulkaklotar
16 months ago

  • Keywords needs-testing added

I think this would be great for 6.2

#23 @mukesh27
16 months ago

  • Milestone changed from Future Release to 6.2

As discuss in today's Performance bug scrub it. Let move this ticket in 6.2 for the visibility.

#25 follow-up: @mukesh27
16 months ago

Thanks @markjaquith for the ticket!

PR #3598 that working fine without any issue.

@desrosj @schlessera @azaozz Can you please take a look?

#26 in reply to: ↑ 25 @schlessera
16 months ago

@desrosj @schlessera @azaozz Can you please take a look?

I left a comment on the PR about turning the regex into a constant or static string to avoid having it be reconstructed multiple times per process.

#27 @mukesh27
16 months ago

I appreciate the review and advice, @schlessera. I have updated my PR because the changes make sense.

#28 @azaozz
16 months ago

Looking at the patches/PR, may be missing something but why the big regex?

The idea here is to short-circuit wp_kses_bad_protocol() when that function won't have anything to do. The purpose of this function is to strip "bad" protocols from the beginning of a string (presumably an URL). Seems it is mostly used to prevent things like javascript://alert(123) when the javascript:// protocol is not allowed. (Frankly I think rejecting a URL if it starts with a bad protocol would be better, but that's outside the scope here.)

Also wp_kses_bad_protocol_once() seems to be converting some HTML entities in the whole string. This is to better match the : char. See:

$string  = preg_replace( '/(&#0*58(?![;0-9])|&#x0*3a(?![;a-f0-9]))/i', '$1;', $string );
$string2 = preg_split( '/:|&#0*58;|&#x0*3a;|:/i', $string, 2 );

The proposed regex seem to be validating the whole URL (which is not intended here), and seems it may be missing the HTML entities. In addition thinking there was a problem with null characters that can "throw off" some regex (or some browsers?), so $string = wp_kses_no_null( $string ); should be at the top in all cases.

If we go with the original idea, the patch should probably be doing something like:

  1. Remove null chars.
  2. Check if https:// is allowed in $allowed_protocols. If yes, check if the string starts with https:// case-insensitive. If yes, short-circuit.
  3. Check if http:// is allowed in $allowed_protocols. If yes, check if the string starts with http:// case-insensitive. If yes, short-circuit.

This will catch the most common use cases and will probably speed up wp_kses_bad_protocol() quite a bit. Don't think it needs to use regex at all. Seems stripos( $string, 'https:// ) === 0` would be enough, and a lot faster.

Version 0, edited 16 months ago by azaozz (next)

#30 @mukesh27
16 months ago

@azaozz thanks for the feedback. I have submitted second approach PR #3615. There is some unit tests failing due to string case that i'm checking.

The current version of esc_url from trunk and my modified version have been compared using benchmarks. To eliminate variations caused by server resource allocations as much as feasible, both versions of each benchmark are run on the same computer from within the same process, with only the relative change being recorded. The benchmark results are displayed in a table here: https://docs.google.com/spreadsheets/d/1eR80SuQkn1gZc4pfWE1D6enbYSlpRDqharfBtAkTLb0/edit?usp=sharing

The benchmark summary states that this improvement should speed up the esc_url function by 15% to 20% under typical use conditions. While the worst-case scenario (essentially, handling 100% broken protocol links) results in a slowdown of 1.2%, I consider this tolerable considering the extremely low likelihood that this will ever happen.

The test script I've used to run the benchmarks can be found here: https://gist.github.com/mukeshpanchal27/2f89f1fe0ed6c6ee091bf17d6e08bbb6 (I have edited/updated @schlessera code in this script)

@azaozz commented on PR #3615:


16 months ago
#31

@mukeshpanchal27 Thanks for the patch, https://github.com/WordPress/wordpress-develop/pull/3615/commits/20cca48f624bfb03cfefaa58811a5b087b12a44d looks good!

As you mentioned, some tests were failing because there was no lower case normalization of http(s). Don't think there are any security implications of that, but imho better to do it anyways (to keep the function output just as before).

Changed the code to do this in the fastest way possible (I think). Could you run the speed testing again if not a trouble. Would be good to compare with the previous results.

@mukesh27 commented on PR #3615:


16 months ago
#32

Thanks @azaozz for the feedback and changes.

I have re-run the analysis against both approaches, and below are the results:

Approach 1 use regex - PR #3598

Approach 2 - Current PR

The benchmark summary states that Approach 1 improvement should speed up the esc_url function by 2% to 3% under typical use conditions. I think we should implement some more performant functions for this PR to normalization of http(s) so we get some more performance improvement.

cc. @schlessera

@azaozz commented on PR #3615:


16 months ago
#33

I have re-run the analysis against both approaches

Thanks!

The benchmark summary states that Approach 1 improvement should speed up the esc_url function by 2% to 3%

Yea, this seems negligible. Thinking to get as close as possible to the "real picture" the results have to be weighted. The short-circuit here is only for http(s) because (the assumption is that) this matches most cases. A weighted results would probably be something like:

  • 95% http://example.org/ and https://example.org/.
  • 4.95% HTTP://EXAMPLE.ORG/ and HTTPS://EXAMPLE.ORG/ (his is probably even lower as the browsers normalize the URLs to lower case, but don't think it would matter that much).
  • 0.05% everything else.

It is surprising to see that the big regex from https://github.com/WordPress/wordpress-develop/pull/3598 is actually faster than a strpos( $string, 'https://' ) and eventual strpos( $string, 'http://' ). That regex does a lot of needless things. Perhaps testing with longer strings (most URLs come with a path) would be better. The rest of the code in both patches seems the same, i.e. both call in_array( [protocol], $allowed_protocols, true ), etc.

Looking again at https://github.com/WordPress/wordpress-develop/pull/3598, it doesn't have the wp_kses_no_null( $string ) at the top which seems to be a regression. wp_kses_bad_protocol() is removing null chars from the string. Could that be the slow-down? Another thing there is returning early when the regex can't find a protocol. Don't think that's right. Also str_replace() may be unsafe (can replace more than needed). Will comment there.

@azaozz commented on PR #3615:


16 months ago
#34

Looked at this a bit more, specifically at how many URLs start with upper case HTTP. On the sites I have access to (not that many but still...), there were very few HTTP. Final count was 27 out of over 3000 URIs in post_content (nearly all URIs there are inserted by the editor and most are to uploaded images).

Of course, this is not representative, but thinking the upper case HTTP and HTTPS can probably be ignored here as not significant enough. This will simplify the shortcut code and make it even faster.

Also got curious to see if strpos() or 2x strpos() are faster than preg_match(). Did some quick testing and it seems they are :) The test setup is here. Typical results of 10,000 loops on my test server are:

Test regex:           3.438122 milliseconds.
Test strpos:          1.098487 milliseconds.
Test str_starts_with: 1.301403 milliseconds.

Using strpos() directly is fastest (no surprises) and the simple regex is slowest. These results are for 10,000 runs, so in reality all three are super fast (less than 0.001 millisecond). However since we want to speed things up, thinking that going with the fastest makes most sense. So the proposed code here would be:

function wp_kses_bad_protocol( $string, $allowed_protocols ) {
	$string = wp_kses_no_null( $string );

	// Short-circuit if the string starts with `https://` or `http://`. Most common cases.
	if (
		( 0 === strpos( $string, 'https://' ) && in_array( 'https', $allowed_protocols, true ) ) ||
		( 0 === strpos( $string, 'http://' ) && in_array( 'http', $allowed_protocols, true ) )
	) {
		return $string;
	}

	$iterations = 0;
	.............

@mukesh27 commented on PR #3615:


15 months ago
#35

Yea, this seems negligible. Thinking to get as close as possible to the "real picture" the results have to be weighted. The short-circuit here is only for http(s) because (the assumption is that) this matches most cases. A weighted results would probably be something like:

  • 95% http://example.org/ and https://example.org/.
  • 4.95% HTTP://EXAMPLE.ORG/ and HTTPS://EXAMPLE.ORG/ (his is probably even lower as the browsers normalize the URLs to lower case, but don't think it would matter that much).
  • 0.05% everything else.

If we only consider standard urls (http and https), we have covered 95% of the URLs, which is fantastic. When the changes listed below are applied to the code, all of those urls see a performance boost of more than 30%.

function wp_kses_bad_protocol( $string, $allowed_protocols ) {
	$string = wp_kses_no_null( $string );

	// Short-circuit if the string starts with `https://` or `http://`. Most common cases.
	if (
		( 0 === strpos( $string, 'https://' ) && in_array( 'https', $allowed_protocols, true ) ) ||
		( 0 === strpos( $string, 'http://' ) && in_array( 'http', $allowed_protocols, true ) )
	) {
		return $string;
	}

	$iterations = 0;
	.............

This code looks solid to me, and I ran the analysis, and it performed better than the regex. The approx. The performance improvement is ~30% which is significant for standard URLs.

I post the result of strpos and str_starts_with here: https://github.com/WordPress/wordpress-develop/pull/3615#discussion_r1031073591 but for visibility, I would like to add it here also.

I ran the analysis for two functions in PHP 7.4 and 8.0 and the different is 1-2%.

  • 7.4
    • strpos ~31%
    • str_starts_with ~30%
  • PHP 8+
    • strpos ~32.5%
    • str_starts_with ~33.5%

As it improves performance now, improves performance further as more sites move to PHP 8+, it's easier to read, and Core has been changing existing instances of 0 === strpos() to str_starts_with(), I'm agreeing to use str_starts_with().

@azaozz commented on PR #3615:


15 months ago
#36

As it improves performance now, improves performance further as more sites move to PHP 8+, it's easier to read, and Core has been changing existing instances of 0 === strpos() to str_starts_with(), I'm agreeing to use str_starts_with().

Sounds good to me :)

#37 @flixos90
15 months ago

  • Keywords commit added; 2nd-opinion dev-feedback needs-testing removed
  • Owner changed from schlessera to flixos90

The approach from https://github.com/WordPress/wordpress-develop/pull/3615 looks reasonable and good to commit to me.

@markjaquith Since you originally opened this ticket (years ago), it would be great to get your feedback on that approach as well.

I'm happy to commit this, but we can let it wait for a bit more feedback first as there's still enough time until 6.2.

@flixos90 commented on PR #3615:


14 months ago
#38

Note that the function parameter was renamed in https://github.com/WordPress/wordpress-develop/commit/a9afc8e36bf669ea1b5a5cb79602aae4e0ec613a, resulting in a merge conflict. But no worries, I will fix this conflict prior to committing, which I'm going to do shortly.

#39 @flixos90
14 months ago

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

In 55053:

Formatting: Improve performance of esc_url().

This changeset indirectly improves performance of the commonly used esc_url() function by optimizing the low-level function wp_kses_bad_protocol() for the by far most common scenarios, which are URLs using either the http or https protocol.

For this common scenario, the changeset now avoids the do while loop. While for a single call to the esc_url() function the performance wins are negligible, given that esc_url() is often called many times in one page load, they can add up, making this a worthwhile improvement.

Props mukesh27, schlessera, markjaquith, azaozz, spacedmonkey.
Fixes #22951.

@flixos90 commented on PR #3598:


14 months ago
#41

Closing in favor of #3615, which was committed in https://core.trac.wordpress.org/changeset/55053.

#42 @flixos90
14 months ago

  • Keywords commit removed

#43 @milana_cap
12 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.