Fix integer overflow in ChunkingPlugin #8112

MorrisJobke · 2018-01-30T13:24:53Z

Avoids errors when the size exceeds MAX_INT because of the cast to int. Better cast it to float to avoid this.

Fix #7948 and #8109 (comment)

@nariox @pasxalisk Could you check if this fixes the issue for you?

rullzer

Ah 32bit systems?

codecov · 2018-01-30T13:48:36Z

Codecov Report

Merging #8112 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #8112      +/-   ##
============================================
+ Coverage     51.88%   51.88%   +<.01%     
  Complexity    25352    25352              
============================================
  Files          1606     1606              
  Lines         95069    95069              
  Branches       1378     1378              
============================================
+ Hits          49326    49328       +2     
+ Misses        45743    45741       -2

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/Upload/ChunkingPlugin.php	`96.29% <100%> (ø)`	`8 <0> (ø)`	⬇️
lib/private/Files/Cache/Propagator.php	`94.93% <0%> (-1.27%)`	`16% <0%> (ø)`
lib/private/Files/ObjectStore/SwiftFactory.php	`56.32% <0%> (+3.44%)`	`35% <0%> (ø)`	⬇️

MorrisJobke · 2018-01-30T14:07:35Z

Ah 32bit systems?

Exactly - some raspberry Pis and stuff like that.

pasxalisk · 2018-01-30T16:15:05Z

@MorrisJobke For me it does fix it.
I managed to upload a single file 45gb on my RPi 3 running Nextcloud 13 RC3.

pasxalisk · 2018-01-30T17:34:07Z

@MorrisJobke
EDIT: It did work with the solution i posted.
When i tried the above i saw a "Unkniwn CSync Error" on the windows client.

pasxalisk · 2018-01-30T18:31:01Z

@MorrisJobke
EDIT2: I tried again on a clean install and it seems that the fix is working as expected.
I am sorry for any inconvinience.

ghost · 2018-01-30T19:11:26Z

