WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 5 weeks ago

#26598 closed defect (bug) (fixed)

Broken methods in WP_Filesystem_Direct and WP_Filesystem_SSH2

Reported by: DavidAnderson Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: normal Version: 2.8
Component: Filesystem API Keywords: has-patch needs-testing
Focuses: Cc:

Description (last modified by ocean90)

Hi,

#25741 mentions some broken methods in WP_Filesystem classes, but here are two different broken methods, for different reasons:

getchmod() in both WP_Filesystem_Direct and WP_Filesystem_SSH2 uses substr(, 3) on the results of decoct().

e.g. in WP_Filesystem_Direct:

return substr(decoct(@fileperms($file)),3);

That's wrong if $file was a directory; in the directory case, substr(, 3) results in the first character being dropped:

$ mkdir /tmp/775
$ chmod 775 /tmp/775
$ ls -ld /tmp/775
$ ls -ld /tmp/775
drwxrwxr-x. 2 david david 4096 Dec 13 10:44 /tmp/775
$ php -r 'echo decoct(@fileperms("/tmp/775"));'
40775
$ php -r 'echo substr(decoct(@fileperms("/tmp/775")),3);'
75

The correct result would be obtained for both directories and files with the help of a sprintf:

substr(sprintf("%06d", decoct(@fileperms($file))),3);

e.g.:

$ mkdir /tmp/775dir
$ touch /tmp/775file
$ chmod 775 /tmp/775dir /tmp/775file
$ ls -ld /tmp/775 /tmp/775file
drwxrwxr-x. 2 david david 4096 Dec 13 10:44 /tmp/775
-rwxrwxr-x. 1 david david    0 Dec 13 10:49 /tmp/775file
$ php -r 'echo substr(sprintf("%06d", decoct(@fileperms("/tmp/775dir"))),3);'
775
$ php -r 'substr(sprintf("%06d", decoct(@fileperms("/tmp/775file"))),3);'
775

Patch attached.

Attachments (2)

getchmod.diff (1.1 KB) - added by DavidAnderson 4 months ago.
Patch (replacing previous version)
getchmod.2.diff (1.1 KB) - added by DavidAnderson 8 weeks ago.
New patch using substr(, -3) instead of sprintf

Download all attachments as: .zip

Change History (10)

comment:1 DavidAnderson4 months ago

  • Keywords has-patch added

comment:2 ocean904 months ago

  • Description modified (diff)

DavidAnderson4 months ago

Patch (replacing previous version)

comment:3 DavidAnderson4 months ago

Note that gethchmod() is also broken when called on a directory as a knock-on effect of this bug, as it calls getchmod().

comment:4 SergeyBiryukov4 months ago

  • Version changed from trunk to 2.8

Introduced in [10919] and [11063].

comment:5 samuelsidler8 weeks ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 3.9

comment:6 dd328 weeks ago

Instead of using sprintf could we simply use negative positions with substr? ie, substr( $data, -3 ) to pull the last 3 characters?

DavidAnderson8 weeks ago

New patch using substr(, -3) instead of sprintf

comment:7 DavidAnderson8 weeks ago

Hi,

I've attached an updated patch.

It should be entirely equivalent, as I think there's no way for the results of dec2oct to exceed 6 characters. But, if perhaps there is, then using substr(, -3) instead will have it covered.

David

comment:8 nacin5 weeks ago

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

In 27566:

Filesystem: Fix getchmod() for direct and ssh2 transports, for directories.

props DavidAnderson.
fixes #26598.

Note: See TracTickets for help on using tickets.