bug #14246 [Filesystem] dumpFile() negates default file permissions #17063

hboomsma · 2015-12-18T11:50:34Z

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

Remain BC compatible with Symfony 2.8.
Without this change a chmod is needed after calling dumpFile, making it non atomic.

cs278 · 2015-12-18T13:28:14Z

src/Symfony/Component/Filesystem/Filesystem.php

@@ -523,6 +523,7 @@ public function dumpFile($filename, $content)
        }

        $tmpFile = $this->tempnam($dir, basename($filename));
+        $this->chmod($tmpFile, 0666, umask());


Apply the mode change after writing to the file, the umask may remove the write permission.

linaori · 2015-12-30T13:51:31Z

If I understand it correctly, there has been a deprecation added in 2.8 which was never merged into 3.0 and the code was never fixed properly there. If you take a look at the history of Filesystem.php:

The 3rd option is marked as deprecated
The default value is 0666
If the value is not null, it will set the permissions
By passing 2 arguments you will not trigger the deprecation notice but it will still set the permissions to 0666

This means that the only case where no permissions would be set, would be by passing null as third argument which was deprecated, but that's the exact behavior 3.0 has right now.

When you take a look at the 3.0 Filesystem.php, you can see that the permissions are not longer fixed. The only non-deprecated way, which is setting it 0666 afterwards is broken in 3.0.

/cc @xabbuh

xabbuh · 2015-12-30T14:04:22Z

@iltar I think your reasoning is correct and we need to always set the permissions as proposed here.

By the way, the solution before Symfony 3.0 was not really atomic, was it? I mean the chmod() call was done after renaming the file. Shouldn't this happened the other way around (like proposed here)?

hboomsma · 2015-12-30T14:05:02Z

@iltar, this is indeed the case.

Code that ran in 2.8 without any deprication message stops working in 3.0 because the tempnam function of php creates files with access permissions set to 0600. In 2.8 this was fixed by running a chmod with 0666 (ignoring system umask). In 3.0 however, nothing is done and only the creating user can read the file.

hboomsma · 2015-12-30T14:06:29Z

@xabbuh I agree that the permissions should be changed after writing as @cs278 pointed out and before renaming to make this operation atomic.

xabbuh · 2015-12-30T14:08:45Z

@hboomsma Would you like to create a pull request for older Symfony versions that swaps the order of the calls?

hboomsma · 2015-12-30T14:12:10Z

@xabbuh sure, which versions should we patch? I'll create a pull request for 2.8 right away.

xabbuh · 2015-12-30T14:17:19Z

Hm, I think we will get other issues when merging this on filesystems that don't allow to change permissions (see #8205).

hboomsma · 2015-12-30T14:26:40Z

@xabbuh you are right, I'll fix it in the same way, it was fixed in the mentioned issue, because if chmod is not supported tempnam will not mess with permissions either.

As for the request for 2.8 do you have an open issue for me to mention in the PR?

xabbuh · 2015-12-30T14:28:13Z

@hboomsma I think you can refer to the same issue (it's also talking about the (not)-atomic behaviour).

hboomsma · 2015-12-30T14:44:05Z

@xabbuh #17184 for atomic behaviour in 2.8

linaori · 2015-12-30T14:54:53Z

Looks like the failure is unrelated to the changes in this PR.

xabbuh · 2015-12-30T15:03:12Z

src/Symfony/Component/Filesystem/Filesystem.php

        $tmpFile = $this->tempnam($dir, basename($filename));

        if (false === @file_put_contents($tmpFile, $content)) {
            throw new IOException(sprintf('Failed to write file "%s".', $filename), 0, null, $filename);
        }

+        // Ignore for filesystems that do not support umask
+        @chmod($tempFile, 0666 & ~umask());


Why is this done twice? And what does $tempFile refer to?

hboomsma · 2016-01-04T09:27:03Z

@xabbuh @cs278 @iltar Updated with changes done in 2.3 and merged upward for atomic behaviour

xabbuh · 2016-01-04T09:36:19Z

src/Symfony/Component/Filesystem/Filesystem.php

        $tmpFile = $this->tempnam($dir, basename($filename));

        if (false === @file_put_contents($tmpFile, $content)) {
            throw new IOException(sprintf('Failed to write file "%s".', $filename), 0, null, $filename);
        }

+        // Ignore for filesystems that do not support umask
+        @chmod($tmpFile, 0666 & ~umask());


Taking umask() into account here looks like a new feature to me.

@xabbuh, I agree you could see it that way, lets not take the umask into account and create a separate pull request for it.

hboomsma · 2016-01-04T14:34:49Z

@xabbuh updated PR to not take the umask into account.

nicoschoenmaker · 2016-01-04T14:52:33Z

src/Symfony/Component/Filesystem/Tests/FilesystemTest.php

@@ -1070,13 +1070,19 @@ public function testDumpFile()
    {
        $filename = $this->workspace.DIRECTORY_SEPARATOR.'foo'.DIRECTORY_SEPARATOR.'baz.txt';

+        // skip mode check on Windows
+        if ('\\' !== DIRECTORY_SEPARATOR) {
+            $oldMask = umask(0002);


Unit-test can be simplified now, no need to set the umask anymore.

I want to explicitly show that the umask does not influence the resulting access permissions.

If we wanted to improve the tests this way, we should do that on the 2.3 branch imo.

I agree, removed the extra tests, because I will create a separate pull request for enhancing the test and taking the umask into account for 2.3.

hboomsma · 2016-01-06T13:46:07Z

@xabbuh updated pr to not enhance the unit tests, I will create a separate PR for taking the umask into account with the extra tests for 2.3 after this PR is finished.

xabbuh · 2016-01-07T09:43:40Z

👍

Status: Reviewed

fabpot · 2016-01-07T13:26:00Z

Thank you @hboomsma.

…rmissions (Hidde Boomsma) This PR was merged into the 3.0 branch. Discussion ---------- bug #14246 [Filesystem] dumpFile() negates default file permissions | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14246 | License | MIT | Doc PR | none Remain BC compatible with Symfony 2.8. Without this change a chmod is needed after calling `dumpFile`, making it non atomic. Commits ------- 53d6d4b bug #14246 [Filesystem] dumpFile() negates default file permissions

…e (leofeyer) This PR was merged into the 3.1 branch. Discussion ---------- [Filesystem] Consider the umask setting when dumping a file | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14246 , #17063 | License | MIT | Doc PR | - `Filesystem::dumpFile()` does not consider the umask setting and thus creates files with write permissions for everyone (`0666`). Other `chmod()` calls in Symfony do consider the umask setting (see e.g. [here](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/File/File.php#L101) or [here](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/File/UploadedFile.php#L230)), therefore I have adjusted the `dumpFile()` method accordingly. Commits ------- fdd266f Consider the umask setting when dumping a file.

carsonbot added the Status: Needs Review label Dec 18, 2015

cs278 reviewed Dec 18, 2015
View reviewed changes

xabbuh reviewed Dec 30, 2015
View reviewed changes

xabbuh mentioned this pull request Jan 4, 2016

[Filesystem] set default permissions when dumping files #17246

Closed

xabbuh added the Filesystem label Jan 4, 2016

xabbuh reviewed Jan 4, 2016
View reviewed changes

nicoschoenmaker reviewed Jan 4, 2016
View reviewed changes

bug #14246 [Filesystem] dumpFile() negates default file permissions

53d6d4b

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 7, 2016

fabpot merged commit 53d6d4b into symfony:3.0 Jan 7, 2016

xabbuh mentioned this pull request Jan 18, 2016

[Filesystem] dumpFile() ignores umask settings #17303

Closed

fabpot mentioned this pull request Jan 25, 2016

bug #17303 [Filesystem] dumpFile() ignores umask settings #17304

Closed

fabpot mentioned this pull request Feb 3, 2016

Release v3.0.2 #17665

Merged

bgatellier mentioned this pull request Feb 14, 2016

chmod 600? liip/LiipImagineBundle#702

Closed

leofeyer mentioned this pull request Sep 6, 2016

[Filesystem] Consider the umask setting when dumping a file #19872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug #14246 [Filesystem] dumpFile() negates default file permissions #17063

bug #14246 [Filesystem] dumpFile() negates default file permissions #17063

hboomsma commented Dec 18, 2015

cs278 Dec 18, 2015

linaori commented Dec 30, 2015

xabbuh commented Dec 30, 2015

hboomsma commented Dec 30, 2015

hboomsma commented Dec 30, 2015

xabbuh commented Dec 30, 2015

hboomsma commented Dec 30, 2015

xabbuh commented Dec 30, 2015

hboomsma commented Dec 30, 2015

xabbuh commented Dec 30, 2015

hboomsma commented Dec 30, 2015

linaori commented Dec 30, 2015

xabbuh Dec 30, 2015

hboomsma commented Jan 4, 2016

xabbuh Jan 4, 2016

hboomsma Jan 4, 2016

hboomsma commented Jan 4, 2016

nicoschoenmaker Jan 4, 2016

hboomsma Jan 4, 2016

xabbuh Jan 5, 2016

hboomsma Jan 6, 2016

hboomsma commented Jan 6, 2016

xabbuh commented Jan 7, 2016

fabpot commented Jan 7, 2016

bug #14246 [Filesystem] dumpFile() negates default file permissions #17063

bug #14246 [Filesystem] dumpFile() negates default file permissions #17063

Conversation

hboomsma commented Dec 18, 2015

Choose a reason for hiding this comment

linaori commented Dec 30, 2015

xabbuh commented Dec 30, 2015

hboomsma commented Dec 30, 2015

hboomsma commented Dec 30, 2015

xabbuh commented Dec 30, 2015

hboomsma commented Dec 30, 2015

xabbuh commented Dec 30, 2015

hboomsma commented Dec 30, 2015

xabbuh commented Dec 30, 2015

hboomsma commented Dec 30, 2015

linaori commented Dec 30, 2015

Choose a reason for hiding this comment

hboomsma commented Jan 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hboomsma commented Jan 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hboomsma commented Jan 6, 2016

xabbuh commented Jan 7, 2016

fabpot commented Jan 7, 2016