WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11257 closed defect (bug) (fixed)

Reproducible Output Failure

Reported by: miqrogroove Owned by:
Milestone: 2.9 Priority: high
Severity: major Version: 2.8.4
Component: Shortcodes Keywords: has-patch
Focuses: Cc:

Description

Steps to reproduce:

  1. Open any browser and start a new post.
  1. Paste the attached file into HTML view.
  1. Click the Preview button.

Expected Result: Preview :)

Actual Result: Blank Post D:

Tested only on the one site so far.

Attachments (5)

wordpress-io-crash.html (7.3 KB) - added by miqrogroove 4 years ago.
This post body makes WordPress epic fail.
ticket-11257.patch (2.3 KB) - added by miqrogroove 4 years ago.
Resolves unreasonable recursion depth during wpautop. Explanation to follow.
ticket-11257-v2.patch (2.6 KB) - added by miqrogroove 4 years ago.
First version missed strip_shortcodes
wpautop-test-case.php (7.8 KB) - added by miqrogroove 4 years ago.
Test Case demonstrates failure of the last assignment to $pee in wpautop()
ticket-11257-v3.patch (2.8 KB) - added by miqrogroove 4 years ago.
Alternative patch design with changes suggested by azaozz.

Download all attachments as: .zip

Change History (32)

miqrogroove4 years ago

This post body makes WordPress epic fail.

comment:1 ninjaWR4 years ago

  • Keywords reporter-feedback added
  • Milestone changed from 2.9 to Future Release

Using r12273 and 2.7.1 (first time in a while using that site), I don't have any problems with the attached code.

In any case, moving to Future Release since 2.9 is in beta.

comment:2 scribu4 years ago

What is the URL of the page you are sent to?

If it contains post-new.php, it just means that the post hasn't been saved yet. In that case, this ticket would be invalid.

comment:3 miqrogroove4 years ago

  • Milestone changed from Future Release to 2.9

Hi

ninjaWR, I'm not sure if the 2.7.1 test would be relevant. Hopefully I will have some time to set up a proper test environment to get some more detail for you. Moving back to 2.9 because that's what betas are for. If we can't reproduce this bug before release then it has no business being assigned to another version.

scribu, the URL can't be relevant, because I was able to reproduce this on a published post as well as in preview.

This bug is blocking me from reporting some other bugs, so I will be increasing the Severity if I can pinpoint the cause/trigger.

comment:4 azaozz4 years ago

Seems to work as expected in trunk. Technically you cannot preview something that's not in the db yet so when clicking Preview for an unsaved post, it's saved as draft first and then the browser is redirected to a new tab/window to display it. So the URL your browser is redirected to may be relevant when that fails.

comment:5 miqrogroove4 years ago

  • Keywords reporter-feedback removed

Problem did not show up on a clean install, nor with the usual suspect plugins.

Debugging on the problem site so far shows that the function wpautop() is not returning a value when filters such as 'the_content' are applied.

comment:6 miqrogroove4 years ago

I traced it down to the last line of function wpautop()

$pee = preg_replace('/<p>\s*?(' . get_shortcode_regex() . ')\s*<\/p>/s', '$1', $pee);

I don't have time tonight to debug get_shortcode_regex(), but I'll keep you all posted when I do.

comment:7 dd324 years ago

Sounds like the all too common preg backtrace limit to me: #8553

comment:8 miqrogroove4 years ago

hi dd32, thanks for the heads up, but if this little post of mine triggered 100,000 recursion depth then something has gone horribly wrong. I'm going to check it out now.

comment:9 miqrogroove4 years ago

I've confirmed get_shortcode_regex is not the cause of the problem.

It's starting to look like there is a conflict between the P elements and shortcode matching.

Note the regexp is incorrect for situations like

<p>[shortcode]<p>[shortcode]<p>Hello World</p>

... which is exactly what wpautop() is emitting before the last statement. Could the syntax error be causing the recursion problem? I guess I'll have to test that theory as well.

