Merge pull request #22234 from hrydgard/naett-fixes

naett: Fix leaks, undefined behaviour and error handling across all backends

ext/naett-lib/README-ppsspp.md

@@ -33,3 +33,70 @@ Keep this list up to date - it's what makes it possible to move to a newer upstr
 - `src/naett_linux.c`: `curl_easy_setopt` is varargs and takes a `long` for these options;
   upstream passed `int` literals and `int` variables, which is UB on LP64 (and what curl's own
   typecheck macros warn about). They're `1L`/`(long)` now.
+- `src/naett_core.c`: `naettFree` never freed `options.userAgent`, though it's `strdup`'d by
+  the same setter as `method`. Leaked once per request for every caller that sets a user agent,
+  which we do on all of them.
+- `src/naett_core.c`: `defaultBodyWriter` doubled an `int` capacity until it fit, which is signed
+  overflow on a large response, and used the `realloc` result without checking it - losing the
+  old pointer and then `memcpy`ing through NULL. Grows in `int64_t` against `INT_MAX` and
+  reports failure by returning short, which every caller already treats as an error.
+- `src/naett_linux.c`: `headerCallback` only handed its `strndup` to the header list when the
+  line had a colon, and leaked it otherwise. curl passes the status line and the blank line that
+  ends the header block, so that leaked at least twice per response.
+- `src/naett_osx.c`: the delegate class was built with `objc_allocateClassPair` and then used
+  without ever calling `objc_registerClassPair`, which the runtime requires before the class can
+  be instantiated. Registered now, after the methods and ivar are added.
+- `src/naett_osx.c`: the response header arrays were VLAs sized from the server's header count -
+  unbounded stack use from network data, and a zero-length VLA when a response had no headers.
+  They're heap allocations now, skipped entirely when there are none.
+- `src/naett_osx.c`: the `NSURLSession` was stored in the response without a `retain`, though the
+  autorelease pool it came from is drained before returning. Retained, and released in
+  `naettPlatformCloseResponse`.
+- `src/naett_objc.h`: `addMethod`/`addIvar` reported failure with `assert` only, so in release a
+  delegate could silently come up without its methods. They print as well now.
+- `src/naett_win.c`: the header sizing call only reports a size when it fails with
+  `ERROR_INSUFFICIENT_BUFFER`; on any other failure the size stayed zero and `unpackHeaders` ran
+  `wcslen` over a `malloc(0)` block. Checked, and the second query's result is checked too.
+- `src/naett_win.c`: `winToUTF8`/`winFromUTF8`/`wcsndup` could all return NULL and every caller
+  used the result unchecked - `packHeaders` most visibly, whose result is indexed as
+  `headers[0]`. All checked now.
+- `src/naett_win.c`: `res->bytesLeft` is unsigned, so a read longer than the announced count
+  wrapped it into an enormous value and kept the read loop running.
+- `src/naett_linux.c`: the worker read the queued `CURL*` out of the pipe into the start of its
+  buffer while tracking a fill position, so a short read would have resumed mid-pointer and
+  handed curl a mangled handle. Only ever safe because a write that size to a pipe is atomic.
+- `src/naett_linux.c`: the easy handle is removed from the multi before being cleaned up, which
+  is what curl asks for. `curl_multi_remove_handle` and `curl_multi_cleanup` were added to the
+  dlopen table in `naett_curl.h`/`naett_curl.c` for this.
+- `src/naett_linux.c`: `workerRunning` is written by the worker and read by the request path, so
+  it's an `atomic_int` now rather than a plain `int`.
+- `src/naett_linux.c`: the `write` that hands a request to the worker was unchecked - a failed
+  one meant a request that never ran and never completed, so the caller polled `naettComplete`
+  forever. Also retries on `EINTR`, and `curl_easy_init` failure is handled.
+- `src/naett_linux.c`: the read and write callbacks returned the body callbacks' `int` straight
+  to curl, which reads it as a `size_t` - a negative arrived as an enormous count instead of an
+  error.
+- `src/naett_linux.c`: the multi handle and the pipe leaked when init failed partway.
+- `src/naett_android.c`: `getEnv` called `AttachCurrentThread` and nothing ever detached.
+  `processRequest` detaches its own thread, but `naettPlatformInitRequest`/`FreeRequest` run on
+  the caller's, and a thread that exits while attached is fatal on Android. They attach only if
+  the thread wasn't already, and detach when they're done.
+- `src/naett_android.c`: `pthread_create`'s result was ignored - with no worker, nothing sets
+  `complete` and the caller polls `naettComplete` forever.
+- `src/naett_android.c`: `getOutputStream` can throw, and the calls after it ran with the
+  exception still pending, which isn't allowed for most of JNI. Checked now.
+- `src/naett_android.c`: `GetMethodID` returns NULL for a method it can't find, and calling with
+  a NULL `jmethodID` aborts the VM; the header loop could also hand `GetStringUTFChars` a null
+  value for a header with no entries.
+- `src/naett_core.c`: `naettClose` cleared `res->request` before calling the backend's close,
+  which is the one thing the backend needs - the WinHTTP handles hang off the request. The
+  backend goes first now.
+- `naett.h`: documented that a response should be complete before it's closed. Only the Android
+  backend really cancels and waits.
+- `src/naett_win.c`: `naettPlatformCloseResponse` was empty, so the status callback kept the
+  freed response as its context. Unhooks the callback and closes the request handle. Not a full
+  cancel - a callback already running isn't waited for, which would need the
+  `WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING` handshake.
+- `src/naett_osx.c`: `invalidateAndCancel` returns before the session lets go of its delegate, so
+  the delegate's back pointer to the response is cleared first, and `didReceiveData` checks it
+  (as `didCompleteWithError` already did).

