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, let's move this ticket to 5.6.2 to give it some more time.

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

#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 that as a blocker for 5.6.2.

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

#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.