WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 4 weeks ago

#54036 closed defect (bug) (fixed)

PclZip throwing errors on PHP 8 - previously merged patch is incomplete

Reported by: DavidAnderson Owned by: SergeyBiryukov
Milestone: 5.8.1 Priority: normal
Severity: normal Version: trunk
Component: Filesystem API Keywords: php8 has-patch fixed-major
Focuses: Cc:

Description (last modified by SergeyBiryukov)

The patch merged in #52018 is incomplete, and when using PclZip::add(), you can still get fatal errors.

In UpdraftPlus (where we've had a number of affected users), the trace (from the relevant point onwards) is:

UpdraftPlus_Backup->makezip_addfiles, UpdraftPlus_PclZip->close, PclZip->add, PclZip->privAdd, PclZip->privAddFileList, PclZip->privAddFile

A patch is being attached.

Attachments (2)

pclzip.diff (549 bytes) - added by DavidAnderson 4 weeks ago.
Fix potentially invalid fread() call
pclzip.2.diff (539 bytes) - added by DavidAnderson 4 weeks ago.
Correct patch

Download all attachments as: .zip

Change History (8)

@DavidAnderson
4 weeks ago

Fix potentially invalid fread() call

#1 @SergeyBiryukov
4 weeks ago

  • Description modified (diff)
  • Keywords php8 has-patch added
  • Milestone changed from Awaiting Review to 5.9

#2 @SergeyBiryukov
4 weeks ago

Thanks for the patch!

I believe we should check $p_header['size'] instead of $p_entry['compressed_size'] though, as $p_entry is not set in this method, and the value passed to fread() is $p_header['size'].

#3 @DavidAnderson
4 weeks ago

@SergeyBiryukov Yes, sorry, that was just a copy/paste error - copied from the wrong place instead of my actual working change! New version coming up...

@DavidAnderson
4 weeks ago

Correct patch

#4 @SergeyBiryukov
4 weeks ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 51686:

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

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.

This commit also amends the previous solution for similar issues elsewhere in the file to ensure consistent type for string values, instead of changing the type from string to bool when trying to read from an empty file.

Follow-up to [50355].

Props DavidAnderson, jrf, SergeyBiryukov.
Fixes #54036.

#5 @SergeyBiryukov
4 weeks ago

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

Reopening for 5.8.1 consideration.

#6 @desrosj
4 weeks ago

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

In 51694:

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

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.

This commit also amends the previous solution for similar issues elsewhere in the file to ensure consistent type for string values, instead of changing the type from string to bool when trying to read from an empty file.

Follow-up to [50355].

Props DavidAnderson, jrf, SergeyBiryukov.
Merges [51686] to the 5.8 branch.
Fixes #54036.

Note: See TracTickets for help on using tickets.