ext/naett-lib/naett.h

@@ -131,6 +131,12 @@ naettReq* naettGetRequest(naettRes* response);
 
 /**
  * @brief Closes a response object.
+ *
+ * PPSSPP: the response should be complete (see `naettComplete`) before closing it. Only the
+ * Android backend actually cancels an in-flight request and waits for its worker; the others
+ * stop what they can and return, so a transfer already inside a callback on another thread can
+ * still be running as this returns. Closing an incomplete response is therefore not safe in
+ * general - wait for it, or let it leak, which is what we do on shutdown.
  */
 void naettClose(naettRes* response);
 

ext/naett-lib/src/naett_android.c

@@ -31,13 +31,36 @@ static JavaVM* getVM() {
     return globalVM;
 }
 
-static JNIEnv* getEnv() {
+// PPSSPP: AttachCurrentThread on an already-attached thread is a no-op that still hands back the
+// env, so report whether we're the ones who attached it. A thread that exits while attached is
+// fatal on Android ("Native thread exiting without having called DetachCurrentThread"), and
+// naettPlatformInitRequest/FreeRequest run on whichever thread the caller uses.
+static JNIEnv* getEnvAttached(int* attached) {
     JavaVM* vm = getVM();
-    JNIEnv* env;
-    (*vm)->AttachCurrentThread(vm, &env, NULL);
+    JNIEnv* env = NULL;
+    *attached = 0;
+    if ((*vm)->GetEnv(vm, (void**)&env, JNI_VERSION_1_6) == JNI_OK) {
+        return env;
+    }
+    if ((*vm)->AttachCurrentThread(vm, &env, NULL) != JNI_OK) {
+        return NULL;
+    }
+    *attached = 1;
     return env;
 }
 
+static void detachIfAttached(int attached) {
+    if (attached) {
+        JavaVM* vm = getVM();
+        (*vm)->DetachCurrentThread(vm);
+    }
+}
+
+static JNIEnv* getEnv() {
+    int attached = 0;
+    return getEnvAttached(&attached);
+}
+
 static int catch (JNIEnv* env) {
     int thrown = (*env)->ExceptionCheck(env);
     if (thrown) {
@@ -47,14 +70,23 @@ static int catch (JNIEnv* env) {
 }
 
 static jmethodID getMethod(JNIEnv* env, jobject instance, const char* method, const char* sig) {
+    if (instance == NULL) {
+        return NULL;
+    }
     jclass clazz = (*env)->GetObjectClass(env, instance);
     jmethodID id = (*env)->GetMethodID(env, clazz, method, sig);
     (*env)->DeleteLocalRef(env, clazz);
     return id;
 }
 
+// PPSSPP: GetMethodID leaves an exception pending and returns NULL when it can't find the
+// method, and calling with a NULL jmethodID aborts the VM. Bail out instead - the caller's
+// catch() picks up the pending exception.
 static jobject call(JNIEnv* env, jobject instance, const char* method, const char* sig, ...) {
     jmethodID methodID = getMethod(env, instance, method, sig);
+    if (methodID == NULL) {
+        return NULL;
+    }
     va_list args;
     va_start(args, sig);
     jobject result = (*env)->CallObjectMethodV(env, instance, methodID, args);
@@ -64,6 +96,9 @@ static jobject call(JNIEnv* env, jobject instance, const char* method, const cha
 
 static void voidCall(JNIEnv* env, jobject instance, const char* method, const char* sig, ...) {
     jmethodID methodID = getMethod(env, instance, method, sig);
+    if (methodID == NULL) {
+        return;
+    }
     va_list args;
     va_start(args, sig);
     (*env)->CallVoidMethodV(env, instance, methodID, args);
@@ -72,6 +107,9 @@ static void voidCall(JNIEnv* env, jobject instance, const char* method, const ch
 
 static jint intCall(JNIEnv* env, jobject instance, const char* method, const char* sig, ...) {
     jmethodID methodID = getMethod(env, instance, method, sig);
+    if (methodID == NULL) {
+        return 0;
+    }
     va_list args;
     va_start(args, sig);
     jint result = (*env)->CallIntMethodV(env, instance, methodID, args);
@@ -84,7 +122,11 @@ void naettPlatformInit(naettInitData initData) {
 }
 
 int naettPlatformInitRequest(InternalRequest* req) {
-    JNIEnv* env = getEnv();
+    int attached = 0;
+    JNIEnv* env = getEnvAttached(&attached);
+    if (env == NULL) {
+        return 0;
+    }
     (*env)->PushLocalFrame(env, 10);
     jclass URL = (*env)->FindClass(env, "java/net/URL");
     jmethodID newURL = (*env)->GetMethodID(env, URL, "<init>", "(Ljava/lang/String;)V");
@@ -92,10 +134,12 @@ int naettPlatformInitRequest(InternalRequest* req) {
     jobject url = (*env)->NewObject(env, URL, newURL, urlString);
     if (catch (env)) {
         (*env)->PopLocalFrame(env, NULL);
+        detachIfAttached(attached);
         return 0;
     }
     req->urlObject = (*env)->NewGlobalRef(env, url);
     (*env)->PopLocalFrame(env, NULL);
+    detachIfAttached(attached);
     return 1;
 }
 
@@ -137,6 +181,12 @@ static void* processRequest(void* data) {
         strcmp(req->options.method, "PATCH") == 0 || strcmp(req->options.method, "DELETE") == 0) {
         voidCall(env, connection, "setDoOutput", "(Z)V", 1);
         outputStream = call(env, connection, "getOutputStream", "()Ljava/io/OutputStream;");
+        // PPSSPP: most JNI calls are not safe to make with an exception pending, and this one
+        // throws for anything from a refused connection to a protocol the server won't take.
+        if (catch (env)) {
+            res->code = naettConnectionError;
+            goto finally;
+        }
     }
     jobject methodString = (*env)->NewStringUTF(env, req->options.method);
     voidCall(env, connection, "setRequestMethod", "(Ljava/lang/String;)V", methodString);
@@ -182,11 +232,25 @@ static void* processRequest(void* data) {
         if (name == NULL) {
             continue;
         }
-        const char* nameString = (*env)->GetStringUTFChars(env, name, NULL);
-
         jobject values = call(env, headerMap, "get", "(Ljava/lang/Object;)Ljava/lang/Object;", name);
         jstring value = call(env, values, "get", "(I)Ljava/lang/Object;", 0);
+        // PPSSPP: a header with no values gives a null here, and GetStringUTFChars on it aborts.
+        if (value == NULL) {
+            (*env)->ExceptionClear(env);
+            (*env)->DeleteLocalRef(env, name);
+            (*env)->DeleteLocalRef(env, values);
+            continue;
+        }
+        const char* nameString = (*env)->GetStringUTFChars(env, name, NULL);
         const char* valueString = (*env)->GetStringUTFChars(env, value, NULL);
+        if (nameString == NULL || valueString == NULL) {
+            if (nameString) (*env)->ReleaseStringUTFChars(env, name, nameString);
+            if (valueString) (*env)->ReleaseStringUTFChars(env, value, valueString);
+            (*env)->DeleteLocalRef(env, name);
+            (*env)->DeleteLocalRef(env, value);
+            (*env)->DeleteLocalRef(env, values);
+            continue;
+        }
 
         naettAlloc(KVLink, node);
         node->key = strdup(nameString);
@@ -255,8 +319,17 @@ static void startWorkerThread(InternalResponse* res) {
     pthread_attr_t attr;
     pthread_attr_init(&attr);
     pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
-    pthread_create(&res->workerThread, &attr, processRequest, res);
-    pthread_setname_np(res->workerThread, "naett worker thread");
+    // PPSSPP: upstream ignored this. With no thread, nothing ever sets res->complete and the
+    // caller polls naettComplete forever.
+    if (pthread_create(&res->workerThread, &attr, processRequest, res) != 0) {
+        LOGE("Failed to start the request worker thread");
+        res->workerThread = 0;
+        res->code = naettGenericError;
+        res->complete = 1;
+    } else {
+        pthread_setname_np(res->workerThread, "naett worker thread");
+    }
+    pthread_attr_destroy(&attr);
 }
 
 void naettPlatformMakeRequest(InternalResponse* res) {
@@ -264,8 +337,18 @@ void naettPlatformMakeRequest(InternalResponse* res) {
 }
 
 void naettPlatformFreeRequest(InternalRequest* req) {
-    JNIEnv* env = getEnv();
+    if (req->urlObject == NULL) {
+        // Init failed before it got that far.
+        return;
+    }
+    int attached = 0;
+    JNIEnv* env = getEnvAttached(&attached);
+    if (env == NULL) {
+        return;
+    }
     (*env)->DeleteGlobalRef(env, req->urlObject);
+    req->urlObject = NULL;
+    detachIfAttached(attached);
 }
 
 void naettPlatformCloseResponse(InternalResponse* res) {

ext/naett-lib/src/naett_core.c

@@ -1,6 +1,8 @@
 #include "naett_internal.h"
 #include <stdarg.h>
 #include <stdlib.h>
+#include <stdint.h>
+#include <limits.h>
 #include <stddef.h>
 #include <string.h>
 #include <assert.h>
@@ -81,18 +83,34 @@ static int defaultBodyReader(void* dest, int bufferSize, void* userData) {
     return bytesToRead;
 }
 
+// PPSSPP: `bytes` and the resulting capacity come off the network, so the growth has to be
+// done in a type that can't wrap into a negative capacity, and a failed realloc has to leave
+// the buffer we already have intact. Returning short is how this reports failure - every
+// caller compares the result against what it asked to write and errors the request out.
 static int defaultBodyWriter(const void* source, int bytes, void* userData) {
     Buffer* buffer = (Buffer*) userData;
-    int newCapacity = buffer->capacity;
-    if (newCapacity == 0) {
-        newCapacity = bytes;
+    if (bytes <= 0) {
+        return 0;
     }
-    while (newCapacity - buffer->size < bytes) {
-        newCapacity *= 2;
-    }
-    if (newCapacity != buffer->capacity) {
-        buffer->data = realloc(buffer->data, newCapacity);
-        buffer->capacity = newCapacity;
+    if (buffer->capacity - buffer->size < bytes) {
+        int64_t newCapacity = buffer->capacity > 0 ? buffer->capacity : bytes;
+        while (newCapacity - buffer->size < bytes) {
+            newCapacity *= 2;
+            if (newCapacity > INT_MAX) {
+                newCapacity = INT_MAX;
+                break;
+            }
+        }
+        if (newCapacity - buffer->size < bytes) {
+            // Doesn't fit in the int sizes this struct uses.
+            return 0;
+        }
+        void* newData = realloc(buffer->data, (size_t)newCapacity);
+        if (newData == NULL) {
+            return 0;
+        }
+        buffer->data = newData;
+        buffer->capacity = (int)newCapacity;
     }
     char* dest = ((char*)buffer->data) + buffer->size;
     memcpy(dest, source, bytes);
@@ -388,6 +406,8 @@ void naettFree(naettReq* request) {
     KVLink* node = req->options.headers;
     freeKVList(node);
     free((void*)req->options.method);
+    // PPSSPP: userAgent is strdup'd by stringSetter like method is, and was never freed.
+    free((void*)req->options.userAgent);
     free((void*)req->url);
     free(request);
 }
@@ -396,8 +416,10 @@ void naettClose(naettRes* response) {
     assert(response != NULL);
 
     InternalResponse* res = (InternalResponse*)response;
-    res->request = NULL;
+    // PPSSPP: the backends need the request to shut a response down - it owns the handles on
+    // Windows, for one - so clear it after they've had their turn, not before.
     naettPlatformCloseResponse(res);
+    res->request = NULL;
     KVLink* node = res->headers;
     freeKVList(node);
     free(res->body.data);

ext/naett-lib/src/naett_curl.c

@@ -37,7 +37,9 @@ static int LoadSymbols(void *lib) {
 	LOAD(easy_getinfo, "curl_easy_getinfo")
 	LOAD(easy_cleanup, "curl_easy_cleanup")
 	LOAD(multi_init, "curl_multi_init")
+	LOAD(multi_cleanup, "curl_multi_cleanup")
 	LOAD(multi_add_handle, "curl_multi_add_handle")
+	LOAD(multi_remove_handle, "curl_multi_remove_handle")
 	LOAD(multi_perform, "curl_multi_perform")
 	LOAD(multi_info_read, "curl_multi_info_read")
 	LOAD(multi_wait, "curl_multi_wait")

ext/naett-lib/src/naett_curl.h

@@ -46,7 +46,9 @@ typedef struct {
 	void (*easy_cleanup)(CURL *handle);
 
 	CURLM *(*multi_init)(void);
+	CURLMcode (*multi_cleanup)(CURLM *multi);
 	CURLMcode (*multi_add_handle)(CURLM *multi, CURL *easy);
+	CURLMcode (*multi_remove_handle)(CURLM *multi, CURL *easy);
 	CURLMcode (*multi_perform)(CURLM *multi, int *runningHandles);
 	CURLMsg *(*multi_info_read)(CURLM *multi, int *msgsInQueue);
 	CURLMcode (*multi_wait)(CURLM *multi, struct curl_waitfd extraFDs[], unsigned int extraNFDs, int timeoutMS,
@@ -64,7 +66,9 @@ extern NaettCurl g_curl;
 #define curl_easy_getinfo g_curl.easy_getinfo
 #define curl_easy_cleanup g_curl.easy_cleanup
 #define curl_multi_init g_curl.multi_init
+#define curl_multi_cleanup g_curl.multi_cleanup
 #define curl_multi_add_handle g_curl.multi_add_handle
+#define curl_multi_remove_handle g_curl.multi_remove_handle
 #define curl_multi_perform g_curl.multi_perform
 #define curl_multi_info_read g_curl.multi_info_read
 #define curl_multi_wait g_curl.multi_wait

ext/naett-lib/src/naett_linux.c

@@ -14,6 +14,8 @@
 #include <string.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <stdatomic.h>
+#include <errno.h>
 
 static pthread_t workerThread;
 static int handleReadFD = 0;
@@ -21,8 +23,9 @@ static int handleWriteFD = 0;
 
 // PPSSPP: upstream had a panic() here that called exit(1). Taking the whole application
 // down because a download failed isn't acceptable for us, so failures now leave the
-// backend disabled and requests complete with naettGenericError instead.
-static int workerRunning = 0;
+// backend disabled and requests complete with naettGenericError instead. Atomic because
+// the worker writes it while the request path reads it.
+static atomic_int workerRunning = 0;
 
 static void fail(const char* message) {
     fprintf(stderr, "naett: %s\n", message);
@@ -62,6 +65,8 @@ static void* curlWorker(void* data) {
             curl_easy_getinfo(handle, CURLINFO_RESPONSE_CODE, &responseCode);
             res->code = (int)responseCode;
             res->complete = 1;
+            // PPSSPP: curl wants the easy handle out of the multi before it's cleaned up.
+            curl_multi_remove_handle(mc, handle);
             curl_easy_cleanup(handle);
         }
 
@@ -73,7 +78,10 @@ static void* curlWorker(void* data) {
             usleep(100 * 1000);
         }
 
-        int bytesRead = read(handleReadFD, newHandle.buf, sizeof(newHandle.buf) - newHandlePos);
+        // PPSSPP: upstream always read into the start of the buffer while tracking a position,
+        // so a short read would have restarted mid-pointer and handed curl a mangled handle. A
+        // write of this size to a pipe is atomic, which is the only reason it never bit.
+        int bytesRead = read(handleReadFD, newHandle.buf + newHandlePos, sizeof(newHandle.buf) - newHandlePos);
         if (bytesRead > 0) {
             newHandlePos += bytesRead;
         }
@@ -99,6 +107,7 @@ void naettPlatformInit(naettInitData initData) {
     int fds[2];
     if (pipe(fds) != 0) {
         fail("failed to open pipe");
+        curl_multi_cleanup(mc);
         return;
     }
     handleReadFD = fds[0];
@@ -113,25 +122,38 @@ void naettPlatformInit(naettInitData initData) {
     workerRunning = 1;
     if (pthread_create(&workerThread, &attr, curlWorker, mc) != 0) {
         fail("failed to start the HTTP worker thread");
+        // PPSSPP: nothing owns any of this now that there's no worker.
+        close(handleReadFD);
+        close(handleWriteFD);
+        handleReadFD = handleWriteFD = -1;
+        curl_multi_cleanup(mc);
     }
+    pthread_attr_destroy(&attr);
 }
 
 int naettPlatformInitRequest(InternalRequest* req) {
     return 1;
 }
 
+// PPSSPP: the body callbacks return int, and curl reads the result as a size_t - so a negative
+// return used to come through as an enormous count rather than as the error it is. Returning
+// something other than what curl asked for aborts the transfer, which is what we want.
 static size_t readCallback(char* buffer, size_t size, size_t numItems, void* userData) {
     InternalResponse* res = (InternalResponse*)userData;
     InternalRequest* req = res->request;
-    return req->options.bodyReader(buffer, size * numItems, req->options.bodyReaderData);
+    int bytesRead = req->options.bodyReader(buffer, (int)(size * numItems), req->options.bodyReaderData);
+    return bytesRead > 0 ? (size_t)bytesRead : 0;
 }
 
 static size_t writeCallback(char* ptr, size_t size, size_t numItems, void* userData) {
     InternalResponse* res = (InternalResponse*)userData;
     InternalRequest* req = res->request;
-    size_t bytesWritten = req->options.bodyWriter(ptr, size * numItems, req->options.bodyWriterData);
+    int bytesWritten = req->options.bodyWriter(ptr, (int)(size * numItems), req->options.bodyWriterData);
+    if (bytesWritten <= 0) {
+        return 0;
+    }
     res->totalBytesRead += bytesWritten;
-    return bytesWritten;
+    return (size_t)bytesWritten;
 }
 
 #define METHOD(A, B, C) (((A) << 16) | ((B) << 8) | (C))
@@ -171,6 +193,9 @@ static size_t headerCallback(char* buffer, size_t size, size_t nitems, void* use
     size_t headerSize = size * nitems;
 
     char* headerName = strndup(buffer, headerSize);
+    if (headerName == NULL) {
+        return headerSize;
+    }
     char* split = strchr(headerName, ':');
     if (split) {
         *split = 0;
@@ -195,6 +220,11 @@ static size_t headerCallback(char* buffer, size_t size, size_t nitems, void* use
         node->key = headerName;
         node->value = headerValue;
         res->headers = node;
+    } else {
+        // PPSSPP: no colon, so the list never takes ownership of this copy. curl hands us the
+        // status line and the blank line that terminates the header block, neither of which has
+        // one - so upstream leaked at least twice per response, more with redirects.
+        free(headerName);
     }
 
     return headerSize;
@@ -212,6 +242,11 @@ void naettPlatformMakeRequest(InternalResponse* res) {
     }
 
     CURL* c = curl_easy_init();
+    if (c == NULL) {
+        res->code = naettGenericError;
+        res->complete = 1;
+        return;
+    }
     curl_easy_setopt(c, CURLOPT_URL, req->url);
     curl_easy_setopt(c, CURLOPT_CONNECTTIMEOUT_MS, (long)req->options.timeoutMS);
 
@@ -255,7 +290,21 @@ void naettPlatformMakeRequest(InternalResponse* res) {
 
     curl_easy_setopt(c, CURLOPT_PRIVATE, res);
 
-    write(handleWriteFD, &c, sizeof(c));
+    // PPSSPP: this is the only thing that hands the request to the worker. Upstream ignored the
+    // result, so a failed write meant a request that never ran and never completed - the caller
+    // then waits on naettComplete forever.
+    ssize_t written = write(handleWriteFD, &c, sizeof(c));
+    while (written < 0 && errno == EINTR) {
+        written = write(handleWriteFD, &c, sizeof(c));
+    }
+    if (written != (ssize_t)sizeof(c)) {
+        fprintf(stderr, "naett: couldn't queue a request for the HTTP worker\n");
+        curl_slist_free_all(headerList);
+        res->headerList = NULL;
+        curl_easy_cleanup(c);
+        res->code = naettGenericError;
+        res->complete = 1;
+    }
 }
 
 void naettPlatformFreeRequest(InternalRequest* req) {

ext/naett-lib/src/naett_objc.h

@@ -4,6 +4,7 @@
 #if defined(__IOS__) || defined (__MACOS__)
 #include <assert.h>
 #include <math.h>
+#include <stdio.h>
 
 #include <objc/NSObjCRuntime.h>
 #include <objc/message.h>
@@ -42,11 +43,20 @@
 // Check here to get the signature right:
 // https://nshipster.com/type-encodings/
 // https://ko9.org/posts/encode-types/
-#define addMethod(CLASS, NAME, IMPL, SIGNATURE) \
-    if (!class_addMethod(CLASS, sel(NAME), (IMP) (IMPL), (SIGNATURE))) assert(false)
+// PPSSPP: these used to report failure with assert() alone, which is compiled out in release -
+// so a delegate that failed to pick up its methods would just never receive data, and the
+// request would hang with no clue why. Still asserts in debug, says so either way.
+#define addMethod(CLASS, NAME, IMPL, SIGNATURE)                                \
+    if (!class_addMethod(CLASS, sel(NAME), (IMP)(IMPL), (SIGNATURE))) {        \
+        fprintf(stderr, "naett: failed to add method %s\n", (NAME));           \
+        assert(false);                                                         \
+    }
 
-#define addIvar(CLASS, NAME, SIZE, SIGNATURE) \
-    if (!class_addIvar(CLASS, NAME, SIZE, rint(log2(SIZE)), SIGNATURE)) assert(false)
+#define addIvar(CLASS, NAME, SIZE, SIGNATURE)                                  \
+    if (!class_addIvar(CLASS, NAME, SIZE, rint(log2(SIZE)), SIGNATURE)) {      \
+        fprintf(stderr, "naett: failed to add ivar %s\n", (NAME));             \
+        assert(false);                                                         \
+    }
 
 #define objc_alloc(CLASS) objc_msgSend_id(class(CLASS), sel("alloc"))
 #define autorelease(OBJ) objc_msgSend_void(OBJ, sel("autorelease"))

ext/naett-lib/src/naett_osx.c

@@ -98,6 +98,12 @@ void didReceiveData(id self, SEL _sel, id session, id dataTask, id data) {
     id p = pool();
 
     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) {
+        release(p);
+        return;
+    }
 
     if (res->headers == NULL) {
         id response = objc_msgSend_t(id)(dataTask, sel("response"));
@@ -105,18 +111,31 @@ void didReceiveData(id self, SEL _sel, id session, id dataTask, id data) {
         id allHeaders = objc_msgSend_t(id)(response, sel("allHeaderFields"));
 
         NSUInteger headerCount = objc_msgSend_t(NSUInteger)(allHeaders, sel("count"));
-        id headerNames[headerCount];
-        id headerValues[headerCount];
-
-        objc_msgSend_t(NSInteger, id*, id*, NSUInteger)(
-            allHeaders, sel("getObjects:andKeys:count:"), headerValues, headerNames, headerCount);
+        // PPSSPP: this was a pair of VLAs sized straight from the response, so a server with
+        // enough headers ran the stack out - and a response with none at all is a zero-length
+        // VLA, which is undefined on its own. Heap, and nothing to do when there are none.
         KVLink* firstHeader = NULL;
-        for (int i = 0; i < headerCount; i++) {
-            naettAlloc(KVLink, node);
-            node->key = strdup(objc_msgSend_t(const char*)(headerNames[i], sel("UTF8String")));
-            node->value = strdup(objc_msgSend_t(const char*)(headerValues[i], sel("UTF8String")));
-            node->next = firstHeader;
-            firstHeader = node;
+        if (headerCount > 0) {
+            id* headerNames = (id*)calloc(headerCount, sizeof(id));
+            id* headerValues = (id*)calloc(headerCount, sizeof(id));
+            if (headerNames != NULL && headerValues != NULL) {
+                objc_msgSend_t(NSInteger, id*, id*, NSUInteger)(
+                    allHeaders, sel("getObjects:andKeys:count:"), headerValues, headerNames, headerCount);
+                for (NSUInteger i = 0; i < headerCount; i++) {
+                    const char* key = objc_msgSend_t(const char*)(headerNames[i], sel("UTF8String"));
+                    const char* value = objc_msgSend_t(const char*)(headerValues[i], sel("UTF8String"));
+                    if (key == NULL || value == NULL) {
+                        continue;
+                    }
+                    naettAlloc(KVLink, node);
+                    node->key = strdup(key);
+                    node->value = strdup(value);
+                    node->next = firstHeader;
+                    firstHeader = node;
+                }
+            }
+            free(headerNames);
+            free(headerValues);
         }
         res->headers = firstHeader;
 
@@ -126,10 +145,15 @@ void didReceiveData(id self, SEL _sel, id session, id dataTask, id data) {
         }
     }
 
+    if (res->request == NULL) {
+        release(p);
+        return;
+    }
+
     const void* bytes = objc_msgSend_t(const void*)(data, sel("bytes"));
     NSUInteger length = objc_msgSend_t(NSUInteger)(data, sel("length"));
 
-    res->request->options.bodyWriter(bytes, length, res->request->options.bodyWriterData);
+    res->request->options.bodyWriter(bytes, (int)length, res->request->options.bodyWriterData);
     res->totalBytesRead += (int)length;
 
     release(p);
@@ -156,6 +180,10 @@ static id createDelegate(void) {
         addMethod(TaskDelegateClass, "URLSession:dataTask:didReceiveData:", didReceiveData, "v@:@@@");
         addMethod(TaskDelegateClass, "URLSession:task:didCompleteWithError:", didComplete, "v@:@@@");
         addIvar(TaskDelegateClass, "response", sizeof(void*), "^v");
+        // PPSSPP: a class from objc_allocateClassPair isn't usable until it's registered, and
+        // upstream never did - it went straight to +alloc. Methods, protocols and ivars all have
+        // to be added before this call, which is why it comes last.
+        objc_registerClassPair(TaskDelegateClass);
     }
 
     id delegate = objc_msgSend_id((id)TaskDelegateClass, sel("alloc"));
@@ -179,6 +207,10 @@ void naettPlatformMakeRequest(InternalResponse* res) {
         delegate,
         nil);
 
+    // PPSSPP: sessionWithConfiguration: hands back an autoreleased session, and the pool below
+    // is drained before we return. It survived on NSURLSession keeping itself alive while tasks
+    // run, but we hold the pointer until naettClose, so hold a reference to go with it.
+    retain(session);
     res->session = session;
 
     id task = objc_msgSend_t(id, id)(session, sel("dataTaskWithRequest:"), req->urlRequest);
@@ -193,7 +225,18 @@ void naettPlatformFreeRequest(InternalRequest* req) {
 }
 
 void naettPlatformCloseResponse(InternalResponse* res) {
+    if (res->session == nil) {
+        return;
+    }
+    // PPSSPP: invalidateAndCancel returns before the session is done with its delegate, so clear
+    // the back pointer the delegate holds - a callback that lands afterwards then sees NULL
+    // rather than a freed response.
+    id delegate = objc_msgSend_t(id)(res->session, sel("delegate"));
+    if (delegate != nil) {
+        object_setInstanceVariable(delegate, "response", NULL);
+    }
     objc_msgSend_void(res->session, sel("invalidateAndCancel"));
+    release(res->session);
     res->session = nil;
 }
 

ext/naett-lib/src/naett_win.c

@@ -14,7 +14,13 @@ void naettPlatformInit(naettInitData initData) {
 
 static char* winToUTF8(LPWSTR source) {
     int length = WideCharToMultiByte(CP_UTF8, 0, source, -1, NULL, 0, NULL, NULL);
+    if (length <= 0) {
+        return NULL;
+    }
     char* chars = (char*)malloc(length);
+    if (chars == NULL) {
+        return NULL;
+    }
     int result = WideCharToMultiByte(CP_UTF8, 0, source, -1, chars, length, NULL, NULL);
     if (!result) {
         free(chars);
@@ -25,7 +31,13 @@ static char* winToUTF8(LPWSTR source) {
 
 static LPWSTR winFromUTF8(const char* source) {
     int length = MultiByteToWideChar(CP_UTF8, 0, source, -1, NULL, 0);
+    if (length <= 0) {
+        return NULL;
+    }
     LPWSTR chars = (LPWSTR)malloc(length * sizeof(WCHAR));
+    if (chars == NULL) {
+        return NULL;
+    }
     int result = MultiByteToWideChar(CP_UTF8, 0, source, -1, chars, length);
     if (!result) {
         free(chars);
@@ -43,6 +55,9 @@ static LPWSTR winFromUTF8(const char* source) {
 
 static LPWSTR wcsndup(LPCWSTR str, size_t len) {
     LPWSTR result = calloc(1, sizeof(WCHAR) * (len + 1));
+    if (result == NULL) {
+        return NULL;
+    }
     wcsncpy(result, str, len);
     return result;
 }
@@ -69,6 +84,10 @@ static void unpackHeaders(InternalResponse* res, LPWSTR packed) {
     KVLink* firstHeader = NULL;
     while ((len = wcslen(packed)) != 0) {
         char* header = winToUTF8(packed);
+        if (header == NULL) {
+            packed += len + 1;
+            continue;
+        }
         char* split = strchr(header, ':');
         if (split) {
             *split = 0;
@@ -94,6 +113,9 @@ callback(HINTERNET request, DWORD_PTR context, DWORD status, LPVOID statusInform
 
     switch (status) {
         case WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE: {
+            // PPSSPP: the sizing call only fills in bufSize when it fails with
+            // ERROR_INSUFFICIENT_BUFFER. Any other failure left it at zero, and unpackHeaders
+            // then ran wcslen over a malloc(0) block.
             DWORD bufSize = 0;
             WinHttpQueryHeaders(request,
                 WINHTTP_QUERY_RAW_HEADERS,
@@ -101,15 +123,18 @@ callback(HINTERNET request, DWORD_PTR context, DWORD status, LPVOID statusInform
                 NULL,
                 &bufSize,
                 WINHTTP_NO_HEADER_INDEX);
-            LPWSTR buffer = (LPWSTR)malloc(bufSize);
-            WinHttpQueryHeaders(request,
-                WINHTTP_QUERY_RAW_HEADERS,
-                WINHTTP_HEADER_NAME_BY_INDEX,
-                buffer,
-                &bufSize,
-                WINHTTP_NO_HEADER_INDEX);
-            unpackHeaders(res, buffer);
-            free(buffer);
+            if (bufSize >= sizeof(WCHAR)) {
+                LPWSTR buffer = (LPWSTR)calloc(1, bufSize + sizeof(WCHAR));
+                if (buffer != NULL && WinHttpQueryHeaders(request,
+                        WINHTTP_QUERY_RAW_HEADERS,
+                        WINHTTP_HEADER_NAME_BY_INDEX,
+                        buffer,
+                        &bufSize,
+                        WINHTTP_NO_HEADER_INDEX)) {
+                    unpackHeaders(res, buffer);
+                }
+                free(buffer);
+            }
 
             const char* contentLength = naettGetHeader((naettRes*)res, "Content-Length");
             if (!contentLength || sscanf(contentLength, "%d", &res->contentLength) != 1) {
@@ -157,7 +182,9 @@ callback(HINTERNET request, DWORD_PTR context, DWORD status, LPVOID statusInform
                 res->complete = 1;
             }
             res->totalBytesRead += (int)bytesRead;
-            res->bytesLeft -= bytesRead;
+            // PPSSPP: bytesLeft is unsigned, so a read longer than announced used to wrap it
+            // into an enormous count and keep the read loop going.
+            res->bytesLeft = bytesRead >= res->bytesLeft ? 0 : res->bytesLeft - bytesRead;
             if (res->bytesLeft > 0) {
                 size_t bytesToRead = min(res->bytesLeft, sizeof(res->buffer));
                 if (!WinHttpReadData(request, res->buffer, (DWORD)bytesToRead, NULL)) {
@@ -212,6 +239,9 @@ callback(HINTERNET request, DWORD_PTR context, DWORD status, LPVOID statusInform
 
 int naettPlatformInitRequest(InternalRequest* req) {
     LPWSTR url = winFromUTF8(req->url);
+    if (url == NULL) {
+        return 0;
+    }
 
     URL_COMPONENTS components;
     ZeroMemory(&components, sizeof(components));
@@ -230,6 +260,9 @@ int naettPlatformInitRequest(InternalRequest* req) {
     req->host = wcsndup(components.lpszHostName, components.dwHostNameLength);
     req->resource = wcsndup(components.lpszUrlPath, components.dwUrlPathLength + components.dwExtraInfoLength);
     free(url);
+    if (req->host == NULL || req->resource == NULL) {
+        return 0;
+    }
 
     LPWSTR uaBuf = winFromUTF8(req->options.userAgent ? req->options.userAgent : NAETT_UA);
     req->session = WinHttpOpen(uaBuf,
@@ -269,6 +302,12 @@ int naettPlatformInitRequest(InternalRequest* req) {
     }
 
     LPCWSTR headers = packHeaders(req);
+    if (headers == NULL) {
+        // PPSSPP: only happens if a header didn't survive the UTF-8 conversion, but upstream
+        // indexed straight into it.
+        naettPlatformFreeRequest(req);
+        return 0;
+    }
     if (headers[0] != 0) {
         if (!WinHttpAddRequestHeaders(
                 req->request, headers, -1, WINHTTP_ADDREQ_FLAG_ADD | WINHTTP_ADDREQ_FLAG_REPLACE)) {
@@ -326,6 +365,20 @@ void naettPlatformFreeRequest(InternalRequest* req) {
 }
 
 void naettPlatformCloseResponse(InternalResponse* res) {
+    // PPSSPP: this used to be empty. The status callback carries the response as its context, so
+    // once it's freed any further completion writes through a dangling pointer. Unhook the
+    // callback and close the request handle, which stops new ones being raised.
+    //
+    // This is not a full cancel: a callback already running on another thread isn't waited for,
+    // which would need the WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING handshake. Closing a response
+    // that hasn't completed still isn't supported here - see naettClose in naett.h.
+    InternalRequest* req = res->request;
+    if (req == NULL || req->request == NULL) {
+        return;
+    }
+    WinHttpSetStatusCallback(req->request, NULL, WINHTTP_CALLBACK_FLAG_ALL_NOTIFICATIONS, 0);
+    WinHttpCloseHandle(req->request);
+    req->request = NULL;
 }
 
 #endif  // __WINDOWS__