WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 6 months ago

#43255 closed defect (bug) (fixed)

WP_Image_Editor::make_image leaves an output buffer open on failure

Reported by: gnif Owned by: mikeschroder
Milestone: 4.9.9 Priority: normal
Severity: critical Version: 3.5
Component: Media Keywords: has-patch fixed-major fixed-5.0
Focuses: Cc:

Description

WP_Image_Editor::make_image is failing to clean up on failure leaving an output buffer open.

If the call to fopen fails the function returns without calling 'ob_end_clean()'

  protected function make_image( $filename, $function, $arguments ) {
    if ( $stream = wp_is_stream( $filename ) ) {
      ob_start();
    } else {
      // The directory containing the original file may no longer exist when using a replication plugin.
      wp_mkdir_p( dirname( $filename ) );
    }

    $result = call_user_func_array( $function, $arguments );

    if ( $result && $stream ) {
      $contents = ob_get_contents();

      $fp = fopen( $filename, 'w' );

      if ( ! $fp )
      {
        ob_end_clean(); // <-- This is needed
        return false;
      }

      fwrite( $fp, $contents );
      fclose( $fp );
    }

    if ( $stream ) {
      ob_end_clean();
    }

    return $result;
  }

Attachments (1)

43255.diff (365 bytes) - added by dhanendran 17 months ago.

Download all attachments as: .zip

Change History (28)

#1 @SergeyBiryukov
17 months ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 4.9.5

@dhanendran
17 months ago

#2 @DrewAPicture
17 months ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to dhanendran
  • Status changed from new to assigned

Hi @dhanendran, thanks for the patch! I'm going to mark this good-first-bug as claimed to ensure that you get timely feedback on it :-)

If for whatever reason a few days go by without hearing from anyone, I suggest contacting anyone on the the list of maintainers for the Media component.

#3 @SergeyBiryukov
17 months ago

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

In 42695:

Media: In WP_Image_Editor::make_image(), close previously opened output buffer if the file could not be created.

Props dhanendran, gnif.
Fixes #43255.

#4 @SergeyBiryukov
17 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.9.5 consideration.

#5 follow-up: @gnif
17 months ago

The patch committed is incorrect, it is uselessly checking again if $stream is set when it was already checked on line 406

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-image-editor.php?rev=42695#L406

406                if ( $result && $stream ) {
407	                        $contents = ob_get_contents();

#6 in reply to: ↑ 5 @SergeyBiryukov
17 months ago

Replying to gnif:

The patch committed is incorrect, it is uselessly checking again if $stream is set when it was already checked

Ah, I missed the earlier check. Thanks for catching that!

#7 @SergeyBiryukov
17 months ago

In 42702:

Media: After [42695], remove redundant check that is already performed a few lines above.

Props gnif.
See #43255.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


16 months ago

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


16 months ago

#10 @mikeschroder
16 months ago

In the media meeting, we were noticing that this is still open for 4.9.5.

Changing the way output buffering works in a minor release makes me a bit nervous.

What are the potential side effects if folks were working around this behaviour, and it changes?

This ticket was mentioned in Slack in #core-media by mike. View the logs.


16 months ago

This ticket was mentioned in Slack in #core-media by mike. View the logs.


16 months ago

#13 @mikeschroder
16 months ago

  • Keywords good-first-bug has-patch fixed-major removed
  • Milestone changed from 4.9.5 to 5.0
  • Resolution set to fixed
  • Status changed from reopened to closed

To have this be a safer change, going to assign to 5.0 and close as fixed.

If this is causing significant platform problems for anyone, or there are arguments in support of safety of shipping this in a minor, please feel free to comment or re-open, and can definitely reconsider.

#14 @gnif
16 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I have a large client that encountered this bug, it caused the home page content to be replaced with binary image data. I'd say that's a pretty major problem.

It can be triggered by any module that tries to download content from a remote server and fails, in our case it was an image from Flickr (https://farm5.staticflickr.com/4751/39430293454_286b82ba56_b-400x250.jpg) that is no longer valid. Due to this, 200K+ of binary data was dumped into the output of every page on the website.

This could trigger bugs in code that post-processes the output, such as shortcodes, and potentially be leveraged to execute arbitrary code.

Either way there is no way to "work around" this bug, it is unpredictable and is triggered by a failure of a remote service.

Last edited 16 months ago by gnif (previous) (diff)

#15 @SergeyBiryukov
16 months ago

  • Keywords has-patch fixed-major added

#16 @gnif
11 months ago

  • Severity changed from normal to critical

Please patch the latest version of Wordpress with this and remove the 5.0 milestone! My client yet again had a down website due to this bug after upgrading wordpress.

#17 @SergeyBiryukov
11 months ago

  • Milestone changed from 5.0 to 4.9.9

4.9.8 is already at RC stage, moving to 4.9.9.

[42695] and [42702] would need a backport to the 4.9 branch.

#18 @mikeschroder
11 months ago

Given lack of feedback otherwise, and since it's affecting sites, I think it makes sense to go ahead and backport this.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


10 months ago

#21 @joemcgill
10 months ago

  • Owner changed from dhanendran to mikeschroder
  • Status changed from reopened to assigned

@mikeschroder agreed to handle the backport to 4.9.9.

#22 @mikeschroder
9 months ago

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

In 43649:

Media: In WP_Image_Editor::make_image(), close previously opened output buffer if the file could not be created.

In addition to the merge noted below, includes important brackets added in [42343].

Props dhanendran, gnif, sergey.
Merges [42695] and [42702] to the 4.9 branch.
Fixes #43255.

#23 @pento
9 months ago

  • Keywords fixed-major removed
  • Milestone changed from 4.9.9 to 5.0
  • Resolution fixed deleted
  • Status changed from closed to reopened

[43649] can be ported to 5.0. After 5.0, it needs to be reverted from the 4.9 branch (add fixed-major after the port, and leave the issue open).

#24 @SergeyBiryukov
9 months ago

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

In 43717:

Media: In WP_Image_Editor::make_image(), close previously opened output buffer if the file could not be created.

In addition to the merge noted below, includes important brackets added in [42343].

Props dhanendran, gnif, SergeyBiryukov.
Merges [42695] and [42702] to the 5.0 branch.
Fixes #43255.

#25 @SergeyBiryukov
9 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for a revert from the 4.9 branch per comment:23.

#26 @pento
9 months ago

  • Keywords fixed-5.0 added

#27 @SergeyBiryukov
6 months ago

  • Milestone changed from 5.0 to 4.9.9
  • Resolution set to fixed
  • Status changed from reopened to closed

4.9.9 shipped with [43649], I think it's too late to revert at this point :)

Note: See TracTickets for help on using tickets.