Fixing a data transfer error #1724
Conversation
The -1 can cause the last byte to not end up in the request body (incomplete file), causing the server to time out because it expects the content length to be one byte larger. However, this one byte that isn't loaded and isn't sent is present in the file. I was able to successfully test this and it fixed the error for me. Signed-off-by: Alexander-Ger-Reich <[email protected]>
|
If length is 1, it means that 1 byte of content is present. 0 means that there is no content. With -1, the program thinks that maxcount is 0 and therefore empty, even though 1 byte is still there. So if the byte buffer is 4096 and the content is 4097 bytes, the last one won't be loaded because it assumes the wrong length. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
A quick look suggests this may have been introduced via #1173 when |
|
May fix:
Also worth mentioning owncloud/android-library#436. While not directly related, something we should confirm while we're in this area of code since it's a similar matter. |
There was a problem hiding this comment.
@Alexander-Ger-Reich, thank you for taking the time to document this issue and your findings!
I've managed to reproduce the issue using my test setup. For this to work one has to first prepare test files which match a certain pattern. I've uploaded mine to this shared folder. As this was quite intriguing I debugged this as well and left my findings below.
These are the results without this change in place:
| Filename | Size in B | maxCount |
Result | Size on Server in B |
|---|---|---|---|---|
| 16383998.bin | 16383998 | 16383997 | 201 | 16383998 |
| 16383999.bin | 16383999 | 16383998 | 201 | 16383999 |
| 16384000.bin | 16384000 | 16383999 | 201 | 16384000 |
| 16384001.bin | 16384001 | 16384000 | SocketException | - |
| 16384002.bin | 16384002 | 16384001 | 201 | 16384002 |
All files of a certain file size are affected. To rephrase @Alexander-Ger-Reich findings:
0 = (file_size - 1) mod chunk_size
where chunk_size = 40960000 on WiFi
or chunk_size = 10240000 on Mobile
This happened because we check if mChannel.position() < maxCount and if we end up exactly on the boundary between chunks, we fail to copy the last byte.
After this change all test files were uploaded correctly. All file lengths and checksums match so it shouldn't break anything.
I also don't think that adding a test case is necessary since it would be highly specific to one particular piece of faulty logic which will be fixed by this pull request.
Again, thank you for your time and effort!
|
In the issue nextcloud/android#14950, the expected file size is 31232001. This does not satisfy the relation 0 = (31232001 - 1) mod 40960000 (WiFi). Just putting it out here in case there are other edge cases too. |
Looks like the actual relation should be (file_size - 1) mod 4096 = 0 (and not 40960000). A file size of 31232001 satisfies this. So this might fix nextcloud/android#14950. |
|
private ByteBuffer mBuffer = ByteBuffer.allocate(4096); |
|
Just wondering, whats holding back the checkin for this one? |
It is a mathematical problem. It assumes the wrong base in the byte array in the loop. It thinks the base is 1 but in reality it is 0. |
|
@Alexander-Ger-Reich, thank you again for taking the time and effort for finding, documenting, and fixing this issue. As @crimsonflame123 already speculated, we haven't clicked the shiny merge button because of the CI failure. Due to our security policies, we do not allow our CI tests to run on forks of our repositories. As a result the Analysis step of our pipeline will always fail in this pull request, which is also shown in the respective log. To still run all checks and merge this pull request, I've pushed all changes to a local branch of this repository and opened another pull request: #1745. Once all tests have finished, your original pull request will be merged and the other one will be closed. |
|
Since this is a data corruption/service down issue, is there anyway that I can help to get it checked in. The code seems to be there but the checkin held up. |
|
Noob question, but how long does it take for changes in nextcloud:master to make it to the android client in Google Play? |
|
/backport to stable-2.21 |
We've created a backport of this change to |
6 participants
The -1 can cause the last byte to not end up in the request body (incomplete file), causing the server to time out because it expects the body length to be one byte larger based on the content length. However, this one byte that isn't sent is present in the file.
I was able to successfully test this and it fixed the error for me.
Fix for
