Support non-sequential writes #208
Conversation
Issue #207 Support non-sequential writes
| @@ -256,6 +256,8 @@ export class SftpSessionHandler { | |||
| temporaryFile.fd, | |||
| data, | |||
| 0, | |||
| data.length, | |||
| offset, | |||
There was a problem hiding this comment.
Before merging I want to understand what happens if offset is an incredibly high number (since offset is user provided).
We may need to add some type of validation here, especially if the offset maps to a reservation of disk space.
(To be clear, there is nothing to prevent a DOS due to uploading a huge amount of data in general, but at least in that case the data has to be uploaded as opposed to having a single "write" that has a high offset)
There was a problem hiding this comment.
My understanding is that the offset will result in a zero-filled file up to the offset, so if offset was 1TB that would create a 1TB file.
We have a decision to make!
There was a problem hiding this comment.
OK so we had some discussion during today's rclone checkin.
Karl raised the idea of simply not allowing non-sequential writes (throwing an error / returning a failure status if offsets are not sequential). This would prevent corruption but it might result in unpredictable end user experiences depending on whether a given SFTP client attempted non-sequential writes.
That said, I will spend a moment looking at whether or not rclone runs in sequence at all times.
The "right" solution is to (1) fix our temporary storage problem entirely to remove the threat and (2) make it possible for SFTP service to know a user's quota and enforce that quota. The first step is already captured in #145 I'll make an issue for the second.
There was a problem hiding this comment.
In the mean time I added in the 5 GB per-file limit, which feels at least a little bit safer.
AWS has a limit of 5GB uploads for single-part uploads, which is the reason 5GB was selected here. This does not necessarily prevent the risk of a DOS but it does require a user to start multiple simultaneous uploads. Issue #145 A user could create a DOS by uploading files that are too large
|
Testing this PR is not particularly easy to do (since we don't have automated tests set up sigh) -- I'm not sure how to force an SFTP client to send writes out of order. That said, I did (A) upload a file large enough to use multiple writes and confirmed it still arrived correctly and (B) upload a file that was larger than the limit and confirmed an error status was returned for that file. |
This PR fixes a bug where writes were always done in the order the commands were received (instead of writing to specific positions of the temporary file).
Resolves #207