mirror of
https://github.com/hrydgard/ppsspp.git
synced 2026-10-01 14:58:14 +00:00
naett: Fix two leaks and the response buffer's overflow
naettFree frees the method and url it strdup'd but never the user agent, which stringSetter allocates exactly the same way. We set a user agent on every request, so that leaked on every one of them, on all platforms. On Linux, headerCallback strndup's each header line and only hands it to the header list when it finds a colon - the status line and the blank line that ends the block don't have one, so it leaked those every response, and again per hop when following redirects. defaultBodyWriter grew its capacity by doubling an int until the new data fit. Both the length and the resulting capacity come from the response, so that's signed overflow on a large one, and a negative capacity then reaches realloc as a huge size_t. It also assigned the realloc result straight over the old pointer, so a failed allocation lost the buffer and the memcpy below went through NULL. Grow in int64_t, cap at INT_MAX, and report failure by returning short - which is what every caller already checks for.
This commit is contained in:
1 parent
6d4bc5f261
commit
28962036ef
3 files changed
+47
-9
No files matched your search
@@ -33,3 +33,13 @@ 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.
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -171,6 +171,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 +198,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;
|
||||
|
||||
Reference in new issue
Block a user