Skip to content
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

[6.x] Allows multiple OS users to share values cached in file store #32841

Closed
wants to merge 5 commits into from

Conversation

louismrose
Copy link

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:

  1. 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.

@louismrose
Copy link
Author

louismrose commented May 16, 2020

To address the bug described above, I'm proposing adding an ensureOwnership flag to FileSystem#chmod. By default, this will be set to false, which means that the change is backwards-compatible with all consumers of this API.

The new flag is used by FileStore#put to indicate that we only wish to apply $filePermission to files that we own. This is an improvement over existing behaviour, because the chmod will either continue to apply cleanly, or we will ignore any exception that was previously thrown and continue on.

It is safe to ignore this exception, because if we encounter a file that we don't own in the cache directory, it was very likely written by a different PHP process running the Laravel FileStore code, which will itself have already applied the $filePermissions to this file.

@@ -192,9 +192,13 @@ public function append($path, $data)
* @param int|null $mode
* @return mixed
*/
public function chmod($path, $mode = null)
public function chmod($path, $mode = null, $ensureOwnership = false)
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change unfortunately.

@netpok
Copy link
Contributor

netpok commented May 16, 2020

In my opinion it's not the chowns job to ensure this, also since this is changing a public api it should target master.

Better solutions IMO:

  • Simply silence the warning by adding @ FileStore#79
  • Check the perms and only update if required (this has the advantage of the warning is still thrown when needed)

@taylorotwell
Copy link
Member

Breaking change as noted above.

@louismrose
Copy link
Author

louismrose commented May 16, 2020

Thank you all for looking. I think there is a bug here to be resolved, so I wonder if you might consider an alternative solution. (I'm happy to raise the PR).

How about the second solution proposed by @netpok? This would amount to this change in FileStore:

        if ($result !== false && $result > 0) {
-             if (! is_null($this->filePermission))
+             if (! is_null($this->filePermission) && $this->files->chmod($path) !== $this->filePermission) {
                 $this->files->chmod($path, $this->filePermission);
             }

@netpok
Copy link
Contributor

netpok commented May 16, 2020

@louismrose Sent a PR for it. Also the code you sent is not correct since the chmod will return an octal string but it will be evaluated as a decimal string,

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