WP_Image_Editor::make_image leaves an output buffer open on failure

[gnif]

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;
  }

[DrewAPicture]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

Reopening for 4.9.5 consideration.

[gnif]

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();

[SergeyBiryukov]

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!

[SergeyBiryukov]

In 42702:

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

Props gnif.
See #43255.

[kirasong]

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?

[kirasong]

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.

[gnif]

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.

[gnif]

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.

[SergeyBiryukov]

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.

[kirasong]

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

[joemcgill]

@mikeschroder agreed to handle the backport to 4.9.9.

[kirasong]

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.

[pento]

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

[SergeyBiryukov]

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.

[SergeyBiryukov]

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

[SergeyBiryukov]

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