comment:10 dd324 years ago

hi dd32, thanks for the heads up, but if this little post of mine triggered 100,000 recursion depth then something has gone horribly wrong. I'm going to check it out now.

I assume you actually checked the depth on your setup? I've seen it set incredibly small :)

If that wasnt your problem (then GREAT!.. because that tickets been open for awhile now)

The syntax could definately be causing it, causing the regex not to match and then infinitely loop.. or, remove the entire post if it matches the right(wrong) rule possibly.

comment:11 miqrogroove4 years ago

Wait a sec. Doesn't "\2" backtrace the WRONG value?

And on another note, removing some of the extraneous parentheses from get_shortcode_regex() actually solved my test case, but I'm still looking if it's dependent on post length before patching.

comment:12 miqrogroove4 years ago

I'm too sexy for my PHP [) Patch coming right up.

miqrogroove4 years ago

Resolves unreasonable recursion depth during wpautop. Explanation to follow.

comment:13 miqrogroove4 years ago

The primary change made by this patch is to remove the following code, which until now has served only to heat up server CPUs across the planet:

(?:(\/))?

It is replaced by the equivalent non-recursive form:

\\/?

This patch also fixes a related bug wherein wpautop() backreferences the wrong preg match when looking for shortcode close tags like /caption.

comment:14 miqrogroove4 years ago

  • Keywords has-patch added
  • Priority changed from normal to high
  • Severity changed from normal to critical

comment:15 miqrogroove4 years ago

  • Component changed from General to Shortcodes

miqrogroove4 years ago

First version missed strip_shortcodes

comment:16 miqrogroove4 years ago

I'm working on better steps to reproduce. There's something going on before that one line of code that's causing the bug to show up/not on my different sites.

comment:17 miqrogroove4 years ago

OK FYI, when you use my file wordpress-io-crash.html, WordPress will rewrite all of the HREF values based on the attachment ID values. This will prevent you from reproducing the bug using my code.

comment:18 scribu4 years ago

If you can prove that your patch performs better and doesn't cause any regressions, that would be good enough.

miqrogroove4 years ago

Test Case demonstrates failure of the last assignment to $pee in wpautop()

comment:19 miqrogroove4 years ago

Hi scribu, using the new Test Case, I am able to fail the test using the old regexp, and pass it using the new regexp, on two different servers. The patch is also installed and running on my 2.8.4 site with no problems.

comment:20 azaozz4 years ago

Not sure if we need to use the same regex there, no need to capture all the stuff that's needed for parsing the shortcodes.

We are only looking for <p> before and </p> after. Perhaps it would be better to build another pattern using the global $shortcode_tags, even split it in two. Wouldn't be affected by future changes to get_shortcode_regex() and will be faster.

miqrogroove4 years ago

Alternative patch design with changes suggested by azaozz.

comment:21 azaozz4 years ago

(In [12302]) Separate the removal of <p> wrapping from shortcodes into another function and apply it with different filter, props miqrogroove, props mdawaffe, see #11257, see #11249

comment:22 ryan4 years ago

  • Severity changed from critical to normal

Downgrading severity since it seems like the critical part is taken care of.

comment:23 miqrogroove4 years ago

ryan, are you sure shortcodes.php isn't going to NULL-out the content right after wpautop() finishes? I haven't tested the patch piecemeal like this.

comment:24 miqrogroove4 years ago

OK it does pass the test case without the second half of the patch. The shortcodes.php portion will just be a performance boost until someone fixes #8553

comment:25 ryan4 years ago

  • Milestone changed from 2.9 to 3.0

comment:26 miqrogroove4 years ago

  • Milestone changed from 3.0 to 2.9
  • Resolution set to fixed
  • Status changed from new to closed

Punting the performance boost part of this patch seems like a waste of time. Can we call it fixed and move on to something that's still broken?

comment:27 miqrogroove4 years ago

  • Severity changed from normal to major
Note: See TracTickets for help on using tickets.