WordPress.org

Make WordPress Core

Opened 2 weeks ago

Last modified 42 hours ago

#43255 reopened defect (bug)

WP_Image_Editor::make_image leaves an output buffer open on failure

Reported by: gnif Owned by: dhanendran
Milestone: 4.9.5 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: good-first-bug 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 2 weeks ago.

Download all attachments as: .zip

Change History (9)

#1 @SergeyBiryukov
2 weeks ago

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

@dhanendran
2 weeks ago

#2 @DrewAPicture
2 weeks 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
13 days 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
13 days ago

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

Reopening for 4.9.5 consideration.

#5 follow-up: @gnif
13 days 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
13 days 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
13 days 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.


42 hours ago

Note: See TracTickets for help on using tickets.