Skip to content

[6.x] Set FileStore file permissions only once#32845

Merged
taylorotwell merged 3 commits into
laravel:6.xfrom
netpok:bugfix/fileperms
May 16, 2020
Merged

[6.x] Set FileStore file permissions only once#32845
taylorotwell merged 3 commits into
laravel:6.xfrom
netpok:bugfix/fileperms

Conversation

@netpok

@netpok netpok commented May 16, 2020

Copy link
Copy Markdown
Contributor

Follow up to #32841:

This modifies some of the behaviour in 61b5aa1
From pull request #31579

That change was intended to make it easier for multiple OS users to read/write the same cached values when using the FileStore driver. However, the change introduced a subtle bug. Suppose we have the following situation:

One user (say www-data) puts a value in the cache.
2 Some time later but before the value from (1) has expired, another user (say ubuntu) attempts to update the value written in (1).
This will fail irrespective of what $filePermission value is configured for FileStore because:

after (1), the owner of the file on disk is www-data
FileStore will attempt to call FileSystem#chmod on this file
FileSystem#chmod will fail because the OS will prevent ubuntu from changing the file permissions of a file owned by www-data
In short, the $filePermission option is not working as intended when two users try to write to the same cache key.

This PR solves the problem by checking the permissions first so it only tries to set it once. As a bonus on multiple writes the check first aproach is significantly faster (only the chmod was measured).

@netpok
netpok changed the base branch from 7.x to 6.x May 16, 2020 15:43
@GrahamCampbell GrahamCampbell changed the title Bugfix/fileperms [6.x] Bugfix/fileperms May 16, 2020
@netpok netpok changed the title [6.x] Bugfix/fileperms [6.x] Set FileStore file permissions only once May 16, 2020
@GrahamCampbell

Copy link
Copy Markdown
Collaborator

Have you tested this actually works in the way you claim, and that the process is actually able to change the permissions?

@netpok

netpok commented May 16, 2020

Copy link
Copy Markdown
Contributor Author

Yes, it is tested. It should work based on logic but I also tested it manually.

  • Since the permission is set when the file is created the same process naturally can set the permissions.
  • When an other process tries to access it already has the correct permissions so there is no need to change it.
  • When the permissions cannot be changed (because it was created by an other user before the permission feature was enabled) it emits the warning which is expected in this scenario and makes more sense then silently ignore the error.

Also I can see future problems about this feature as it accepts the permission in octal form so passing 'permission' => 666 results in a problem since it should be 'permission' => 0666.

@taylorotwell
taylorotwell merged commit 7f8928b into laravel:6.x May 16, 2020
@louismrose

Copy link
Copy Markdown

Thanks for fixing this, @netpok. I've tested your approach for my use case, and it works well. The original problem that I described in #32841 is fixed by this issue.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants