Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#26598 closed defect (bug) (fixed)

Broken methods in WP_Filesystem_Direct and WP_Filesystem_SSH2

Reported by: davidanderson's profile DavidAnderson Owned by: nacin's profile 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)


#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"));'
$ php -r 'echo substr(decoct(@fileperms("/tmp/775")),3);'

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

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


$ 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);'
$ php -r 'substr(sprintf("%06d", decoct(@fileperms("/tmp/775file"))),3);'

Patch attached.

Attachments (2)

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

Download all attachments as: .zip

Change History (10)

#1 @DavidAnderson
9 years ago

  • Keywords has-patch added

#2 @ocean90
9 years ago

  • Description modified (diff)

9 years ago

Patch (replacing previous version)

#3 @DavidAnderson
9 years ago

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

#4 @SergeyBiryukov
9 years ago

  • Version changed from trunk to 2.8

Introduced in [10919] and [11063].

#5 @samuelsidler
9 years ago

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

#6 @dd32
9 years ago

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

9 years ago

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

#7 @DavidAnderson
9 years ago


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.


#8 @nacin
9 years 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.