Fix mkdir race condition in FileSystemCacheHandler #375

B-Galati · 2019-10-14T16:33:12Z

Changes

From time I am having errors such as Warning: mkdir(): File exists coming from FileSystemCacheHandler.

Not a big a deal but it's creating false positive alerts.

References

https://github.com/kalessil/phpinspectionsea/blob/master/docs/probable-bugs.md#mkdir-race-condition

Testing

Quite hard to test but it's a known issue. Most framework for example are using that approach.

This change adds test coverage
This change has been tested on the latest version of PHP

Checklist

I have read the Auth0 general contribution guidelines.
I have read the Auth0 Code of Conduct.
All existing and new tests complete without errors.
The correct base branch is being used.

https://github.com/kalessil/phpinspectionsea/blob/master/docs/probable-bugs.md#mkdir-race-condition

joshcanhelp · 2019-10-15T00:29:44Z

@B-Galati - I appreciate the contribution here!

We're actually working on a major release for this library and one part of that will be re-implementing the caching that we use in either PSR-6 (#282) or PSR-16. This will mean that this cache driver will go away.

I'll close this for now but if we get more feedback that this would be good to have ahead of that release, we'll consider a patch release for it.

Thanks again!

B-Galati · 2019-10-15T04:43:58Z

@jose-e-rodriguez thanks.

Would you know when do you plan to release this new major? I cannot see any active development on the repo for a new major :/

Preparing a new major release does not mean the current or previous one should not be fixed, that's a pitty.

joshcanhelp · 2019-10-15T13:14:27Z

We don't have any concrete dates but we just merged our first PR towards that here:

https://github.com/auth0/auth0-PHP/tree/7.0.0-dev

I would guess in the next month or so.

Preparing a new major release does not mean the current or previous one should not be fixed

I totally agree, we'll provide support for the 5.x.x version for a while. Your PR says that that isn't a big deal so I figured it would be fine to close it with an explanation.

Is this something you need in the SDK within the next month or so?

B-Galati · 2019-10-15T15:38:14Z

@joshcanhelp Thanks again for these info!

Your PR says that that isn't a big deal so I figured it would be fine to close it with an explanation.

You are right. I meant that it's not a big deal in the sense of which it's a minor bug that should not prevent an application from working as expected. IMHO it worth it to release it within a month.

Is this something you need in the SDK within the next month or so?

Nothing urgent but I would love to have that fix whenever it's possible to prevent Sentry reminding me this bug from time to time.

Another thing is that I don't know how hard it will be to upgrade to the new major and I don't think I will be able to prioritize a migration as soon as the new major is released.

joshcanhelp · 2019-10-15T16:31:36Z

Understood and thanks for the additional clarification. I'll re-open this and take a look this week.

joshcanhelp

A fix and a question.

Also ... it does not seem like this solves your issue. Why would you be getting a File exists error for a mkdir() call right after you check for its existence?

src/Helpers/Cache/FileSystemCacheHandler.php

joshcanhelp · 2019-10-16T22:40:33Z

src/Helpers/Cache/FileSystemCacheHandler.php

