WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#25237 closed defect (bug) (fixed)

WP_Filesystem fails to verify files were written to disk correctly

Reported by: dd32 Owned by: dd32
Milestone: 3.7 Priority: normal
Severity: normal Version: 2.5
Component: Filesystem API Keywords:
Focuses: Cc:

Description

Following on from #22881 and #16057

WP_Filesystem fails to verify that files were written to disk correctly, this happens when disk space is exhausted, a quota is hit, or a disk error occurs.

Attachments (1)

25237.diff (5.1 KB) - added by dd32 6 years ago.

Download all attachments as: .zip

Change History (6)

#1 follow-up: @dd32
6 years ago

Noting it here, the Filesystem classes will have issues with mbstring.func_overload overloading strlen() after this change. A follow up change will be required to cover that case.

#2 @dd32
6 years ago

  • Keywords has-patch needs-testing commit added

Attachment attachment:25237.diff​ added

Committable patch, just attaching here for review for now until proper testing is done.

All - The return results of fwrite()'s were not being checked, as a result, it was possible for it to read/write half a file and return that as a successful read/write. This results in partially written files.

FTP Sockets - Forcing all writes and reads to FTP_BINARY, when using FTP_ASCII the FTP class in use will auto-convert line endings to \n or \r\n on linux/windows servers, this will cause file verification failures in future.

FTP / SSH - These supported 2 extra args which the Direct class don't, $type (ASCII or BINARY), and $resumepos (which wasn't always respected). Neither were respected by SSH2. These are removed in the patch since Direct doesn't support them, and they haven't been able to be relied upon. Any plugin using the $resumepos parameter would have only worked with the FTP Extension, and the $type variable removal will not negatively affect any plugin at all.

Last edited 6 years ago by dd32 (previous) (diff)

@dd32
6 years ago

#3 @dd32
6 years ago

In 25304:

WP_Filesystem: Ensure that all files are read/written correctly by verifying the return values from fwrite() and using FTP_BINARY mode (ASCII converts line endings as per the spec). See #25237

#4 in reply to: ↑ 1 @dd32
6 years ago

  • Keywords has-patch needs-testing commit removed

Replying to dd32:

Noting it here, the Filesystem classes will have issues with mbstring.func_overload overloading strlen() after this change. A follow up change will be required to cover that case.

See #25259, Leaving this ticket open for implementation of that, or, the workaround added to the get_contents() / put_contents() methods here.

#5 @dd32
6 years ago

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

In 25349:

Make use of the mbstring.func_overload helper functions in WP_Filesystem so byte lengths are properly determined. See #25259 Fixes #25237

Note: See TracTickets for help on using tickets.