what if file size changed between Content-Length calculation and sending data out?

[crusader-mike]

When sending data out SDK reads the file (stream) until data runs out (according to my understanding of code) -- but content-length has to be calculated before that.

I am not particularly versed in HTTP (or related layer SDK uses to talk HTTP), but what is going to happen if said file suddenly grows bigger before we reads it in it's entirety?

What if it grows smaller?

Did I discover a race condition that could lead to hanging requests, incorrect data at the end of the object and other interesting effects?

[singku]

It's a good question and yes this is a race condition scenario and will lead to incorrect data in the end. But SDK currently doesn't guarantee the consistence of file during transferring.

[JonathanHenson]

The atomicity of writes when a process is reading a file is defined by the file system. What particular file system is in play here?

[JonathanHenson]

I’ll add, I don’t suspect this is a solvable problem if the file system does not prevent this behavior.

[crusader-mike]

Oh, guys... You won't get out of this one so easy. :-)

First, you obviously can't accept any iostream as request body -- therefore your additionally imposed requirements need to be documented somewhere. At the very minimum! You may (or may not) decide to enforce these requirements in the code and resulting behavior in case of violation (UB/exception/etc) should be mentioned in docs too.

Now, I tested it today on Windows by (deliberately) setting incorrect content-length -- I believe this should emulate actual race situation. Ended up with following outcomes:

• if file became shorter -- request simply hangs for 2 mins and fails with timeout
• if file became longer -- SDK happily sends extra data to WinHTTP, which also happily sends data down the wire, which in turn gets interpreted by server as next (hopefully malformed) request and it drops connection because it is garbage. Looks like another form of buffer overflow attack :). Btw, produced log is completely useless -- I need to install my own hooks into WinHTTP to get the real reason of failure (see attached log). That generic error 99 isn't very helpful. Note that bunch of '?' signs is result of converting unsupported Unicode symbol to ASCII by notepad (basically random memory dump).
  error.log

in my code I ended up working around this issue (and #711) by caching my entire file/chunk in memory via custom streambuf -- effectively freezing it in time on first read.

But, I believe you need to do smth about it. Luckily (as of now) underlying machinery does not multiplex HTTP requests on same connection or you could end up header of next request saved into the previous object (if payload signature is turned off). But this may change in future.

[JonathanHenson]

"incorrect content-length -- I believe this should emulate actual race situation" isn't simulating a race condition, it's simulating a bug in your code.

"if file became shorter -- request simply hangs for 2 mins and fails with timeout"....
that's what happens when you don't write the expected amount of data to a socket.

If you have another process writing to a file while another one is reading it you have a whole other set of problems.

1st, how is any code (not just the SDK) supposed to detect that has happened without murdering performance? How many users WANT the cost of a file system lock? Or the surprising effects of one being there even if they don't mind the cost.

2nd, this is also likely a permissions issue at this point (why do you have multiple processes running under the same user account?). That's a rhetorical question, because I know some of that is Windows' fault.

3rd, there are logical work arounds you can make in your code.... meaning don't just start uploading the file if this is a scenario you need to worry about.... copy it, lock it, or alter the process of your system that is doing this.

4th since there is a logical work around, and the issue would only ever occur in a place where you should be handling it anyways, I don't see justification for the added complexity necessary to handle this case in the SDK code itself. I could entertain the idea of putting filesystem locks in place, but the entire idea of exposing iostream, is that the user gets to make ALL the decisions. And with that the user has responsibility to do the right thing for their application and use case.

As for http, this wouldn't be an issue until Http 2 or http 1.1 pipelining is in play. Even then, unless the last portion of the body is HTTP/1.1\nPOST etc.... it would be considered invalid on the other end.

I'm all for more docs.... I'm also all for more safety. We can look at some doc updates at least.

[JonathanHenson]

Oh wait, it wouldn't be in play at all in an Http 2 scenario since new requests go to a new stream id and that would wind up in a DATA frame anyways . PIpelining would be a potential problem though (but AFAIK doesn't have any public servers supporting pipelining).

[crusader-mike]

isn't simulating a race condition, it's simulating a bug in your code.

According to my reading of SDK code manually setting content-length to incorrect value is equivalent to reproducing situation when file length has changed between determining file size and reading it. You can reproduce it in proper way, it should produce same effect

File lock is a bad idea and since you operate with abstract "stream" provided by user -- you can't do it anyway.

Most logical thing to do is to remember what you put into content-length field and fail if reading from stream produced different number of bytes. After all server on the other end of the pipe expects us to follow our promise made in HTTP header... Or (as said before) at least document additional requirements for user-provided streams.

[JonathanHenson]

"Most logical thing to do is to remember what you put into content-length field and fail if reading from file produced different number of bytes. After all server on the other end of the pipe expects us to follow our promise made in HTTP header... Or (as said before) at least document additional requirements for user-provided streams."

That's a very reasonable suggestion. We actually do that on the response read anyways.

That still doesn't fix your problem though. The data "could" have been mutated.

[JonathanHenson]

"File lock is a bad idea and since you operate with abstract "stream" provided by user -- you can't do it anyway."

Sure you can. We can't (and don't want to). But as the user, if you pass an fstream to the SDK. You can lock it first, and then unlock it when the operation is finished.

[crusader-mike]

That still doesn't fix your problem though. The data "could" have been mutated.

Naturally. But it becomes my problem now, not SDKs -- as in it won't cause sudden disconnect and unusual errors. It will likely produce signature check failures or (if off) inconsistent data stored in S3. Just what you would expect in case of simultaneous file access.

Sure you can. We can't (and don't want to). But as the user, if you pass an fstream to the SDK. You can lock it first, and then unlock it when the operation is finished.

Ah, yes. I misunderstood you. Thought you were taking about SDK locking file behind the stream. :)

[crusader-mike]

@JonathanHenson how about changing status of this issue from "won't fix" to smth like "enchancement"? To implement this at some point:

"Most logical thing to do is to remember what you put into content-length field and fail if reading from file produced different number of bytes. After all server on the other end of the pipe expects us to follow our promise made in HTTP header... Or (as said before) at least document additional requirements for user-provided streams."

That's a very reasonable suggestion. We actually do that on the response read anyways.

[marcomagdy]

I just followed up on the discussion. Thanks for bumping this up. We’ll queue it up.