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.

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

[Filesystem] check permissions if dump target dir is missing #24105

Merged
merged 1 commit into from Sep 7, 2017

Conversation

Projects
None yet
5 participants
@xabbuh
Member

xabbuh commented Sep 5, 2017

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #24097
License MIT
Doc PR

is_dir() returns false if the parent directory misses the executable
bit even when the directory itself is present.

@xabbuh xabbuh added this to the 2.7 milestone Sep 5, 2017

@xabbuh xabbuh changed the base branch from master to 2.7 Sep 5, 2017

check permissions if dump target dir is missing
`is_dir()` returns `false` if the parent directory misses the executable
bit even when the directory itself is present.
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 7, 2017

Member

Thank you @xabbuh.

Member

fabpot commented Sep 7, 2017

Thank you @xabbuh.

@fabpot fabpot merged commit a0f9f2c into symfony:2.7 Sep 7, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Sep 7, 2017

bug #24105 [Filesystem] check permissions if dump target dir is missi…
…ng (xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[Filesystem] check permissions if dump target dir is missing

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24097
| License       | MIT
| Doc PR        |

`is_dir()` returns `false` if the parent directory misses the executable
bit even when the directory itself is present.

Commits
-------

a0f9f2c check permissions if dump target dir is missing

@xabbuh xabbuh deleted the xabbuh:issue-24097 branch Sep 7, 2017

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Sep 8, 2017

Contributor

@xabbuh @fabpot This completely breaks liip/imagine-bundle's user-facing WebPathResolver and a large number of tests that also rely on Filesystem::dumpFile(), for example ResolveCacheTest.php.

I don't believe we're using the method incorrectly; we're passing the method a path (some of which point to paths that already exists and others that don't, sometimes with multiple levels of not-yet-created directories) along with its contents. For test failures, see this recent Travis build. You can also clone the 1.0 branch of the bundle and run the test suite. It succeeds on all version of Symfony except for 2.7 (which is the only one with this patch).

Are we doing something wrong? Or did this change break things it should not have? I'll take a closer look shortly...

Contributor

robfrawley commented Sep 8, 2017

@xabbuh @fabpot This completely breaks liip/imagine-bundle's user-facing WebPathResolver and a large number of tests that also rely on Filesystem::dumpFile(), for example ResolveCacheTest.php.

I don't believe we're using the method incorrectly; we're passing the method a path (some of which point to paths that already exists and others that don't, sometimes with multiple levels of not-yet-created directories) along with its contents. For test failures, see this recent Travis build. You can also clone the 1.0 branch of the bundle and run the test suite. It succeeds on all version of Symfony except for 2.7 (which is the only one with this patch).

Are we doing something wrong? Or did this change break things it should not have? I'll take a closer look shortly...

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 8, 2017

Member

@robfrawley Thanks for stepping up here. I think you are right and this change was not completely right. I'll check if we can fix it easily. Otherwise I suggest we revert this PR as the improved error reporting is not worth it if it breaks functionality.

Member

xabbuh commented Sep 8, 2017

@robfrawley Thanks for stepping up here. I think you are right and this change was not completely right. I'll check if we can fix it easily. Otherwise I suggest we revert this PR as the improved error reporting is not worth it if it breaks functionality.

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Sep 8, 2017

Contributor

My issue is definitely that src/Symfony/Component/Filesystem/Filesystem.php tries to enter a directory that might not exist. Something like the following fixes our usage (it works back from the provided directory path and finds the first one that exists). The problem with it is it likely nullifies the point of the new check, as it again relies on is_dir() but I just wanted to confirm the cause:

