8000 [Filesystem] Fix mirroring a directory with a relative path and a custom iterator by fxbt · Pull Request #26771 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Filesystem] Fix mirroring a directory with a relative path and a custom iterator #26771

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 10, 2018
Merged

[Filesystem] Fix mirroring a directory with a relative path and a custom iterator #26771

merged 1 commit into from
Oct 10, 2018

Conversation

fxbt
Copy link
@fxbt fxbt commented Apr 3, 2018 •
edited
Loading
Q A
Branch? 2.8 up to 4.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

The Filesystem::mirror method with a Finder iterator doesn't work if the origin directory is relative.

This is a case where it won't work:

$dir = '/data/../data/';

$finder = (new Finder())->in($dir)->getIterator();

(new Filesystem())->mirror($dir, '/tmp', $finder);

The finder will return file objects like this :

SplFileInfo {
    pathname: "/data/file.tmp"
    ...
}

But the following line will fail because because the $file->getPathname() and the $originDir are differents.

$target = $targetDir.substr($file->getPathname(), $originDirLen);

// $file->getPathname() = '/data/file.tmp'
// $originDirLen = strlen('/data/../data/') = 14
// $target = '/tmp' instead of '/tpm/file.tpm'

In some case, it's even worse. If the filename length is bigger than the $originDirLen, the target file will be a file with a completely wrong name:

// $file->getPathname() = '/data/file123456789.tmp'
// $originDirLen = strlen('/data/../data/') = 14
// $target = '/tmp/56789.tmp' instead of '/tpm/file123456789.tmp'

I fixed this on my side by using the realpath function everytime i'm calling the mirror method, but i doubt this is the desired behavior.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Apr 3, 2018
@fxbt fxbt force-pushed the Fix-relative-path branch 5 times, most recently from 5e288ac to 26d858f Compare April 9, 2018 07:20
@fxbt fxbt changed the title [Filesystem] Fix mirroring a directory with a relative path [Filesystem] Fix mirroring a directory with a relative path and a custom iterator Apr 9, 2018
@fxbt fxbt force-pushed the Fix-relative-path branch 2 times, most recently from 5147080 to 2799f3f Compare April 18, 2018 08:48
@fxbt fxbt force-pushed the Fix-relative-path branch 2 times, most recently from 835bdfc to 748e3f7 Compare April 27, 2018 09:54
@fxbt fxbt force-pushed the Fix-relative-path branch 2 times, most recently from 9f14b18 to 855d866 Compare May 14, 2018 08:31
@fabpot fabpot modified the milestones: 2.7, 2.8 May 28, 2018
@fxbt fxbt force-pushed the Fix-relative-path branch 2 times, most recently from 03d4673 to 023704e Compare May 28, 2018 08:59
@fxbt fxbt changed the base branch from 2.7 to 2.8 May 28, 2018 09:00
@fxbt fxbt force-pushed the Fix-relative-path branch from 023704e to 711a108 Compare May 28, 2018 09:03
@fabpot
Copy link
Member
fabpot commented Jul 18, 2018 •
edited
Loading

We've removed usage or realpath in the past for various reasons. I don't think reintroducing here is a good idea.

@fxbt
Copy link
Author
fxbt commented Jul 19, 2018

I wasn't aware of this, but it makes sense.

What do you think the correct approach is ? Throwing an exception to avoid mirroring a bunch of useless data ?

@fabpot
Copy link
Member
fabpot commented Jul 19, 2018

Throwing might be a good idea (calling realpath in user's code is indeed a better "fix").

@fxbt fxbt force-pushed the Fix-relative-path branch 2 times, most recently from 91bdfb7 to ad7062c Compare July 19, 2018 09:41
@fxbt
Copy link
Author
fxbt commented Jul 19, 2018

An exception is now thrown and I tried to be explicit with the error message.

The fabbot failure is a false negative.

@nicolas-grekas
Copy link
Member

(could you please rebase on latest 2.8 and fix the CS issues fabbot is reporting?)

@fxbt fxbt force-pushed the Fix-relative-path branch from ad7062c to a92a8ab Compare July 26, 2018 09:40
@fxbt
Copy link
Author
fxbt commented Jul 26, 2018

Done!

fabpot
fabpot approved these changes Oct 10, 2018
View reviewed changes
Copy link
Member
@fabpot fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm going to merge it in master.

@fabpot fabpot changed the base branch from 2.8 to master October 10, 2018 11:37
@fabpot fabpot force-pushed the Fix-relative-path branch from a92a8ab to 27b673c Compare October 10, 2018 11:37
@fabpot
Copy link
Member
fabpot commented Oct 10, 2018

Thank you @fxbt.

@fabpot fabpot merged commit 27b673c into symfony:master Oct 10, 2018
fabpot added a commit that referenced this pull request Oct 10, 2018
@fabpot
feature #26771 [Filesystem] Fix mirroring a directory with a relative…
6fbb494 
… path and a custom iterator (fxbt)

This PR was submitted for the 2.8 branch but it was squashed and merged into the 4.2-dev branch instead (closes #26771).

Discussion
----------

[Filesystem] Fix mirroring a directory with a relative path and a custom iterator

| Q             | A
| ------------- | ---
| Branch?       | 2.8 up to 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

The Filesystem::mirror method with a Finder iterator doesn't work if the origin directory is relative.

This is a case where it won't work:

```
$dir = '/data/../data/';

$finder = (new Finder())->in($dir)->getIterator();

(new Filesystem())->mirror($dir, '/tmp', $finder);
```

The finder will return file objects like this :
```
SplFileInfo {
    pathname: "/data/file.tmp"
    ...
}
```

But the following line will fail because because the `$file->getPathname()` and the `$originDir` are differents.
```
$target = $targetDir.substr($file->getPathname(), $originDirLen);

// $file->getPathname() = '/data/file.tmp'
// $originDirLen = strlen('/data/../data/') = 14
// $target = '/tmp' instead of '/tpm/file.tpm'
```

In some case, it's even worse. If the filename length is bigger than the `$originDirLen`, the target file will be a file with a completely wrong name:
```
// $file->getPathname() = '/data/file123456789.tmp'
// $originDirLen = strlen('/data/../data/') = 14
// $target = '/tmp/56789.tmp' instead of '/tpm/file123456789.tmp'
```

I fixed this on my side by using the realpath function everytime i'm calling the mirror method, but i doubt this is the desired behavior.

Commits
-------

27b673c [Filesystem] Fix mirroring a di
8000
rectory with a relative path and a custom iterator
@fxbt fxbt deleted the Fix-relative-path branch October 16, 2018 14:12
This was referenced Nov 3, 2018
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Bug Filesystem Status: Reviewed
Projects
None yet
Milestone
2.8
Development

Successfully merging this pull request may close these issues.
4 participants
@fxbt @fabpot @nicolas-grekas @carsonbot
0