diff --git a/ext/naett-lib/README-ppsspp.md b/ext/naett-lib/README-ppsspp.md index 114b630b61..32bd5fa89b 100644 --- a/ext/naett-lib/README-ppsspp.md +++ b/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). diff --git a/ext/naett-lib/naett.h b/ext/naett-lib/naett.h index b79130db3b..d49614d0b1 100644 --- a/ext/naett-lib/naett.h +++ b/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); diff --git a/ext/naett-lib/src/naett_android.c b/ext/naett-lib/src/naett_android.c index 30d35550dd..4b79fa6012 100644 --- a/ext/naett-lib/src/naett_android.c +++ b/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, "", "(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) { diff --git a/ext/naett-lib/src/naett_core.c b/ext/naett-lib/src/naett_core.c index 0e9ee529e7..71f5cdaf7a 100644 --- a/ext/naett-lib/src/naett_core.c +++ b/ext/naett-lib/src/naett_core.c @@ -1,6 +1,8 @@ #include "naett_internal.h" #include #include +#include +#include #include #include #include @@ -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); diff --git a/ext/naett-lib/src/naett_curl.c b/ext/naett-lib/src/naett_curl.c index 4a9a7617aa..da798fc53c 100644 --- a/ext/naett-lib/src/naett_curl.c +++ b/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") diff --git a/ext/naett-lib/src/naett_curl.h b/ext/naett-lib/src/naett_curl.h index b474027525..656b5f713f 100644 --- a/ext/naett-lib/src/naett_curl.h +++ b/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 diff --git a/ext/naett-lib/src/naett_linux.c b/ext/naett-lib/src/naett_linux.c index 165abb1960..ad505137e6 100644 --- a/ext/naett-lib/src/naett_linux.c +++ b/ext/naett-lib/src/naett_linux.c @@ -14,6 +14,8 @@ #include #include #include +#include +#include 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) { diff --git a/ext/naett-lib/src/naett_objc.h b/ext/naett-lib/src/naett_objc.h index da95a02a52..813651c9d9 100644 --- a/ext/naett-lib/src/naett_objc.h +++ b/ext/naett-lib/src/naett_objc.h @@ -4,6 +4,7 @@ #if defined(__IOS__) || defined (__MACOS__) #include #include +#include #include #include @@ -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")) diff --git a/ext/naett-lib/src/naett_osx.c b/ext/naett-lib/src/naett_osx.c index 68c70992f9..1692b0c09e 100644 --- a/ext/naett-lib/src/naett_osx.c +++ b/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; } diff --git a/ext/naett-lib/src/naett_win.c b/ext/naett-lib/src/naett_win.c index b75c6874a8..c80b157150 100644 --- a/ext/naett-lib/src/naett_win.c +++ b/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__