WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 3 weeks ago

#52018 reviewing defect (bug)

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

Reported by: fierevere Owned by: SergeyBiryukov
Milestone: 5.6.1 Priority: normal
Severity: normal Version: 5.6
Component: Filesystem API Keywords: php8 has-patch needs-unit-tests
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"

Change History (12)

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


5 weeks ago

#3 @SergeyBiryukov
5 weeks 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
5 weeks ago

  • Keywords php8 added

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


5 weeks 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
5 weeks 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
3 weeks ago

#52179 was marked as a duplicate.

#8 @jrf
3 weeks 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
3 weeks 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
3 weeks ago

@fierevere Sorry for the confusion.

#11 @SergeyBiryukov
3 weeks ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing
Note: See TracTickets for help on using tickets.