535a536
>             $exists = $dir;
537c538,542
<             if (!@chdir(dirname($exists))) {
---
>             do {
>                 $exists = dirname($exists);
>             } while (!empty($exists) && !is_dir($exists));
> 
>             if (!empty($exists) && !@chdir(dirname($exists))) {

A straight removal of the chdir-related code resolves the issues as well, of course:

535,544d534
<             $oldCwd = getcwd();
< 
<             if (!@chdir(dirname($exists))) {
<                 // When the parent directory misses the executable permission bit, we are unable to enter it and thus
<                 // cannot check if the target directory exists.
<                 throw new IOException(sprintf('Unable to detect if the target directory "%s" exists.', $dir));
<             }
< 
<             chdir($oldCwd);
<
Contributor

robfrawley commented Sep 8, 2017

My issue is definitely that src/Symfony/Component/Filesystem/Filesystem.php tries to enter a directory that might not exist. Something like the following fixes our usage (it works back from the provided directory path and finds the first one that exists). The problem with it is it likely nullifies the point of the new check, as it again relies on is_dir() but I just wanted to confirm the cause:

535a536
>             $exists = $dir;
537c538,542
<             if (!@chdir(dirname($exists))) {
---
>             do {
>                 $exists = dirname($exists);
>             } while (!empty($exists) && !is_dir($exists));
> 
>             if (!empty($exists) && !@chdir(dirname($exists))) {

A straight removal of the chdir-related code resolves the issues as well, of course:

535,544d534
<             $oldCwd = getcwd();
< 
<             if (!@chdir(dirname($exists))) {
<                 // When the parent directory misses the executable permission bit, we are unable to enter it and thus
<                 // cannot check if the target directory exists.
<                 throw new IOException(sprintf('Unable to detect if the target directory "%s" exists.', $dir));
<             }
< 
<             chdir($oldCwd);
<
@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Sep 9, 2017

Contributor

@xabbuh @fabpot Can we please revert this PR in the immediate until another solution is found? A working Filesystem::dumpFile() method is much more important than the goal of this PR (better error handling), and such can be followed up later in a subsequent PR.

The bugfix release 1.9.1 of liip/imagine-bundle needs to be released ASAP, but (as this breaks our test suite badly) I don't feel great about tagging a release the is associated with a failing Travis build (nor would I like for our main README's build badge to show failing once the release is tagged). I'd also prefer not to remove Symfony 2.7 from our Travis build matrix, even if temporarily. :-)

For now: please revert.

Contributor

robfrawley commented Sep 9, 2017

@xabbuh @fabpot Can we please revert this PR in the immediate until another solution is found? A working Filesystem::dumpFile() method is much more important than the goal of this PR (better error handling), and such can be followed up later in a subsequent PR.

The bugfix release 1.9.1 of liip/imagine-bundle needs to be released ASAP, but (as this breaks our test suite badly) I don't feel great about tagging a release the is associated with a failing Travis build (nor would I like for our main README's build badge to show failing once the release is tagged). I'd also prefer not to remove Symfony 2.7 from our Travis build matrix, even if temporarily. :-)

For now: please revert.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 9, 2017

Member

@robfrawley reverted

Member

fabpot commented Sep 9, 2017

@robfrawley reverted

fabpot added a commit that referenced this pull request Sep 9, 2017

Revert "bug #24105 [Filesystem] check permissions if dump target dir …
…is missing (xabbuh)"

This reverts commit d74144f, reversing
changes made to 2b79f48.

fabpot added a commit that referenced this pull request Sep 9, 2017

Merge branch '2.7' into 2.8
* 2.7:
  Revert "bug #24105 [Filesystem] check permissions if dump target dir is missing (xabbuh)"
  [Filesystem] skip tests if not applicable
  [Fabbot] Do not run php-cs-fixer if there are no change in src/
  [Security] Fix exception when use_referer option is true and referer is not set or empty
  Get KERNEL_DIR through $_ENV too for KernelTestCase
  check permissions if dump target dir is missing
@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Sep 9, 2017

Contributor

Wonderful; thanks for the quick response!

Contributor

robfrawley commented Sep 9, 2017

Wonderful; thanks for the quick response!

nicolas-grekas added a commit that referenced this pull request Sep 11, 2017

Merge branch '2.8' into 3.3
* 2.8:
  Revert "bug #24105 [Filesystem] check permissions if dump target dir is missing (xabbuh)"
  [Filesystem] skip tests if not applicable
  [Fabbot] Do not run php-cs-fixer if there are no change in src/
  [Security] Fix exception when use_referer option is true and referer is not set or empty
  Get KERNEL_DIR through $_ENV too for KernelTestCase
  check permissions if dump target dir is missing

nicolas-grekas added a commit that referenced this pull request Sep 11, 2017

Merge branch '3.3' into 3.4
* 3.3:
  Revert "bug #24105 [Filesystem] check permissions if dump target dir is missing (xabbuh)"
  [Filesystem] skip tests if not applicable
  [Fabbot] Do not run php-cs-fixer if there are no change in src/
  [Security] Fix exception when use_referer option is true and referer is not set or empty
  [HttpKernel] "controller.service_arguments" services should be public
  Get KERNEL_DIR through $_ENV too for KernelTestCase
  Get KERNEL_CLASS through $_ENV too
  check permissions if dump target dir is missing

nicolas-grekas added a commit that referenced this pull request Sep 11, 2017

Merge branch '3.4'
* 3.4:
  [SecurityBundle] Fix valid provider considered undefined
  Revert "bug #24105 [Filesystem] check permissions if dump target dir is missing (xabbuh)"
  [Filesystem] skip tests if not applicable
  [Fabbot] Do not run php-cs-fixer if there are no change in src/
  [ExpressionLanguage] make a proposal in SyntaxError message
  [Security] Fix exception when use_referer option is true and referer is not set or empty
  [HttpKernel] "controller.service_arguments" services should be public
  Get KERNEL_DIR through $_ENV too for KernelTestCase
  Get KERNEL_CLASS through $_ENV too
  check permissions if dump target dir is missing
@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 11, 2017

Member

@fabpot thanks 👍

@robfrawley I have some code locally that would probably fix the issue that you experienced. Though I don't think it's worth it as the error we tried to detect here is an extreme edge case IMO. However, we have #24134 which may provide some other improved error handling. Would you mind testing that PR with your code base?

Member

xabbuh commented Sep 11, 2017

@fabpot thanks 👍

@robfrawley I have some code locally that would probably fix the issue that you experienced. Though I don't think it's worth it as the error we tried to detect here is an extreme edge case IMO. However, we have #24134 which may provide some other improved error handling. Would you mind testing that PR with your code base?

This was referenced Oct 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment