WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 2 months ago

#43255 reopened defect (bug)

WP_Image_Editor::make_image leaves an output buffer open on failure

Reported by: gnif Owned by: dhanendran
Milestone: 5.0 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch fixed-major
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 4 months ago.

Download all attachments as: .zip

Change History (16)

#1 @SergeyBiryukov
4 months ago

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

@dhanendran
4 months ago

#2 @DrewAPicture
4 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
3 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
3 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
3 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
3 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
3 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.


3 months ago

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


3 months ago

#10 @mikeschroder
3 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.


3 months ago

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


2 months ago

#13 @mikeschroder
2 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
2 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 2 months ago by gnif (previous) (diff)

#15 @SergeyBiryukov
2 months ago

  • Keywords has-patch fixed-major added
Note: See TracTickets for help on using tickets.