Make WordPress Core

Opened 6 years ago

Closed 5 months ago

Last modified 5 months ago

#41855 closed defect (bug) (fixed)

copy_dir fails in coping some files in nested dirs

Reported by: caraffande's profile caraffande Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.3 Priority: normal
Severity: normal Version: 4.8.1
Component: Filesystem API Keywords: has-patch has-unit-tests has-testing-info commit
Focuses: Cc:

Description

Tricky, subtle bug!
Let's suppose we do have a folder structure like this:

/folder1/folder2/
/folder1/folder2/file1.txt
/folder1/folder2/subfolder1/
/folder1/folder2/subfolder1/file2.txt
/folder3/

and that we want to move or copy the entire folder2 to folder3 so that the final structure is like this:

/folder1/folder2/
/folder1/folder2/file1.txt
/folder1/folder2/subfolder1/
/folder1/folder2/subfolder1/file1.txt
/folder3/folder2/
/folder3/folder2/file1.txt
/folder3/folder2/subfolder1/
/folder3/folder2/subfolder1/file1.txt

IMPORTANT: Please note that initially the path /folder3/folder2 doesn't exist.
The problem arise based on the order used internally by copy_dir to retieve the source elements.
Let's see it in detail.

command:

copy_dir('/folder1/folder2', '/folder3/folder2')

Result:

First iteration: /folder3/folder2 is created with mkdir.
Second iteration (if copy_dir retrieves first /folder1/folder2/file1.txt): copy(/folder1/folder2/file1.txt, /folder3/folder2/file1.txt) fails because /folder3/folder2 doesn't exist!
Second iteration (if copydir retrieves first /folder1/folder2): /folder3/folder2 is created. Now, by the next iteration, copy(/folder1/folder2/file1.txt, /folder3/folder2/file1.txt) will not fail.

I've discover this bug almost accidentally using the following (buggy) code:

<?php
    function copy_recursive($source, $dest, $merge = true)
    {
        if (is_file($dest) && is_dir($source)) {
            error_log(__("[PPC] - Error: Cannot copy a dir into a file."));
            return;
        }

        if (!$merge) delete_recursive($dest);

        if (is_dir($source)) {
            $iterator = new RecursiveIteratorIterator(
                new RecursiveDirectoryIterator($source, RecursiveDirectoryIterator::SKIP_DOTS),
                RecursiveIteratorIterator::SELF_FIRST
            );

            foreach ($iterator as $file) {
                if ($file->isDir()) {
                    mkdir($dest.DIRECTORY_SEPARATOR.$iterator->getSubPathName(), 0777, true);
                } else {
                    copy($file, $dest.DIRECTORY_SEPARATOR.$iterator->getSubPathName());
                }
            }
        } else {
            copy($source, $dest);
        }
    }

I need a copy_dir (or copy_recursive) function for copying my custom plugin's data (a complex nested structure of file/folders) to wordpress' upload dir. The above code used to fail, apparently randomly, the copy of only some files.
Then I've tried to use wordpress' copy_dir function only to discover, with disappointment, that it used to fail exactly the same way.

The solution, at least for my (buggy) code, is to create the destination folder before any copy command, like this is:

<?php
    function copy_recursive($source, $dest, $merge = true)
    {
        if (is_file($dest) && is_dir($source)) {
            error_log(__("[PPC] - Error: Cannot copy a dir into a file."));
            return;
        }

        if (!$merge) delete_recursive($dest);

        if (!file_exists($dest)) mkdir($dest, 0777, true); // <<<---- Bug Fix!

        if (is_dir($source)) {
            $iterator = new RecursiveIteratorIterator(
                new RecursiveDirectoryIterator($source, RecursiveDirectoryIterator::SKIP_DOTS),
                RecursiveIteratorIterator::SELF_FIRST
            );

            foreach ($iterator as $file) {
                if ($file->isDir()) {
                    mkdir($dest.DIRECTORY_SEPARATOR.$iterator->getSubPathName(), 0777, true);
                } else {
                    copy($file, $dest.DIRECTORY_SEPARATOR.$iterator->getSubPathName());
                }
            }
        } else {
            copy($source, $dest);
        }
    }



Attachments (5)

test-41855.zip (4.5 KB) - added by costdev 6 months ago.
Plugin for issue reproduction and patch testing.
Before Applying Patch 4221.png (113.7 KB) - added by zunaid321 6 months ago.
Before Applying Patch #4221
Before Applying Patch - Files Shouldn't Exist #4221.png (4.6 KB) - added by zunaid321 6 months ago.
Before Applying Patch - Files Shouldn't Exist #4221
After Applying Patch 4221.png (76.2 KB) - added by zunaid321 6 months ago.
After Applying Patch 4221
Patch Worked #4221.png (8.8 KB) - added by zunaid321 6 months ago.
Patch Worked 4221

Download all attachments as: .zip

Change History (15)

This ticket was mentioned in PR #4221 on WordPress/wordpress-develop by @costdev.


9 months ago
#1

  • Keywords has-patch has-unit-tests added

This creates the destination directory in copy_dir() if it does not exist.

Trac ticket: https://core.trac.wordpress.org/ticket/41855

#2 @costdev
9 months ago

  • Milestone changed from Awaiting Review to 6.3

PR 4221 creates the destination directory if it does not exist. Includes a PHPUnit test which demonstrates the issue, proves the fix is successful, and protects backward compatibility going forward.

Milestoning for 6.3.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


6 months ago

@costdev
6 months ago

Plugin for issue reproduction and patch testing.

#4 @costdev
6 months ago

  • Keywords has-testing-info added

Testing Instructions

These steps define how to reproduce the issue, and indicate the expected behavior.

Steps to Reproduce or Test

  1. Download the attached testing plugin.
  2. Navigate to Plugins > Add New.
  3. Click Upload Plugin, select test-41855.zip and click Install Now.
  4. 🐞 Once installed, click Activate.
  5. Apply the patch.
  6. ✅ Refresh the page.

Expected Results

When reproducing a bug:

  • ❌ An admin notice should appear stating Could not create directory..
  • wp-content/plugins/test-41855/test-result/test-directory should not exist.

When testing a patch to validate it works as expected:

  • ✅ An admin notice should appear stating Success! .
  • wp-content/plugins/test-41855/test-result/test-directory should exist with the same contents as wp-content/plugins/test-41855/test-directory.

Test Report Icons:
🐞 <= Indicates where issue ("bug") occurs.
✅ <= Behavior is expected.
❌ <= Behavior is NOT expected.

Last edited 6 months ago by costdev (previous) (diff)

#5 @zunaid321
6 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/4221.diff

Environment

  • OS: Windows 11 (22H2)
  • Web Server: nginx/1.23.4
  • PHP: 7.4.33
  • WordPress: 6.3-alpha-55505-src
  • Browser: Chrome Version 114.0.5735.110 (Official Build) (64-bit)
  • Theme: Twenty Twenty-Three

Before Applying The Patch

  • Followed the instructions of @costdev
  • ❌ Error message does show up after activating the plugin
  • ❌ This folder exists: wp-content/plugins/test-41855/test-result/test-directory

After Applying The Patch

  • Followed the same instructions
  • ✅ The admin notice: Success! does show up
  • ✅ wp-content/plugins/test-41855/test-result/test-directory exists with the same contents as wp-content/plugins/test-41855/test-directory.

Plugins Used

  • Attached: test-41855.zip

@zunaid321
6 months ago

Before Applying Patch #4221

@zunaid321
6 months ago

Before Applying Patch - Files Shouldn't Exist #4221

@zunaid321
6 months ago

After Applying Patch 4221

@zunaid321
6 months ago

Patch Worked 4221

#6 @costdev
6 months ago

  • Keywords commit added

Thanks @zunaid321! Adding for commit consideration.

#7 @peterwilsoncc
6 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4221

Environment

  • Filesystem: Direct
  • OS: Linux 5.15.0-30-generic x86_64
  • Web Server: nginx/1.18.0
  • PHP: 7.4.33
  • WordPress: 6.3-alpha-55505-src
  • Browser: Safari 15.4
  • Theme: Twenty Twenty-Three
  • Active Plugins: None

Testing procedure

  1. Create directory wp-content/empty
  2. Run following in wp shell
    WP_Filesystem();
    copy_dir( '/vagrant/content/plugins', '/vagrant/content/empty/plugins' );
    

Actual Results

  • ✅ Verified report: First block of code fails
  • ✅ Verified patch: First block of code succeeds with PR checked out.

Additional Notes

I also ran the following test attempting to copy the folder in to a sub-directory of a directory that doesn't exist:

  1. Empty directory wp-content/empty
  2. Run following in wp shell
    WP_Filesystem();
    copy_dir( '/vagrant/content/plugins', '/vagrant/content/empty/sub-folder/plugins' );
    

With the PR applied, this fails as the call to the PHP function mkdir() doesn't use the recursive function. Honestly, I think this is fine.

#8 @peterwilsoncc
6 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4221

Environment

  • Filesystem: ftpext
  • OS: Linux 5.15.0-30-generic x86_64
  • Web Server: nginx/1.18.0
  • PHP: 7.4.33
  • WordPress: 6.3-alpha-55505-src
  • Browser: Safari 15.4
  • Theme: Twenty Twenty-Three
  • Active Plugins: None

Testing procedure

  1. Add a one line mu-plugin to force FTP on WP CLI: add_filter( 'filesystem_method', function() { return 'ftpext'; }, 100 );
  2. Create directory wp-content/empty
  3. Run following in wp shell
    WP_Filesystem([ 'hostname' => 'vagrant.local', 'username' => 'vagrant', 'password' => '********' ]);
    copy_dir( '/vagrant/content/plugins', '/vagrant/content/empty/plugins' );
    

Actual Results

  • ✅ Verified report: Test above fails
  • ✅ Verified patch: Test passes

#9 @peterwilsoncc
5 months ago

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

In 55938:

Filesystem API: Attempt to create directory in copy_dir().

Adds a check to the start of copy_dir() that the destination directory exists and attempts to create it if it does not.

An error is returned if the directory can not be created, either due to a permissions error or the parent directory not existing.

Props caraffande, costdev, zunaid321.
Fixes #41855.

Note: See TracTickets for help on using tickets.