Core: ChunkingNG: Missing chunks are silently ignored

[ckamm]

Steps to reproduce

1. Start a chunked upload with a MKCOL, setting OC-Total-Length to 10M.
2. Upload some, but not all, of the 5 2M chunks.
3. MOVE .file to the destination.
4. The MOVE will succeed. The resulting file will not be 10M in size.

Expected behaviour

The MOVE should fail because chunks are missing.

Actual behaviour

The MOVE succeeds and the target file is a concatenation of the existing chunks.

While not uploading some chunks is a user error, the server should guard against creating corrupted files. Comparison to the OC-Total-Length header allows easy detection of missing data. Additionally, if the server uses integer chunk numbers, holes in the chunk numbering could be detected (e.g. detect that only chunks 0, 1, 3, 4 were uploaded)

Server configuration

ownCloud version: master (6f726844d0164197c83872cd960a179185274ccb)

[guruz]

@PVince81 @DeepDiver1975 I need some guidance how to implement this. Not sure which layer to put it in.

You told me somewhere this should be checked at the MOVE of the .file.
However I don't think there is a specific place for this as it is just a transparent operation on a file which is a FutureFile from an UploadFolder inside the RootCollection.

So shall I check instead the OC-Total-Length header from the FutureFile? :-\

[guruz]

Wait a sec, in https://github.com/cernbox/smashbox/blob/master/protocol/chunking.md it says the OC-Total-Length is for the MKCOL not for the MOVE

[guruz]

Indeed, the client sends it with the MKCOL.

[guruz]

So the question is how could this be stored with the MKCOL? And then checked again later at the MOVE?

Or shall we change the spec to also send it with the MOVE? (FYI @ogoffart @dragotin) .. but then my question above in which layer this needs to be holds.

[DeepDiver1975]

The headers as sent with the MKCOL need to be store on the server side - yes - this is missing.
In addition to the total length we soon-ish shall add the checksum .....

[guruz]

If I need to implement this I need guidance :-)

Or is @IljaN doing this since he's also doing this then too?

[ckamm]

@guruz Also, the full validation would include checking whether the OC-Chunk-Start headers we send with each chunk are consistent with the current summed size of the file, i.e.

assert( sum_of_chunk_sizes == OC-Total-Size )
foreach chunk i
   assert( chunk_size_sum_up_to_chunk_i == OC-Chunk-Start_of_chunk_i+1 )

[PVince81]

Any update ?

[PVince81]

@DeepDiver1975 thoughts ?

[PVince81]

Store it on the server... where ?

In a different ticket I suggested having a table to list what chunks are already uploaded for easier autocleanup.

[guruz]

@PVince81 Store what on the server? I think this should be enough for now since we also should have checksums:

Comparison to the OC-Total-Length header allows easy detection of missing data.

[PVince81]

the checksum check only works if the client actually sends one

If a third party client doesn't send a checksum then they'll likely run into this issue if not careful.

[ogoffart]

If needed, the client could send the OC-Total-Length also in the MOVE call.
(It was added in the MKDIR call for the needs of cern so they can pre-allocate the buffer for the file)

[PVince81]

I can't think of any quicker solution than having clients send "OC-Total-Length" on the final MOVE also. But I'm not too happy about it.

The server code currently has no way to save any upload/transfer-related metadata because that would require a DB table to track uploads. That is, unless we add a temporary JSON file in the local "uploads" folder with everything in it... then reread. Something to think about...

@DeepDiver1975 @butonic objections to a temporary file ?

[PVince81]

to be more precise, there would be one JSON file per transfer, so they'd get cleant up with the DELETE: "data/$user/uploads/$transferId/transfer.json".

{
    checksum: 'xyz',
    totalLength: 12345
}

[PVince81]

I'm moving this to 10.0.1 as we don't have an agreement yet on the approach.

@DeepDiver1975 @guruz

[butonic]

I think a transfer.json or another file that somehow describes the chunks is what @dragotin proposed in https://dragotin.wordpress.com/2015/11/13/owncloud-chunking-ng-part-3-incremental-syncing/

[guruz]

From our side, we're ok if the server would check X-OC-Total-Size for now. Not so important to do more than that..

[PVince81]

As long as a checksum is sent by clients, I think it is catchable already. We need an integration test to prove it.

However when no checksums are involved for clients who use chunking without checksums (less likely), we need a proper solution.

[PVince81]

Still need to verify what the current behavior is without checksums to evaluate criticality...

[PVince81]

I found similar problems related to the fact that we don't have all the necessary info at the start of the transfer but only at the end, raised here: https://github.com/owncloud/core/issues/28452 (it's about target path)

[DeepDiver1975]

from my pov this is all within the clients responsibility - this was one major design decision when thinking about the new chunking api.

• the server does no implicit action (chunking or cleanup)
• the server is to be seen as passive component in this scenario

[ogoffart]

@DeepDiver1975 So what do you suggest that the client does? Should the client do an additional PROPFIND before the MOVE to make sure that all the files (that it just uploaded without error?)
That's a bit annoying as we try to reduce the amount of call to the server (altough you may argue that for big files, one call more does not have much impact)

We send the X-OC-Total-Size in the MOVE so the server should have enough information to detect that a chunk is missing or incomplete.

[guruz]

Sorry, I disagree with @DeepDiver1975

Checking X-OC-Total-Size is an appropriate sanity check.

[DeepDiver1975]

Checking X-OC-Total-Size is an appropriate sanity check.

okay - agreed ;-) - are you sending the mtime header as well?

[guruz]

Semi-related but after the assembly of the file, not during:
https://github.com/owncloud/core/issues/28585
(And unsure if my report is correct)

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.