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

do not write htaccess file if disk space is too low #41544

Merged
merged 2 commits into from Nov 20, 2023

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Nov 16, 2023

I copied the logic from

// Never write file back if disk space should be too low
if (function_exists('disk_free_space')) {
$df = disk_free_space($this->configDir);
$size = strlen($content) + 10240;
if ($df !== false && $df < (float)$size) {
throw new \Exception($this->configDir . " does not have enough space for writing the config file! Not writing it back!");
}
}

@szaimen szaimen added bug 3. to review Waiting for reviews labels Nov 16, 2023
@szaimen szaimen added this to the Nextcloud 28 milestone Nov 16, 2023
@szaimen szaimen requested review from tobiasKaminsky, a team, ArtificialOwl, Altahrim and sorbaugh and removed request for a team November 16, 2023 15:09
@szaimen
Copy link
Contributor Author

szaimen commented Nov 16, 2023

/backport to stable27

@kesselb
Copy link
Contributor

kesselb commented Nov 16, 2023

Why?

@szaimen
Copy link
Contributor Author

szaimen commented Nov 16, 2023

Why?

Because the file got truncated on @tobiasKaminsky's instance due to low disk space...

@blizzz blizzz mentioned this pull request Nov 16, 2023
@szaimen szaimen requested a review from kesselb November 17, 2023 10:33
if ($df !== false && $df < (float)$size) {
throw new \Exception(\OC::$SERVERROOT . " does not have enough space for writing the htaccess file! Not writing it back!");
}
}
//suppress errors in case we don't have permissions for it
return (bool)@file_put_contents($setupHelper->pathToHtaccess(), $htaccessContent . $content . "\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also check if all bytes have been written. Should be something like this:

Suggested change
return (bool)@file_put_contents($setupHelper->pathToHtaccess(), $htaccessContent . $content . "\n");
$fullContent = $htaccessContent . $content . "\n";
return @file_put_contents($setupHelper->pathToHtaccess(), $$fullContent) === strlen($fullContent);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not going to help here. The prpblem is that file_pu_content truncates the file if there is not enough space which should never happen as the file is very important to work...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had the same prpblem with config.php before and fixed in in the same way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's another security in case something happen during the copy.
The file will already be erased, but at least you'll have the info

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see.

Signed-off-by: Simon L <szaimen@e.mail.de>
Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen szaimen force-pushed the enh/noid/updatehtaccess-disk-space-check branch from 99a3dc6 to 3f6caa4 Compare November 17, 2023 14:22
@szaimen szaimen merged commit 753e7c2 into master Nov 20, 2023
50 checks passed
@szaimen szaimen deleted the enh/noid/updatehtaccess-disk-space-check branch November 20, 2023 07:14
@blizzz blizzz mentioned this pull request Nov 20, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants