[URLSession] Fix for SR-5516 #1121
Conversation
|
@swift-ci please test |
| @@ -495,6 +498,16 @@ extension _HTTPURLProtocol: _EasyHandleDelegate { | |||
| } | |||
| } | |||
|
|
|||
| fileprivate func notifyDelegate(aboutSentData count: Int64) { | |||
There was a problem hiding this comment.
Would it be better for this function's parameter to be Data?
There was a problem hiding this comment.
As the function is specific to upload tasks I wonder if this should be mentioned in the name too.
There was a problem hiding this comment.
@ianpartridge I agree that the name should be better. However, it may not make sense to have Data as a parameter since the delegate call only needs the length of the data that was uploaded. Also, the caller of this method has the data in the form ofDispatchData. I reckon there'll be some unnecessary cost of converting DispatchData to Data.
| @@ -495,6 +498,16 @@ extension _HTTPURLProtocol: _EasyHandleDelegate { | |||
| } | |||
| } | |||
|
|
|||
| fileprivate func notifyDelegate(aboutSentData count: Int64) { | |||
| guard let session = self.task?.session as? URLSession else { fatalError() } | |||
There was a problem hiding this comment.
maybe a better error message in the fatalError()? Otherwise just as! URLSession
TestFoundation/HTTPServer.swift
Outdated
|
|
||
| if uri == "/upload" { | ||
| let text = "Upload completed!" | ||
| return _HTTPResponse(response: .OK, headers: "Content-Length: \(text.characters.count)", body: text) |
There was a problem hiding this comment.
hmm, technically text.characters.count is not the length in bytes. It happens to be for this string in .ascii. but maybe we want to be more correct? text.data(using: .ascii)...
| @@ -127,6 +129,7 @@ fileprivate extension _HTTPURLProtocol { | |||
| set(requestBodyLength: .noBody) | |||
| case (_, .some(let length)): | |||
| set(requestBodyLength: .length(length)) | |||
| bodyLength = Int64(length) | |||
There was a problem hiding this comment.
what happens if an attacker sends a content-length that exceeds Int64 but not UInt64? Could we have a test case for that? I already added a few test cases for a misbehaving server, this one should be really easy to add.
There was a problem hiding this comment.
@weissi I hope I am getting your comment right here. This length isn't something sent by the server. It corresponds to the length of the data being uploaded - it is either the count of a Data in memory or the length of a file being uploaded (see the definitions of _HTTPURLProtocol._Body and _Body.getBodyLength()). A size of more that Int64.max doesn't seem possible then. Even if it were possible, we'd have an OOM before we get here.
There was a problem hiding this comment.
Sorry, ignore me, didn't quite realise this is the upload length rather than the Content-Length header sent by the server. Ignore me please!
pushkarnk
force-pushed
the
uploadtask-delegate
branch
from
f398886 to
91036e2
Compare
|
Thanks for the review comments @ianpartridge and @weissi Can you please take a look at the changes/responses? |
|
@swift-ci please test |
|
@pushkarnk 👍, thanks |
|
@swift-ci please test and merge |
https://bugs.swift.org/browse/SR-5516
Upload tasks must invoke the
urlSession(_:task:didSendBodyData: totalBytesSent:totalBytesExpectedToSend)delegate method every time data is uploaded.