-        if (! file_exists($this->tmp_dir)) {
-            mkdir($this->tmp_dir);
+        if (! is_dir($this->tmp_dir) && ! @mkdir($this->tmp_dir, 0777, true) && ! is_dir($this->tmp_dir) ) {
+            throw new \RuntimeException("Cache Handler was not able to create directory '$this->tmp_dir'");


Are we sure we want to throw an exception here? Seems like we shouldn't fail the application if cache isn't running. Also, if we're increasing from a warning/notice that can be hidden/logged to a potential exception, that has the potential to break folks on a patch/minor upgrade.

Yes, IMHO it sounds like a big problem to have a failing cache system and not being aware that it does not work.

Also, if we're increasing from a warning/notice that can be hidden/logged to a potential exception, that has the potential to break folks on a patch/minor upgrade.

At the moment the set method of the cache handler would fail if the tmp directory does not exist so it should not change anything for the next version. On the other hand, with this PR it would not create false positive warning for a non existent directory.

At the moment the set method of the cache handler would fail

It would fail but it would be a warning, not a fatal or exception:

https://www.php.net/manual/en/function.fopen.php

I agree that a failing cache system is a problem and, in this case, since it's not something that runs on every load, it's probably best to stop the process. But we can't introduce an exception here in a minor or patch release.

At the moment the set method of the cache handler would fail

It would fail but it would be a warning, not a fatal or exception:

https://www.php.net/manual/en/function.fopen.php

Indeed! Thank you :-)

I agree that a failing cache system is a problem and, in this case, since it's not something that runs on every load, it's probably best to stop the process. But we can't introduce an exception here in a minor or patch release.

That is not a documented and should be catched exception. That is only a safe guard in case something is wrong that the user should be aware of. A bit like a NullPointerException or IndexOufOfBoundException that would be thrown by the runtime himself.

B-Galati · 2019-10-17T12:02:16Z

@joshcanhelp

Also ... it does not seem like this solves your issue. Why would you be getting a File exists error for a mkdir() call right after you check for its existence?

Because it's a quite hard to reproduce race condition issue. See https://github.com/kalessil/phpinspectionsea/blob/master/docs/probable-bugs.md#mkdir-race-condition.

After checking this link I looked at how Symfony/Doctrine/etc. deal with that kind of race condition and then come up with this PR.

joshcanhelp

Thanks for sticking with me on this @B-Galati !

joshcanhelp · 2019-10-21T22:32:07Z

src/Helpers/Cache/FileSystemCacheHandler.php

-        if (! file_exists($this->tmp_dir)) {
-            mkdir($this->tmp_dir);
+        if (! is_dir($this->tmp_dir) && ! @mkdir($this->tmp_dir, 0777, true) && ! is_dir($this->tmp_dir) ) {
+            throw new \RuntimeException("Cache Handler was not able to create directory '$this->tmp_dir'");


At the moment the set method of the cache handler would fail

It would fail but it would be a warning, not a fatal or exception:

https://www.php.net/manual/en/function.fopen.php

I agree that a failing cache system is a problem and, in this case, since it's not something that runs on every load, it's probably best to stop the process. But we can't introduce an exception here in a minor or patch release.

src/Helpers/Cache/FileSystemCacheHandler.php

joshcanhelp · 2019-11-18T18:59:13Z

@B-Galati - Apologies for the long pause here.

I'd like to get this merged into the master branch for 5.6 but I think an exception, even if warranted, should not be added in a minor release. I think it's important enough to stop application startup but if an existing application has this misconfigured and not caching but still running fine, then a minor update will crash it immediately.

Since we're going to change how this works in the major (use a standard interface, discussed in #282 if you want to chime in), I think replacing the exception with a trigger_error() set to E_WARNING would be best.

B-Galati · 2019-12-02T09:03:59Z

@joshcanhelp done :-)

joshcanhelp · 2019-12-03T00:14:16Z

❯ composer test
> SHELL_INTERACTIVE=1 "vendor/bin/phpunit" --coverage-text
PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.1.29 with Xdebug 2.6.1
Configuration: /Users/joshcanhelp/Sites/php-auth0/auth0/phpunit.xml.dist

...S...........................................................  63 / 253 ( 24%)
............................................................... 126 / 253 ( 49%)
............................................................... 189 / 253 ( 74%)
............................................................... 252 / 253 ( 99%)
.                                                               253 / 253 (100%)

Time: 28.05 seconds, Memory: 18.00MB

There was 1 skipped test:

1) Auth0\Tests\API\Authentication\DeprecatedTest::testAuthorizeWithRO
New applications do not provide this grant.

/Users/josh-cunningham/Sites/php-auth0/auth0/tests/API/Authentication/DeprecatedTest.php:12

OK, but incomplete, skipped, or risky tests!
Tests: 253, Assertions: 924, Skipped: 1.

github-actions · 2022-08-23T21:05:37Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Fix mkdir race condition in FileSystemCacheHandler

a2f59d2

https://github.com/kalessil/phpinspectionsea/blob/master/docs/probable-bugs.md#mkdir-race-condition

B-Galati requested a review from a team October 14, 2019 16:33

joshcanhelp requested review from joshcanhelp and removed request for a team October 14, 2019 17:41

joshcanhelp closed this Oct 15, 2019

joshcanhelp reopened this Oct 15, 2019

joshcanhelp added the CH: Fixed label Oct 15, 2019

joshcanhelp added this to the 5.6.1 milestone Oct 16, 2019

joshcanhelp suggested changes Oct 16, 2019

View reviewed changes

review

574130b

joshcanhelp self-requested a review October 17, 2019 15:04

joshcanhelp reviewed Oct 21, 2019

View reviewed changes

replace exception by trigger_error E_USER_WARNING

29ae0c5

Merge branch 'master' into fix-mkdir-cache

25dadd4

joshcanhelp merged commit 16247ba into auth0:master Dec 3, 2019

github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mkdir race condition in FileSystemCacheHandler #375

Fix mkdir race condition in FileSystemCacheHandler #375

B-Galati commented Oct 14, 2019 •

edited

Loading

joshcanhelp commented Oct 15, 2019

B-Galati commented Oct 15, 2019

joshcanhelp commented Oct 15, 2019

B-Galati commented Oct 15, 2019

joshcanhelp commented Oct 15, 2019

joshcanhelp left a comment

joshcanhelp Oct 16, 2019 •

edited

Loading

B-Galati Oct 17, 2019

joshcanhelp Oct 21, 2019

B-Galati Oct 22, 2019

B-Galati commented Oct 17, 2019

joshcanhelp left a comment

joshcanhelp Oct 21, 2019

joshcanhelp commented Nov 18, 2019

B-Galati commented Dec 2, 2019

joshcanhelp commented Dec 3, 2019

github-actions bot commented Aug 23, 2022

Fix mkdir race condition in FileSystemCacheHandler #375

Fix mkdir race condition in FileSystemCacheHandler #375

Conversation

B-Galati commented Oct 14, 2019 • edited Loading

Changes

References

Testing

Checklist

joshcanhelp commented Oct 15, 2019

B-Galati commented Oct 15, 2019

joshcanhelp commented Oct 15, 2019

B-Galati commented Oct 15, 2019

joshcanhelp commented Oct 15, 2019

joshcanhelp left a comment

Choose a reason for hiding this comment

joshcanhelp Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

B-Galati Oct 17, 2019

Choose a reason for hiding this comment

joshcanhelp Oct 21, 2019

Choose a reason for hiding this comment

B-Galati Oct 22, 2019

Choose a reason for hiding this comment

B-Galati commented Oct 17, 2019

joshcanhelp left a comment

Choose a reason for hiding this comment

joshcanhelp Oct 21, 2019

Choose a reason for hiding this comment

joshcanhelp commented Nov 18, 2019

B-Galati commented Dec 2, 2019

joshcanhelp commented Dec 3, 2019

github-actions bot commented Aug 23, 2022

B-Galati commented Oct 14, 2019 •

edited

Loading

joshcanhelp Oct 16, 2019 •

edited

Loading