Make WordPress Core

Opened 3 days ago

Closed 2 days ago

#66408 closed defect (bug) (fixed)

PclZip: fatal TypeError in privExtractFileUsingTempFile() when the temporary file cannot be created (PHP 8)

Reported by: ExtremsX Owned by: SergeyBiryukov
Priority: normal Milestone: 7.2
Component: Filesystem API Version: trunk
Severity: normal Keywords: has-patch
Cc: Focuses:

Description

In wp-admin/includes/class-pclzip.php, PclZip::privExtractFileUsingTempFile() closes a variable that was never opened when it fails to create its temporary file:

<?php
$v_gzip_temp_name = PCLZIP_TEMPORARY_DIR.uniqid('pclzip-').'.gz';
if (($v_dest_file = @fopen($v_gzip_temp_name, "wb")) == 0) {
  fclose($v_file);   // $v_file is not defined in this function
  PclZip::privErrorLog(PCLZIP_ERR_WRITE_OPEN_FAIL, 'Unable to open temporary file ...');
  return PclZip::errorCode();
}

On PHP 7 this was a notice plus a warning. On PHP 8, fclose(null) throws a TypeError, so instead of returning PCLZIP_ERR_WRITE_OPEN_FAIL the request dies with a fatal error.

Steps to reproduce (PHP 8.2, trunk, standalone):

<?php
define( 'PCLZIP_TEMPORARY_DIR', '/path/that/does/not/exist/' );
require 'wp-admin/includes/class-pclzip.php';

file_put_contents( '/tmp/data.txt', str_repeat( "test line\n", 2000 ) );
$zip = new PclZip( '/tmp/test.zip' );
$zip->create( '/tmp/data.txt', PCLZIP_OPT_REMOVE_ALL_PATH );
$zip->extract( PCLZIP_OPT_PATH, '/tmp/out', PCLZIP_OPT_TEMP_FILE_ON );

Result:

Warning: Undefined variable $v_file in .../class-pclzip.php on line 3984
Fatal error: Uncaught TypeError: fclose(): Argument #1 ($stream) must be of type resource, null given

Expected: extract() returns 0 and errorInfo() reports PCLZIP_ERR_WRITE_OPEN_FAIL.

Impact is low: core's own _unzip_file_pclzip() uses PCLZIP_OPT_EXTRACT_AS_STRING, which does not go through this function. It is reachable by plugins that use the bundled PclZip to extract to disk, either with PCLZIP_OPT_TEMP_FILE_ON or with an entry above the temp-file threshold, when the temporary directory is not writable. Note that class-pclzip.php is excluded from PHPStan (tests/phpstan/base.neon), which is probably why it was not caught there.

Suggested fix: nothing has been opened at that point, so the fclose() can simply be removed:

     if (($v_dest_file = @fopen($v_gzip_temp_name, "wb")) == 0) {
-      fclose($v_file);
       PclZip::privErrorLog(PCLZIP_ERR_WRITE_OPEN_FAIL, 'Unable to open temporary file \''.$v_gzip_temp_name.'\' in binary write mode');
       return PclZip::errorCode();
     }

Found with the help of a static analysis tool I'm building (code-intruder) and AI review; reproduced manually on PHP 8.2.

Change History (3)

This ticket was mentioned in ​PR #15133 on ​WordPress/wordpress-develop by ​@ExtremsX.


3 days ago
#1

PclZip::privExtractFileUsingTempFile() calls fclose( $v_file ) when it
fails to open its temporary file, but $v_file is not defined in that
function. On PHP 8, fclose( null ) throws a TypeError, so the request
dies with a fatal error instead of extract() returning 0 with
PCLZIP_ERR_WRITE_OPEN_FAIL. Nothing has been opened at that point, so the
call is removed.

The similar fclose( $v_file ) in privAddFileUsingTempFile() is correct
(the file is opened a few lines above) and is left unchanged.

Tested on PHP 8.2 with the reproduction script from the ticket: before, a
fatal TypeError at line 3984; after, extract() returns 0 and
errorInfo() reports PCLZIP_ERR_WRITE_OPEN_FAIL.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code, with code-intruder (my own static analysis tool)
Model(s): Claude Opus
Used for: code-intruder flagged $v_file as read without being assigned
in that function. AI helped reproduce it on PHP 8.2, check that core's own
_unzip_file_pclzip() does not reach this path, and verify the fix.
I reviewed and tested the change myself.

#2 @SergeyBiryukov
2 days ago

  • Milestone Awaiting Review → 7.2

#3 @SergeyBiryukov
2 days ago

  • Owner set to SergeyBiryukov
  • Resolution → fixed
  • Status new → closed

In 65003:

Filesystem API: Avoid a fatal error in PclZip when the temporary file cannot be created.

PclZip::privExtractFileUsingTempFile() calls fclose( $v_file ) when it fails to open its temporary file, but $v_file is not defined in that function. On PHP 8, fclose( null ) throws a TypeError, so the request dies with a fatal error instead of returning an error with PCLZIP_ERR_WRITE_OPEN_FAIL. Nothing has been opened at that point, so the call is removed.

The similar fclose( $v_file ) in privAddFileUsingTempFile() is correct (the file is opened a few lines above) and is left unchanged.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/15133.

Follow-up to r11056.

Props ExtremsX.
Fixes #66408.

Note: See TracTickets for help on using tickets.