diff --git a/Common/Net/HTTPNaettRequest.cpp b/Common/Net/HTTPNaettRequest.cpp index 5d61427bd0..250b01fa8e 100644 --- a/Common/Net/HTTPNaettRequest.cpp +++ b/Common/Net/HTTPNaettRequest.cpp @@ -89,11 +89,35 @@ void HTTPSRequest::Start() { // Our own writer, so that Cancel() can actually stop a transfer rather than just relabelling // it once it finishes. sink_ = std::make_shared(); + // In case someone managed to cancel us between construction and here. + sink_->cancelled = cancelled_; options.push_back(naettBodyWriter(&HTTPSRequest::WriteBodyThunk, sink_.get())); const naettOption **opts = (const naettOption **)options.data(); req_ = naettRequestWithOptions(url_.c_str(), (int)options.size(), opts); + if (!req_) { + // naett couldn't set the request up - a URL it can't parse, most likely. Fail it here + // rather than handing a null to naettMake. + ERROR_LOG(Log::HTTP, "Couldn't create a request for '%s'", url_.c_str()); + resultCode_ = naettGenericError; + failed_ = true; + completed_ = true; + sink_.reset(); + progress_.Update(0, 0, true); + return; + } res_ = naettMake(req_); + if (!res_) { + ERROR_LOG(Log::HTTP, "Couldn't start a request for '%s'", url_.c_str()); + naettFree(req_); + req_ = nullptr; + resultCode_ = naettGenericError; + failed_ = true; + completed_ = true; + sink_.reset(); + progress_.Update(0, 0, true); + return; + } progress_.Update(0, 0, false); } @@ -129,8 +153,10 @@ void HTTPSRequest::Join() { bool HTTPSRequest::Done() { if (completed_) return true; - - _dbg_assert_(res_ != nullptr); + if (!res_) { + // Never started, or already let go of. Nothing left to wait for. + return true; + } if (!naettComplete(res_)) { int total = 0; @@ -164,6 +190,10 @@ bool HTTPSRequest::Done() { case naettGenericError: // -5 ERROR_LOG(Log::HTTP, "Generic error"); break; + case -1000: + // Ours, not naett's - see above. Cancelling is a normal thing to do, not an error. + INFO_LOG(Log::HTTP, "Request to '%s' cancelled", url_.c_str()); + break; default: ERROR_LOG(Log::HTTP, "Unhandled naett error %d", resultCode_); break; diff --git a/ext/naett-lib/README-ppsspp.md b/ext/naett-lib/README-ppsspp.md index 5f56e0f1a6..b0612eb6fb 100644 --- a/ext/naett-lib/README-ppsspp.md +++ b/ext/naett-lib/README-ppsspp.md @@ -107,3 +107,12 @@ Keep this list up to date - it's what makes it possible to move to a newer upstr the data just keeps arriving. - `naett.h`: documented what the body writer's return value means, since three of the four backends' behaviour depends on it. +- `src/naett_win.c`: a short write from the body writer marked the request complete and then + queued another read anyway, so the transfer carried on and WinHTTP kept writing into a + response the caller was by then free to close. It stops there now, and the callback returns + early for anything raised after the request is complete. +- `src/naett_core.c`: `naettMake` only asserted that the request wasn't NULL, and the request + constructors return NULL when the platform can't set one up - a URL it can't parse, say. In a + release build that walked into a null dereference. Returns NULL instead. +- `src/naett_osx.c`: the delegate callbacks ignore a response that's already complete, so a + cancellation we asked for doesn't overwrite the error that caused it. diff --git a/ext/naett-lib/src/naett_core.c b/ext/naett-lib/src/naett_core.c index 71f5cdaf7a..b6f4efc8c3 100644 --- a/ext/naett-lib/src/naett_core.c +++ b/ext/naett-lib/src/naett_core.c @@ -310,6 +310,12 @@ naettReq* naettRequestWithOptions(const char* url, int numOptions, const naettOp naettRes* naettMake(naettReq* request) { assert(initialized); assert(request != NULL); + // PPSSPP: naettRequest* returns NULL when the platform can't set the request up - a URL it + // can't parse, most likely - and the assert above is compiled out in release, so this used to + // walk straight into a null dereference. + if (request == NULL) { + return NULL; + } InternalRequest* req = (InternalRequest*)request; naettAlloc(InternalResponse, res); diff --git a/ext/naett-lib/src/naett_osx.c b/ext/naett-lib/src/naett_osx.c index 4befc79e20..a583d09057 100644 --- a/ext/naett-lib/src/naett_osx.c +++ b/ext/naett-lib/src/naett_osx.c @@ -100,7 +100,8 @@ void didReceiveData(id self, SEL _sel, id session, id dataTask, id data) { object_getInstanceVariable(self, "response", (void**)&res); // PPSSPP: didComplete already checked this; this one didn't, and the delegate outlives the // response when a session is invalidated. - if (res == NULL) { + if (res == NULL || res->complete) { + // PPSSPP: once complete, the caller may already be closing this - don't touch it further. release(p); return; } @@ -174,6 +175,11 @@ static void didComplete(id self, SEL _sel, id session, id dataTask, id error) { InternalResponse* res = NULL; object_getInstanceVariable(self, "response", (void**)&res); if (res != NULL) { + // PPSSPP: if we already failed the request - a writer that wouldn't take the data, say - + // that's the reason we want reported, not the cancellation it caused. + if (res->complete) { + return; + } if (error != nil) { res->code = naettConnectionError; } diff --git a/ext/naett-lib/src/naett_win.c b/ext/naett-lib/src/naett_win.c index c80b157150..84c38340b6 100644 --- a/ext/naett-lib/src/naett_win.c +++ b/ext/naett-lib/src/naett_win.c @@ -111,6 +111,13 @@ static void CALLBACK callback(HINTERNET request, DWORD_PTR context, DWORD status, LPVOID statusInformation, DWORD statusInfoLength) { InternalResponse* res = (InternalResponse*)context; + // PPSSPP: once we've reported the request as complete the caller is free to close it, so + // don't start any more I/O against it - anything still in flight would be writing into a + // response that's being freed. + if (res->complete) { + return; + } + switch (status) { case WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE: { // PPSSPP: the sizing call only fills in bufSize when it fails with @@ -177,9 +184,17 @@ callback(HINTERNET request, DWORD_PTR context, DWORD status, LPVOID statusInform size_t bytesRead = statusInfoLength; InternalRequest* req = res->request; + if (req == NULL) { + return; + } if (req->options.bodyWriter(res->buffer, (int)bytesRead, req->options.bodyWriterData) != bytesRead) { + // PPSSPP: the writer failed the request - it couldn't grow, or the caller is + // cancelling. Upstream marked it complete and then queued another read anyway, + // which both kept the transfer running and left WinHTTP writing into a response + // the caller was by then free to close. res->code = naettReadError; res->complete = 1; + break; } res->totalBytesRead += (int)bytesRead; // PPSSPP: bytesLeft is unsigned, so a read longer than announced used to wrap it