@pasxalisk Raspberry Pi 3 use a 64 bit ARM CPU ( https://en.wikipedia.org/wiki/ARM_Cortex-A53 ) can you check on another ARM CPU please?

pasxalisk · 2018-01-30T19:37:17Z

@voxdemonix true. It does use a x64 but the raspbian OS is 32bits.
Unfortunatelly i don't own an ARM x64 OS device to test it.

go2sh · 2018-01-31T08:18:28Z

apps/dav/lib/Upload/ChunkingPlugin.php

@@ -99,7 +99,7 @@ private function verifySize() {
 			return;
 		}
 		$actualSize = $this->sourceNode->getSize();
-		if ((int)$expectedSize !== $actualSize) {
+		if ((float)$expectedSize !== (float)$actualSize) {


This actually might create some very very tricky corner cases, where two not equal numbers get casted to the same float value, I think.

Do you have an example? Currently this is exactly the case that one number is the float (because it can store higher numbers) and the other one is casted to integer and caused an overflow there and thus is not the same.

I created a small test program and with higher numbers there a ton of collisions. And it must be: Int has 31 bits for the actual number and a float only 23. See my other comment.

Why you have to cast the values? String comparison not good enough?

If not, maybe bcmath/gmp should be used on 32bit systems.

@icewind1991 @rullzer @nickvergessen What do you think about the cast to string? Is this the better approach?

go2sh · 2018-01-31T09:46:45Z

Test program in c#:

namespace ConsoleApp2
{
    class Program
    {
        static void Main(string[] args)
        {
            int i = 0;
            for(i=0;i<= 2147483647;i++)
            {
                int j;
                float a = (float)i;
                for (j=i-128;j <= i+128;j++)
                {
                    float b = (float)j;
                    if (a == b && i != j)
                    {
                        System.Console.Out.WriteLine(String.Format("i: {0}, j: {1}, a: {2}, b: {3}", i,j,a,b));
                    }
                }
            }
        }
    }
}

Result:

...
i: 16782085, j: 16782084, a: 1,678208E+07, b: 1,678208E+07
i: 16782087, j: 16782088, a: 1,678209E+07, b: 1,678209E+07
i: 16782087, j: 16782089, a: 1,678209E+07, b: 1,678209E+07
i: 16782088, j: 16782087, a: 1,678209E+07, b: 1,678209E+07
i: 16782088, j: 16782089, a: 1,678209E+07, b: 1,678209E+07
i: 16782089, j: 16782087, a: 1,678209E+07, b: 1,678209E+07
i: 16782089, j: 16782088, a: 1,678209E+07, b: 1,678209E+07
i: 16782091, j: 16782092, a: 1,678209E+07, b: 1,678209E+07
i: 16782091, j: 16782093, a: 1,678209E+07, b: 1,678209E+07
i: 16782092, j: 16782091, a: 1,678209E+07, b: 1,678209E+07
i: 16782092, j: 16782093, a: 1,678209E+07, b: 1,678209E+07
i: 16782093, j: 16782091, a: 1,678209E+07, b: 1,678209E+07
i: 16782093, j: 16782092, a: 1,678209E+07, b: 1,678209E+07
i: 16782095, j: 16782096, a: 1,67821E+07, b: 1,67821E+07
i: 16782095, j: 16782097, a: 1,67821E+07, b: 1,67821E+07
i: 16782096, j: 16782095, a: 1,67821E+07, b: 1,67821E+07
i: 16782096, j: 16782097, a: 1,67821E+07, b: 1,67821E+07
i: 16782097, j: 16782095, a: 1,67821E+07, b: 1,67821E+07
i: 16782097, j: 16782096, a: 1,67821E+07, b: 1,67821E+07
i: 16782099, j: 16782100, a: 1,67821E+07, b: 1,67821E+07
...

lnicola · 2018-01-31T09:49:16Z

@go2sh I don't know what float and int usually are in PHP, but a C# float is 32-bit, the same as an int. So large ints will will map to the same float.

The PHP documentation seems to imply that float is sometimes double, which would work fine for this purpose, but it's probably not true on 32-bit ARM systems.

lnicola · 2018-01-31T09:52:38Z

On the other hand, this seems more like an extra safety check (a file size match won't actually guarantee that the data was received correctly. Worst case, an error won't be reported when it should, but this won't prevent uploads from succeeding.

go2sh · 2018-01-31T10:07:55Z

This very tricky... I have no proper solution.
@MorrisJobke You mean getSize() returns a float anyway, if its bigger than 2 GB on 32-bit systems?

nariox · 2018-01-31T17:28:43Z

Thank you for the patch @MorrisJobke , it does "solve" my problem.

For those interested, PHP only has INTs and FLOATs defined, but whether they are 32-bit or 64-bit depends on the platform. The default behavior in PHP is to (quietly) convert INTs to FLOATs if they exceed PHP_INT_MAX.

It seems like there is a proposal to introduce BIGINTs to PHP (https://wiki.php.net/rfc/bigint), but not implemented yet.

The main problem with both approaches (casting to floats or ints) is that we lose precision. Floats would miss "small" errors, where the chunk size difference is beyond the precision, while ints fail in 32-bit int implementations, which the overflow would allow "large" chunk size errors to pass. Maybe a better approach would be to compare both (and casting ints, so we are only comparing the LSBs)? (Or to convert both to string, although I don't know to prevent overflow errors from happening before that)

nariox · 2018-01-31T21:22:20Z

By the way, wouldn't it be better to checksum the files instead? This might require some changes in the client, but would give us a better check altogether than just chunk sizes and avoid the 32/64 bit debacle.

lnicola · 2018-01-31T22:30:54Z

This doesn't affect me, and I don't know the full context, but I'm thinking that:

the check isn't that useful and if two different floats compare as equal it's just a false negative; maybe it can be omitted on 32-bit systems
if the client is a browser, JavaScript has double precision support, so it can either compute the number of chunks, or send the size as a pair of integers (high and low part)
if the client is PHP, though luck
if the client is another application, this check might be annoying; one use case I have in mind is streaming a tar archive to the NextCloud server — the client won't know the total size beforehand.

davidpoza · 2018-02-23T18:39:17Z

@pasxalisk

@MorrisJobke For me it does fix it.I managed to upload a single file 45gb on my RPi 3 running Nextcloud 13 RC3.

What OS are you running in the rpi3? I'm using hypriotOS (debian based) and cannot pass over 2GB. I've patched the code.

Thx

Avoids errors when the size exceeds MAX_INT because of the cast to int. Better cast it to float to avoid this. Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke · 2018-03-06T17:48:22Z

I updated this to be a cast to string. @pasxalisk @davidpoza @nariox Could you try the new patch and check if this works properly for you?

MorrisJobke · 2018-03-06T17:49:10Z

@maanloper and @Danny3 reported that this already works in #7948 (comment)

nariox · 2018-03-07T17:42:06Z

I'm now having a different issue, but not sure if it is part of this (seems to be).
My uploads fail with "Sabre\DAV\Exception: Error while copying file to target location (copied bytes: 2147483647, expected filesize: )"

2147483647 is 2³¹-1

The particular exception is being thrown by /apps/dav/lib/Connector/Sabre/File.php at line 178:
throw new BadRequest('expected filesize ' . $expected . ' got ' . $count);
Commenting out this line leads to "successful" uploads, but files are cropped to 2GiB. My guess (as a non-PHP dev, is that casting the $expected to int causes it to be empt (maybe casting to strings might help?), but the copied bytes might need some work.

Since other people don't seem to having this problem, my guess is that I might want to file a new bug. But let me know what happens to you guys.

MorrisJobke · 2018-03-09T09:27:15Z

Let's get this in

MorrisJobke · 2018-03-09T09:30:33Z

backport to stable13 is in #8752

MorrisJobke added bug backport-request labels Jan 30, 2018

MorrisJobke added this to the Nextcloud 14 milestone Jan 30, 2018

MorrisJobke requested review from rullzer, nickvergessen and icewind1991 January 30, 2018 13:24

MorrisJobke mentioned this pull request Jan 30, 2018

Nextcloud 13 #8109

Closed

rullzer approved these changes Jan 30, 2018

View reviewed changes

go2sh reviewed Jan 31, 2018

View reviewed changes

nariox mentioned this pull request Jan 31, 2018

Chunks on server do not sum up to X but to X (same number) #7948

Closed

icewind1991 approved these changes Feb 26, 2018

View reviewed changes

MorrisJobke added the 2. developing Work in progress label Feb 26, 2018

Fix integer overflow in ChunkingPlugin

fc4e050

Avoids errors when the size exceeds MAX_INT because of the cast to int. Better cast it to float to avoid this. Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke force-pushed the fix-integer-overflow branch from 3d670a0 to fc4e050 Compare March 6, 2018 17:47

MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 6, 2018

MorrisJobke merged commit ed50085 into master Mar 9, 2018

MorrisJobke deleted the fix-integer-overflow branch March 9, 2018 09:27

MorrisJobke mentioned this pull request Mar 9, 2018

[stable13] Fix integer overflow in ChunkingPlugin #8752

Merged

MorrisJobke removed the backport-request label Mar 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix integer overflow in ChunkingPlugin #8112

Fix integer overflow in ChunkingPlugin #8112

MorrisJobke commented Jan 30, 2018

rullzer left a comment

codecov bot commented Jan 30, 2018 •

edited

Loading

MorrisJobke commented Jan 30, 2018

pasxalisk commented Jan 30, 2018

pasxalisk commented Jan 30, 2018

pasxalisk commented Jan 30, 2018

ghost commented Jan 30, 2018

pasxalisk commented Jan 30, 2018

go2sh Jan 31, 2018 •

edited

Loading

MorrisJobke Jan 31, 2018

go2sh Jan 31, 2018

jkroepke Jan 31, 2018

MorrisJobke Feb 26, 2018

go2sh commented Jan 31, 2018

lnicola commented Jan 31, 2018 •

edited

Loading

lnicola commented Jan 31, 2018

go2sh commented Jan 31, 2018

nariox commented Jan 31, 2018

nariox commented Jan 31, 2018

lnicola commented Jan 31, 2018 •

edited

Loading

davidpoza commented Feb 23, 2018

MorrisJobke commented Mar 6, 2018

MorrisJobke commented Mar 6, 2018 •

edited

Loading

nariox commented Mar 7, 2018

MorrisJobke commented Mar 9, 2018

MorrisJobke commented Mar 9, 2018

Fix integer overflow in ChunkingPlugin #8112

Fix integer overflow in ChunkingPlugin #8112

Conversation

MorrisJobke commented Jan 30, 2018

rullzer left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2018 • edited Loading

Codecov Report

MorrisJobke commented Jan 30, 2018

pasxalisk commented Jan 30, 2018

pasxalisk commented Jan 30, 2018

pasxalisk commented Jan 30, 2018

ghost commented Jan 30, 2018

pasxalisk commented Jan 30, 2018

go2sh Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

MorrisJobke Jan 31, 2018

Choose a reason for hiding this comment

go2sh Jan 31, 2018

Choose a reason for hiding this comment

jkroepke Jan 31, 2018

Choose a reason for hiding this comment

MorrisJobke Feb 26, 2018

Choose a reason for hiding this comment

go2sh commented Jan 31, 2018

lnicola commented Jan 31, 2018 • edited Loading

lnicola commented Jan 31, 2018

go2sh commented Jan 31, 2018

nariox commented Jan 31, 2018

nariox commented Jan 31, 2018

lnicola commented Jan 31, 2018 • edited Loading

davidpoza commented Feb 23, 2018

MorrisJobke commented Mar 6, 2018

MorrisJobke commented Mar 6, 2018 • edited Loading

nariox commented Mar 7, 2018

MorrisJobke commented Mar 9, 2018

MorrisJobke commented Mar 9, 2018

codecov bot commented Jan 30, 2018 •

edited

Loading

go2sh Jan 31, 2018 •

edited

Loading

lnicola commented Jan 31, 2018 •

edited

Loading

lnicola commented Jan 31, 2018 •

edited

Loading

MorrisJobke commented Mar 6, 2018 •

edited

Loading