8000 Verify bytes written in Filesystem::dumpFile() by lstrojny · Pull Request #36952 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Verify bytes written in Filesystem::dumpFile() #36952

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Verify bytes written in Filesystem::dumpFile() #36952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
576a778
Verify bytes written in Filesystem::dumpFile()
lstrojny May 25, 2020
70cc2db
Support dumping partial files
lstrojny May 25, 2020
2993878
Don’t advertise array
lstrojny Jun 3, 2020
820c025
Test with https wrappers
lstrojny Jun 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions src/Symfony/Component/Filesystem/Filesystem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,8 +663,8 @@ public function tempnam($dir, $prefix)
/**
* Atomically dumps content into a file.
*
* @param string $filename The file to be written to
* @param string $content The data to write into the file
* @param string $filename The file to be written to
* @param string|resource $content The data to write into the file
*
* @throws IOException if the file cannot be written to
*/
Expand All @@ -684,8 +684,20 @@ public function dumpFile($filename, $content)
// when the filesystem supports chmod.
$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);
$expectedSize = false;

if (\is_resource($content)) {
$stat = fstat($content);
if (false !== $stat) {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry that's not generic enough: a userland stream wrapper could very well decode to return 0 ou -1 here. The only safe way would be to stream "manually".
BUT I would really like to know how PHP behaves in out of space situations here.
Could you give it a try? Mounting a loopback filesystem could be a way to try maybe? (I don't know how better than Google sorry).

Copy link
Contributor Author
@lstrojny lstrojny Jun 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good idea to check again! Here is an example using a loopback device:

Warning: file_put_contents(): Only 401408 of 1048576 bytes written, possibly out of free disk space in Command line code on line 1
bool(false)

It even leaves behind a file:

 ls -lh another-file
-rw-r--r-- 1 vagrant vagrant 392K Jun  9 19:14 another-file

If this is indeed cross-platform behavior than I misdiagnosed the issue here – in fact there wouldn't be an issue. The tempfile couldn’t be fully written and file_put_contents() returns false, the exception is thrown, everything is fine.

Setup for the loopback device:

# Create loopback image file with 100x 10K blocks
$ dd if=/dev/zero of=limited.img bs=10K count=100
# Initiate the image as a loopback device
$ losetup -fP limited.img
# Create ext4 filesystem on the loopback image
$ mkfs.ext4 limited.img
# Create mountpoint
$ mkdir limited-mountpoint
# Mount the loopback device. Check that loop0 is indeed your device with losetup --list
$ sudo mount -o loop /dev/loop0 limited-mountpoint
# Fill the disk
dd if=/dev/zero of=limited-mountpoint/file bs=10K count=50
# Try file_put_contents()
php -ddisplay_errors=1 -r 'var_dump(file_put_contents("limited-mountpoint/another-file", str_repeat("a", 1024 * 1024)));'

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks for double-checking. This means we can close as everything is fine: an error will happen as expected, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that behavior is consistent across platform, yes. I don’t have access to Windows nor would I know how to simulate this. Do you have anybody in mind?

Copy link
Member

Choose a reason for hiding this comment

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

It would make sense that this behaves the same on Windows.
I'm not able to verify. But let's close until someone proves there is something to do.
Thanks for having a look!

$expectedSize = $stat['size'] - ftell($content);
}
} else {
$expectedSize = array_sum(array_map('strlen', (array) $content));
Copy link
Member

Choose a reason for hiding this comment

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

we don't want to add support for arrays - they worked "by chance" but that's not supported
10000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are not dealing with arrays in 3.4 it would break the current behavior – the one that is only explicitly deprecated in 4.0. That doesn’t seem the right thing to do.

}

$actualSize = @file_put_contents($tmpFile, $content);
if ((false === $expectedSize && false === $actualSize) && ($actualSize !== $expectedSize)) {
throw new IOException(sprintf('Failed to write file "%s". Wrote %d of %d bytes.', $filename, $actualSize, $expectedSize), 0, null, $filename);
}

@chmod($tmpFile, file_exists($filename) ? fileperms($filename) : 0666 & ~umask());
Expand Down
32 changes: 32 additions & 0 deletions src/Symfony/Component/Filesystem/Tests/FilesystemTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1505,6 +1505,38 @@ public function testDumpFileWithResource()
$this->assertStringEqualsFile($filename, 'bar');
}

public function testDumpPartialFileWithResource()
{
$filename = $this->workspace.\DIRECTORY_SEPARATOR.'foo'.\DIRECTORY_SEPARATOR.'baz.txt';

$resource = fopen('php://memory', 'rw');
fwrite($resource, 'bar');
fseek($resource, 1);

$this->filesystem->dumpFile($filename, $resource);

fclose($resource);
$this->assertFileExists($filename);
$this->assertStringEqualsFile($filename, 'ar');
}

/**
* @group network
*/
public function testDumpFileWithHttpStream()
{
if (!\in_array('https', stream_get_wrappers())) {
$this->markTestSkipped('"https" stream wrapper is not enabled.');
}
$sourceFilePath = 'https://symfony.com/images/common/logo/logo_symfony_header.png';
$targetFilePath = $this->workspace.\DIRECTORY_SEPARATOR.'dump_target_file';

$this->filesystem->dumpFile($targetFilePath, fopen($sourceFilePath, 'rb'));

$this->assertFileExists($targetFilePath);
$this->assertEquals(file_get_contents($sourceFilePath), file_get_contents($targetFilePath));
}

public function testDumpFileOverwritesAnExistingFile()
{
$filename = $this->workspace.\DIRECTORY_SEPARATOR.'foo.txt';
Expand Down
0