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

Prevent 507 Insufficient Storage on 32-bit systems #40709

Merged
merged 4 commits into from
Mar 29, 2023
Merged

Conversation

pako81
Copy link

@pako81 pako81 commented Mar 28, 2023

Description

Prevent 507 Insufficient Storage on 32-bit systems

Related Issue

Motivation and Context

With the introduction of c368786 we apparently broke compatibility to 32-bit systems as we are now casting $freeSpace to int and this is causing an integer overflow on such systems when the free space is above the max supported value. We added therefore an additional check for 32-bit systems in QuotaPlugin.php.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised
  • Changelog item

@pako81 pako81 self-assigned this Mar 28, 2023
@pako81 pako81 added this to the development milestone Mar 28, 2023
@pako81 pako81 changed the title Prevent 507 Insufficient Storage on 32bit systems Prevent 507 Insufficient Storage on 32-bit systems Mar 28, 2023
@pako81 pako81 requested a review from jnweiger March 28, 2023 13:15
@heppel
Copy link

heppel commented Mar 28, 2023

Can't test this now. But I think the original code with the (int) cast should be in the "else" clause of the PHP_INT_SIZE test or the overflow and the incorrect error will still happen.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 4 Code Smells

16.7% 16.7% Coverage
0.0% 0.0% Duplication

@owncloud owncloud deleted a comment from update-docs bot Mar 29, 2023
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM now. There is code duplication, but minimizing that might obfuscate the separate 32-bit and not-32-bit code blocks.

@phil-davis phil-davis requested a review from jvillafanez March 29, 2023 04:59
@pako81
Copy link
Author

pako81 commented Mar 29, 2023

Manual testing in #40695 seems to confirm this solves the issue.

@pako81 pako81 merged commit b831feb into master Mar 29, 2023
@delete-merged-branch delete-merged-branch bot deleted the avoid_507_on_32bit_OS branch March 29, 2023 07:36
Copy link
Contributor

@jnweiger jnweiger left a comment

Choose a reason for hiding this comment

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

I am not 100% convinced, all edge cases are covered. But as the immediate problem of the user is nicely fixed. I am happy with the solution too.

@jvillafanez
Copy link
Member

I am not 100% convinced, all edge cases are covered.

As in "there could be more issues caused by integer overflow in 32 bits"?. Yeah, probably. If it isn't officially supported and we aren't testing with 32 bits, there could be issues lurking around.
For this particular issue with the quota, I don't think we're missing anything.

@Valentin-Bleuse
Copy link

I changed apps/dav/lib/Connector/Sabre/QuotaPlugin.php still having this issue : there is insufficient space available on the server for some upload. Nothing have changed even with your new code. No idea why it doesn't work properly.

@jvillafanez
Copy link
Member

If you're using a 32 bit system, you might still be limited to uploads of less than 2GB. I'm not sure about the exact behavior, but there could be issues.
Other than that, I guess you have a regular "not enough quota" error, so check how much quota your user has and ensure the file fits in it.

@jvillafanez
Copy link
Member

@pako81 we might need a different fix. It seems that the $extraSpace variable can also be a float.
If we're copying to an already existing file, the $extraSpace will be the size of that file. Doctrine should return the size of the file as string in https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Cache.php#L162 and 0 + <string> could return a float

php > var_dump(0 + "12378974983729847892798758974897389578934759876978");
float(1.237897498373E+49)
php > var_dump(0 + "345");
int(345)

That's "consistency" in PHP....

I think the reproduction steps should be:

  1. Upload a 3GB file to ownCloud
  2. Copy any file into the 3GB file in order to replace it.

Maybe it works for 64 bits because the bigint coming from the DB fits in a 64 bits integer

I think we'll have to keep the "32 bits block" and remove the other one. Need to check all the places where the getSize method says it returns an integer...
For now, we'll have to assume that both $freeSpace and $extraSpace can be floats.

@Valentin-Bleuse
Copy link

It's not from the quotas for sure and even when I put a single photo, it told me I don't have any space left

@jvillafanez
Copy link
Member

@Valentin-Bleuse we might need a new ticket for that. I'd recommend you to try to reproduce the issue either in a fresh environment or in https://demo.owncloud.com . It might be a different issue, and without steps to reproduce we can't fix the problem.

In addition, make sure you have enough space everywhere:

  • The web server uses the "/tmp" folder (or whatever temporary directory is configured), and that folder might be limited.
  • There could be also quota limitations for the user configured by the ownCloud's admin.
  • Your host might not have enough free space

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

Successfully merging this pull request may close these issues.

6 participants