From bc095c5e311a370bf7152a8589eb9e24b4c1e253 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sat, 5 Sep 2026 13:22:44 -0600 Subject: [PATCH] naett: Stop the transfer when the body writer fails it Found reviewing the commits before this one. On Windows a short write marked the request complete and then queued another read regardless, which is worse than it sounds: the caller is free to close a response the moment it sees complete, so WinHTTP carried on writing into memory that was being freed. It also meant cancelling didn't actually stop anything there. Stop at that point, and ignore anything raised after the request is complete. The Apple callbacks do the same check, so the cancellation we ask for after a failed write doesn't overwrite the error that caused it. naettMake only asserted that its request was non-NULL, and the request constructors do return NULL when a platform can't set one up - an unparseable URL is enough on Windows. Asserts are compiled out in release, so that was a null dereference. It returns NULL now, and HTTPSRequest checks for it and fails the request instead of handing it on. Also: a request cancelled between construction and Start() didn't carry that into the sink, and -1000 - our own cancellation code, not naett's - was logging "Unhandled naett error" on a perfectly normal cancel. --- Common/Net/HTTPNaettRequest.cpp | 34 +++++++++++++++++++++++++++++++-- ext/naett-lib/README-ppsspp.md | 9 +++++++++ ext/naett-lib/src/naett_core.c | 6 ++++++ ext/naett-lib/src/naett_osx.c | 8 +++++++- ext/naett-lib/src/naett_win.c | 15 +++++++++++++++ 5 files changed, 69 insertions(+), 3 deletions(-) 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