WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 10 months ago

Last modified 5 months ago

#52018 closed defect (bug) (fixed)

Zip Module 2.8.2 - class-pclzip fatal error with PHP 8.0

Reported by: fierevere Owned by: SergeyBiryukov
Milestone: 5.6.2 Priority: normal
Severity: normal Version: 5.6
Component: Filesystem API Keywords: php8 has-patch needs-unit-tests commit fixed-major
Focuses: Cc:

Description

Software fallback for unpacking ZIP archives gives fatal error with PHP 8.0

To replicate:

  1. Install WordPress 5.6 with PHP 8.0.0
  2. Disable "zip" PHP extension
  3. Upload plugin in a zip archive (Installing from dashboard will work, file upload will give fatal)

Trace:

FastCGI sent in stderr: "PHP message: PHP Fatal error:  Uncaught ValueError: fread(): Argument #2 ($length) must be greater than 0 in /sandbox/sandbox4/wp-admin/includes/class-pclzip.php:4212
Stack trace:
#0 /sandbox/sandbox4/wp-admin/includes/class-pclzip.php(4212): fread(Resource id #138, 0)
#1 /sandbox/sandbox4/wp-admin/includes/class-pclzip.php(3518): PclZip->privExtractFileAsString(Array, '', Array)
#2 /sandbox/sandbox4/wp-admin/includes/class-pclzip.php(811): PclZip->privExtractByRule(Array, './', '', false, Array)
#3 /sandbox/sandbox4/wp-admin/includes/file.php(1639): PclZip->extract(77006)
#4 /sandbox/sandbox4/wp-admin/includes/file.php(1476): _unzip_file_pclzip('/sandbox/sandbo...', '/sandbox/sandbo...', Array)
#5 /sandbox/sandbox4/wp-admin/includes/class-wp-upgrader.php(328): unzip_file('/sandbox/sandbo...', '/sandbox/sandbo...')
#6 /sandbox/sandbox4/wp-admin/includes/class-wp-upgrader.php(779): WP_Upgrader->unpack_package('/sandbox/sandbo...', false)
#7 /sandbox/sandbox4/wp-admin/includes/class-plugin-upgrader"

Attachments (2)

Change History (27)

This ticket was mentioned in Slack in #forums by yui. View the logs.


12 months ago

#3 @SergeyBiryukov
12 months ago

  • Component changed from External Libraries to Filesystem API
  • Milestone changed from Awaiting Review to 5.6.1

Thanks for the report.

Just noting that while class-pclzip.php is technically an external library, it's no longer maintained externally and is considered "adopted" in core, see comment:1:ticket:49163 for more details.

Based on the above, moving this to the "Filesystem API" component.

#4 @iandunn
12 months ago

  • Keywords php8 added

This ticket was mentioned in PR #817 on WordPress/wordpress-develop by yakimun.


12 months ago

  • Keywords has-patch added

These changes fix the fatal error when the compressed file has a size of 0.

Trac ticket: https://core.trac.wordpress.org/ticket/52018

#6 @fierevere
12 months ago

Applied the above patch
Tested again.
Seem to fix the issue.
ZIP file with plugin been successfully uploaded, unpacked and plugin installed.

#7 @jrf
11 months ago

#52179 was marked as a duplicate.

#8 follow-up: @jrf
11 months ago

  • Keywords needs-unit-tests added

Thank you @fierevere for reporting this and adding an initial patch.

I've reviewed the patch and I don't think it's ready.

  1. The privExtractFileAsString() gets passed all parameters by reference - &$p_entry, &$p_string, &$p_options -, but the parameter types are not documented.
  2. The current patch changes the parameter type for $p_string from string to bool. While this is also done on failure by the call to fread() this makes the method unstable and could cause unintended side-effects.
  3. With this in mind, this change would really need to be accompanied by unit tests, preferably quite extensive ones which also verify that changing the type of $p_string does not negatively impact other functions calling privExtractFileAsString().

As this is an external library which is no longer maintained and for which I cannot find any tests at all in the current test suite, I wonder if it may be time to look for an (external) replacement library for this functionality. While such an effort should be addressed in a separate ticket, it may influence how much effort should be put into fixing the current issue.

#9 @fierevere
11 months ago

@jrf
This patch been added by https://github.com/yakimun
i picked it from GitHub and re-added on Trac here.

I see you have already commented on GH: https://github.com/WordPress/wordpress-develop/pull/817

#10 @jrf
11 months ago

@fierevere Sorry for the confusion.

#11 @SergeyBiryukov
11 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


10 months ago

#13 @audrasjb
10 months ago

  • Milestone changed from 5.6.1 to 5.6.2

Given 5.6.1 is slated to next week, let's move this ticket to 5.6.2 to give it some more time.

Last edited 10 months ago by audrasjb (previous) (diff)

#14 @SergeyBiryukov
10 months ago

Just noting that I was able to reproduce the issue. Some notes:

  • Disable the zip PHP extension. On Windows, it's built-in since PHP 5.3, so this filter can be used instead:
    add_filter( 'unzip_file_use_ziparchive', '__return_false' );
    
  • For the fatal error to occur, the uploaded plugin archive must contain at least one empty file.

#15 @SergeyBiryukov
10 months ago

#52511 was marked as a duplicate.

#17 @DavidAnderson
10 months ago

This bug also affects plugins which end up using the bundled library (as is advised by development guidelines) to unzip - e.g. backup/restore plugins, including UpdraftPlus.

#18 in reply to: ↑ 8 @SergeyBiryukov
10 months ago

  • Keywords commit added

Replying to jrf:

The current patch changes the parameter type for $p_string from string to bool. While this is also done on failure by the call to fread() this makes the method unstable and could cause unintended side-effects.

Thanks for the review! While technically true, in my testing the patch does not make any functionality changes other than avoiding a fatal error, because, as you've noted, fread() also returns false on failure.

I think similar changes should also be applied to other similar fragments of the file, see 52018.diff.

I agree it would be great to add tests for this, but I don't see it as a blocker for 5.6.2.

Version 0, edited 10 months ago by SergeyBiryukov (next)

#19 @SergeyBiryukov
10 months ago

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

In 50355:

Filesystem API: Make sure to only call fread() on non-empty files in the PclZip library.

This avoids a fatal error on PHP 8 caused by passing a zero value to fread() as the $length argument, which must be greater than zero.

Props yakimun, fierevere, jrf, DavidAnderson, SergeyBiryukov.
Fixes #52018.

#20 @SergeyBiryukov
10 months ago

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

Reopening for backporting to the 5.6 branch.

#21 @SergeyBiryukov
10 months ago

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

In 50356:

Filesystem API: Make sure to only call fread() on non-empty files in the PclZip library.

This avoids a fatal error on PHP 8 caused by passing a zero value to fread() as the $length argument, which must be greater than zero.

Props yakimun, fierevere, jrf, DavidAnderson, SergeyBiryukov.
Merges [50355] to the 5.6 branch.
Fixes #52018.

#23 @pbiron
7 months ago

Related: #44002

This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.


7 months ago

#25 @desrosj
5 months ago

#44002 was marked as a duplicate.

Note: See TracTickets for help on using tickets.