From 63dbf9ae35c03df05613dac9418c4b9a50456484 Mon Sep 17 00:00:00 2001 From: Jordan Bancino Date: Mon, 31 Oct 2022 19:52:37 -0400 Subject: [PATCH] Fix a few more memory bugs. Still have some to tackle. --- TODO.txt | 4 +- src/HttpServer.c | 30 ++++- src/Memory.c | 291 +++++++++++++++++++++++++++++-------------- src/Telodendria.c | 5 +- src/Util.c | 85 +++++++++++++ src/include/Memory.h | 4 +- src/include/Util.h | 8 ++ 7 files changed, 321 insertions(+), 106 deletions(-) diff --git a/TODO.txt b/TODO.txt index 9f06c14..ebc4561 100644 --- a/TODO.txt +++ b/TODO.txt @@ -52,7 +52,7 @@ Phase 2: Building a foundation [~] Data abstraction layer [ ] Database upgrades/migration path [ ] Caching and cache control - [ ] Make memory info access O(1) + [x] Make memory info access O(1) [x] Error generation [x] Properly implement the command line options as stated in telodendria(8) [x] Remove "chroot" option, just chroot into the data dir, and make the log @@ -61,6 +61,7 @@ Phase 2: Building a foundation [x] Fix memory leaks [ ] Fix bug where the socket stays open after quit. [ ] Figure out how to write unit tests for array/hashmap/etc +[ ] Add recipe to td script to upload patches to the Matrix room Phase 3: Welcome to Matrix @@ -135,3 +136,4 @@ Documentation [x] Add onboarding documentation [x] Building from source [!] Writing config file (see Config.5) + diff --git a/src/HttpServer.c b/src/HttpServer.c index 2ddd971..35af7bc 100644 --- a/src/HttpServer.c +++ b/src/HttpServer.c @@ -122,7 +122,10 @@ HttpServerContextFree(HttpServerContext * c) while (HashMapIterate(c->requestHeaders, &key, &val)) { - /* Since we create these, we know they're always on the heap */ + /* + * These are always parsed from the request, so they should + * always be on the heap. + */ Free(key); Free(val); } @@ -130,8 +133,25 @@ HttpServerContextFree(HttpServerContext * c) while (HashMapIterate(c->responseHeaders, &key, &val)) { - Free(key); - Free(val); + /* + * These are generated by code. As such, they may be either + * on the heap, or on the stack, depending on how they were + * added. + * + * Basically, if the memory API knows about a pointer, then + * it can be freed. If it doesn't know about a pointer, skip + * freeing it because it's probably a stack pointer. + */ + + if (MemoryInfoGet(key)) + { + Free(key); + } + + if (MemoryInfoGet(val)) + { + Free(val); + } } HashMapFree(c->responseHeaders); @@ -432,7 +452,7 @@ HttpServerWorkerThread(void *args) } /* Get the first line of the request */ - lineLen = getline(&line, &lineSize, fp); + lineLen = UtilGetLine(&line, &lineSize, fp); if (lineLen == -1) { goto bad_request; @@ -501,7 +521,7 @@ HttpServerWorkerThread(void *args) goto internal_error; } - while ((lineLen = getline(&line, &lineSize, fp)) != -1) + while ((lineLen = UtilGetLine(&line, &lineSize, fp)) != -1) { char *headerKey; char *headerValue; diff --git a/src/Memory.c b/src/Memory.c index 22ad6a5..5f9eb43 100644 --- a/src/Memory.c +++ b/src/Memory.c @@ -33,16 +33,105 @@ struct MemoryInfo const char *file; int line; void *pointer; - - MemoryInfo *next; - MemoryInfo *prev; }; static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER; -static MemoryInfo *lastAllocation = NULL; static void (*hook) (MemoryAction, MemoryInfo *, void *) = NULL; static void *hookArgs = NULL; +static MemoryInfo **allocations = NULL; +static size_t allocationsSize = 0; +static size_t allocationsLen = 0; + +static size_t +MemoryHash(void *p) +{ + return (((size_t) p) >> 2 * 7) % allocationsSize; +} + +static int +MemoryInsert(MemoryInfo * a) +{ + size_t hash; + + if (!allocations) + { + allocationsSize = 64; + allocations = calloc(allocationsSize, sizeof(void *)); + if (!allocations) + { + return 0; + } + } + + if ((allocationsLen + 1) >= (0.75 * allocationsSize)) + { + size_t i; + size_t tmpAllocationsSize = allocationsSize; + MemoryInfo **tmpAllocations; + + allocationsSize *= 2; + tmpAllocations = calloc(allocationsSize, sizeof(void *)); + + if (!tmpAllocations) + { + return 0; + } + + for (i = 0; i < tmpAllocationsSize; i++) + { + if (allocations[i]) + { + hash = MemoryHash(allocations[i]->pointer); + + while (tmpAllocations[hash]) + { + hash = (hash + 1) % allocationsSize; + } + + tmpAllocations[hash] = allocations[i]; + } + } + + free(allocations); + allocations = tmpAllocations; + } + + hash = MemoryHash(a->pointer); + + while (allocations[hash]) + { + hash = (hash + 1) % allocationsSize; + } + + allocations[hash] = a; + allocationsLen++; + + return 1; +} + +static void +MemoryDelete(MemoryInfo * a) +{ + size_t hash = MemoryHash(a->pointer); + size_t count = 0; + + while (count <= allocationsSize) + { + if (allocations[hash] && allocations[hash] == a) + { + allocations[hash] = NULL; + allocationsLen--; + return; + } + else + { + hash = (hash + 1) % allocationsSize; + count++; + } + } +} + void * MemoryAllocate(size_t size, const char *file, int line) { @@ -70,16 +159,15 @@ MemoryAllocate(size_t size, const char *file, int line) a->file = file; a->line = line; a->pointer = p; - a->next = NULL; - a->prev = lastAllocation; - if (lastAllocation) + if (!MemoryInsert(a)) { - lastAllocation->next = a; + free(a); + free(p); + pthread_mutex_unlock(&lock); + return NULL; } - lastAllocation = a; - if (hook) { hook(MEMORY_ALLOCATE, a, hookArgs); @@ -100,99 +188,102 @@ MemoryReallocate(void *p, size_t size, const char *file, int line) return MemoryAllocate(size, file, line); } - pthread_mutex_lock(&lock); - - a = lastAllocation; - while (a) + a = MemoryInfoGet(p); + if (a) { - if (a->pointer == p) + pthread_mutex_lock(&lock); + new = realloc(a->pointer, size); + if (new) { - new = realloc(p, size); - if (new) - { - a->pointer = new; - a->size = size; - a->file = file; - a->line = line; + MemoryDelete(a); + a->size = size; + a->file = file; + a->line = line; - if (hook) - { - hook(MEMORY_REALLOCATE, a, hookArgs); - } + a->pointer = new; + MemoryInsert(a); + + if (hook) + { + hook(MEMORY_REALLOCATE, a, hookArgs); } - break; } - - a = a->prev; + pthread_mutex_unlock(&lock); + } + else if (hook) + { + a = malloc(sizeof(MemoryInfo)); + if (a) + { + a->size = 0; + a->file = file; + a->line = line; + a->pointer = p; + hook(MEMORY_BAD_POINTER, a, hookArgs); + free(a); + } } - pthread_mutex_unlock(&lock); return new; } void -MemoryFree(void *p) +MemoryFree(void *p, const char *file, int line) { MemoryInfo *a; - pthread_mutex_lock(&lock); - - a = lastAllocation; - - while (a) + if (!p) { - if (a->pointer == p) - { - if (a->prev) - { - a->prev->next = a->next; - } - else - { - lastAllocation = a->next; - } - - if (a->next) - { - a->next->prev = a->prev; - } - else - { - lastAllocation = a->prev; - } - - if (hook) - { - hook(MEMORY_FREE, a, hookArgs); - } - - free(a); - free(p); - - break; - } - - a = a->prev; + return; } - pthread_mutex_unlock(&lock); + a = MemoryInfoGet(p); + if (a) + { + pthread_mutex_lock(&lock); + if (hook) + { + a->file = file; + a->line = line; + hook(MEMORY_FREE, a, hookArgs); + } + MemoryDelete(a); + free(a->pointer); + free(a); + + pthread_mutex_unlock(&lock); + } + else if (hook) + { + a = malloc(sizeof(MemoryInfo)); + if (a) + { + a->file = file; + a->line = line; + a->size = 0; + a->pointer = p; + hook(MEMORY_BAD_POINTER, a, hookArgs); + free(a); + } + } } size_t MemoryAllocated(void) { - MemoryInfo *a; + size_t i; size_t total = 0; pthread_mutex_lock(&lock); - a = lastAllocation; - while (a) + for (i = 0; i < allocationsSize; i++) { - total += a->size; - a = a->prev; + if (allocations[i]) + { + total += allocations[i]->size; + } } pthread_mutex_unlock(&lock); @@ -203,22 +294,23 @@ MemoryAllocated(void) void MemoryFreeAll(void) { - MemoryInfo *a; + size_t i; pthread_mutex_lock(&lock); - a = lastAllocation; - while (a) + for (i = 0; i < allocationsSize; i++) { - MemoryInfo *prev = a->prev; - - free(a->pointer); - free(a); - - a = prev; + if (allocations[i]) + { + free(allocations[i]->pointer); + free(allocations[i]); + } } - lastAllocation = NULL; + free(allocations); + allocations = NULL; + allocationsSize = 0; + allocationsLen = 0; pthread_mutex_unlock(&lock); } @@ -226,22 +318,28 @@ MemoryFreeAll(void) MemoryInfo * MemoryInfoGet(void *p) { - MemoryInfo *a; + size_t hash, count; pthread_mutex_lock(&lock); - a = lastAllocation; - while (a) + hash = MemoryHash(p); + + count = 0; + while (count <= allocationsSize) { - if (a->pointer == p) + if (!allocations[hash] || allocations[hash]->pointer != p) + { + hash = (hash + 1) % allocationsSize; + count++; + } + else { break; } - - a = a->prev; } - return a; + pthread_mutex_unlock(&lock); + return allocations[hash]; } size_t @@ -291,15 +389,16 @@ MemoryInfoGetPointer(MemoryInfo * a) void MemoryIterate(void (*iterFunc) (MemoryInfo *, void *), void *args) { - MemoryInfo *a; + size_t i; pthread_mutex_lock(&lock); - a = lastAllocation; - while (a) + for (i = 0; i < allocationsSize; i++) { - iterFunc(a, args); - a = a->prev; + if (allocations[i]) + { + iterFunc(allocations[i], args); + } } pthread_mutex_unlock(&lock); diff --git a/src/Telodendria.c b/src/Telodendria.c index 676902b..55ec45b 100644 --- a/src/Telodendria.c +++ b/src/Telodendria.c @@ -58,8 +58,8 @@ TelodendriaMemoryHook(MemoryAction a, MemoryInfo * i, void *args) action = "Freed"; break; case MEMORY_BAD_POINTER: - Log(lc, LOG_WARNING, "Bad pointer passed into a memory function: %p", (void *) i); - return; + action = "Bad pointer to"; + break; default: action = "Unknown operation on"; break; @@ -154,6 +154,7 @@ main(int argc, char **argv) memset(&matrixArgs, 0, sizeof(matrixArgs)); lc = LogConfigCreate(); + LogConfigLevelSet(lc, LOG_DEBUG); if (!lc) { diff --git a/src/Util.c b/src/Util.c index b51ac85..bc784c2 100644 --- a/src/Util.c +++ b/src/Util.c @@ -25,12 +25,14 @@ #include +#include #include #include #include #include #include #include +#include unsigned long UtilServerTs(void) @@ -174,3 +176,86 @@ UtilStringToBytes(char *str) return bytes; } + +ssize_t +UtilGetDelim(char **linePtr, size_t * n, int delim, FILE * stream) +{ + char *curPos, *newLinePtr; + size_t newLinePtrLen; + int c; + + if (!linePtr || !n || !stream) + { + errno = EINVAL; + return -1; + } + + if (*linePtr == NULL) + { + *n = 128; + + if (!(*linePtr = Malloc(*n))) + { + errno = ENOMEM; + return -1; + } + } + + curPos = *linePtr; + + while (1) + { + c = getc(stream); + + if (ferror(stream) || (c == EOF && curPos == *linePtr)) + { + return -1; + } + + if (c == EOF) + { + break; + } + + if ((*linePtr + *n - curPos) < 2) + { + if (SSIZE_MAX / 2 < *n) + { +#ifdef EOVERFLOW + errno = EOVERFLOW; +#else + errno = ERANGE; +#endif + return -1; + } + + newLinePtrLen = *n * 2; + + if (!(newLinePtr = Realloc(*linePtr, newLinePtrLen))) + { + errno = ENOMEM; + return -1; + } + + curPos = newLinePtr + (curPos - *linePtr); + *linePtr = newLinePtr; + *n = newLinePtrLen; + } + + *curPos++ = (char) c; + + if (c == delim) + { + break; + } + } + + *curPos = '\0'; + return (ssize_t) (curPos - *linePtr); +} + +ssize_t +UtilGetLine(char **linePtr, size_t * n, FILE * stream) +{ + return UtilGetDelim(linePtr, n, '\n', stream); +} diff --git a/src/include/Memory.h b/src/include/Memory.h index 8769cb5..f0db18b 100644 --- a/src/include/Memory.h +++ b/src/include/Memory.h @@ -36,13 +36,13 @@ typedef enum MemoryAction #define Malloc(x) MemoryAllocate(x, __FILE__, __LINE__) #define Realloc(x, s) MemoryReallocate(x, s, __FILE__, __LINE__) -#define Free(x) MemoryFree(x) +#define Free(x) MemoryFree(x, __FILE__, __LINE__) typedef struct MemoryInfo MemoryInfo; extern void *MemoryAllocate(size_t, const char *, int); extern void *MemoryReallocate(void *, size_t, const char *, int); -extern void MemoryFree(void *); +extern void MemoryFree(void *, const char *, int); extern size_t MemoryAllocated(void); extern void MemoryFreeAll(void); diff --git a/src/include/Util.h b/src/include/Util.h index a789e90..673564f 100644 --- a/src/include/Util.h +++ b/src/include/Util.h @@ -31,7 +31,9 @@ #ifndef TELODENDRIA_UTIL_H #define TELODENDRIA_UTIL_H +#include #include +#include /* * Get the current type in milliseconds since the Unix epoch. This uses @@ -101,4 +103,10 @@ extern int extern size_t UtilStringToBytes(char *); +extern ssize_t + UtilGetDelim(char **, size_t *, int, FILE *); + +extern ssize_t + UtilGetLine(char **, size_t *, FILE *); + #endif /* TELODENDRIA_UTIL_H */