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

[fierevere]

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"

[fierevere]

Initially reported on forums by: @chainofchaos
​https://wordpress.org/support/topic/cant-install-uploaded-plugin-using-php-8-0

[SergeyBiryukov]

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.

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

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

[fierevere]

​https://github.com/WordPress/wordpress-develop/pull/817/commits/1ab4969bbdf3d8d173fe8ff28c515cbee019895e

[fierevere]

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

[jrf]

#52179 was marked as a duplicate.

[jrf]

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.

[fierevere]

@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

[jrf]

@fierevere Sorry for the confusion.

[audrasjb]

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

[SergeyBiryukov]

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.

[SergeyBiryukov]

#52511 was marked as a duplicate.

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

[DavidAnderson]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

Reopening for backporting to the 5.6 branch.

[SergeyBiryukov]

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.

Merged into Core in https://core.trac.wordpress.org/changeset/50355.

[pbiron]

Related: #44002

[desrosj]

#44002 was marked as a duplicate.