From b2e0e849ef9b55c64f4a67d92a99927c5e78fb6e Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Mon, 9 Oct 2023 15:23:23 -0700 Subject: [PATCH] Tracking down a memory error. The store memory address is being overrun in the call to `<<`. --- Arena.cc | 264 +++++++++++++++++++++++++++----------------------- Arena.h | 89 ++++++++++++----- Buffer.cc | 12 +-- Buffer.h | 2 +- Dictionary.cc | 8 +- TLV.cc | 38 ++++---- TLV.h | 15 +-- tlvTest.cc | 53 +++++----- 8 files changed, 272 insertions(+), 209 deletions(-) diff --git a/Arena.cc b/Arena.cc index 5eae512..88a233c 100644 --- a/Arena.cc +++ b/Arena.cc @@ -1,6 +1,7 @@ -#include -#include -#include +#include +#include +#include +#include #if defined(__linux__) #include @@ -10,59 +11,61 @@ #include #endif -#if defined(DESKTOP_BUILD) -#include #include -#endif #include "Arena.h" -#define ARENA_UNINIT 0 -#define ARENA_STATIC 1 -#define ARENA_ALLOC 2 -#if defined(__linux__) -#define ARENA_MMAP 3 -#define PROT_RW PROT_READ|PROT_WRITE -#endif +namespace klib { + + +Arena::Arena() + : store(nullptr), size(0), fd(0), arenaType(ARENA_UNINIT) +{} + + +Arena::~Arena() +{ + this->Destroy(); +} void -InitializeArena(Arena &arena) +Arena::Initialize() { - arena.Store = NULL; - arena.Size = 0; - arena.Type = ARENA_UNINIT; - arena.fd = 0; + assert(this->arenaType != ARENA_UNINIT); + this->store = nullptr; + this->size = 0; + this->arenaType = ARENA_UNINIT; + this->fd = 0; } int -NewStaticArena(Arena &arena, uint8_t *mem, size_t size) +Arena::SetStatic(uint8_t *mem, size_t allocSize) { - arena.Store = mem; - arena.Size = size; - arena.Type = ARENA_STATIC; + this->store = mem; + this->size = allocSize; + this->arenaType = ARENA_STATIC; return 0; } int -AllocNewArena(Arena & arena, size_t size) +Arena::SetAlloc(size_t allocSize) { - if (arena.Size > 0) { - if (DestroyArena(arena) != 0) { - return -1; - } + if (this->size > 0) { + this->Destroy(); } - arena.Type = ARENA_ALLOC; - arena.Size = size; - arena.Store = (uint8_t *)calloc(sizeof(uint8_t), size); - if (arena.Store == NULL) { + this->arenaType = ARENA_ALLOC; + this->size = allocSize; + this->store = new uint8_t[allocSize]; + if (this->store == nullptr) { return -1; } + this->Clear(); return 0; } @@ -71,19 +74,19 @@ AllocNewArena(Arena & arena, size_t size) int MMapArena(Arena &arena, int fd, size_t size) { - if (arena.Size > 0) { + if (this->size > 0) { if (DestroyArena(arena) != 0) { return -1; } } - arena.Type = ARENA_MMAP; - arena.Size = size; - arena.Store = (uint8_t *)mmap(NULL, size, PROT_RW, MAP_SHARED, fd, 0); - if ((void *)arena.Store == MAP_FAILED) { + this->arenaType = ARENA_MMAP; + this->size = size; + this->store = (uint8_t *)mmap(NULL, size, PROT_RW, MAP_SHARED, fd, 0); + if ((void *)this->store == MAP_FAILED) { return -1; } - arena.fd = fd; + this->fd = fd; return 0; } @@ -93,7 +96,7 @@ OpenArena(Arena &arena, const char *path) { struct stat st; - if (arena.Size > 0) { + if (this->size > 0) { if (DestroyArena(arena) != 0) { return -1; } @@ -103,12 +106,12 @@ OpenArena(Arena &arena, const char *path) return -1; } - arena.fd = open(path, O_RDWR); - if (arena.fd == -1) { + this->fd = open(path, O_RDWR); + if (this->fd == -1) { return -1; } - return MMapArena(arena, arena.fd, (size_t)st.st_size); + return MMapArena(arena, this->fd, (size_t)st.st_size); } @@ -117,7 +120,7 @@ CreateArena(Arena &arena, const char *path, size_t size, mode_t mode) { int fd = 0; - if (arena.Size > 0) { + if (this->size > 0) { if (DestroyArena(arena) != 0) { return -1; } @@ -139,115 +142,132 @@ CreateArena(Arena &arena, const char *path, size_t size, mode_t mode) #endif +bool +Arena::CursorInArena(uint8_t *cursor) +{ + if (cursor < this->store) { + return false; + } + + if (cursor >= this->End()) { + return false; + } + + return true; +} + + /* * ClearArena clears the memory being used, removing any data * present. It does not free the memory; it is effectively a * wrapper around memset. */ void -ClearArena(Arena &arena) +Arena::Clear() { - if (arena.Size == 0) { + if (this->size == 0) { return; } - memset(arena.Store, 0, arena.Size); + memset(this->store, 0, this->size); } -int -DestroyArena(Arena &arena) -{ - if (arena.Type == ARENA_UNINIT) { - return 0; - } - - switch (arena.Type) { - case ARENA_STATIC: - break; - case ARENA_ALLOC: - free(arena.Store); - break; - #if defined(__linux__) - case ARENA_MMAP: - if (munmap(arena.Store, arena.Size) == -1) { - return -1; - } - - if (close(arena.fd) == -1) { - return -1; - } - - arena.fd = 0; - break; - #endif - default: - #if defined(NDEBUG) - return -1; - #else - abort(); - #endif - - } - - arena.Type = ARENA_UNINIT; - arena.Size = 0; - arena.Store = NULL; - return 0; -} - -#if defined(DESKTOP_BUILD) void -DisplayArena(const Arena &arena) +Arena::Destroy() { - std::cout << "Arena @ 0x"; - std::cout << std::hex << (uintptr_t)&arena << std::endl; - std::cout << std::dec; - std::cout << "\tStore is " << arena.Size << " bytes at address 0x"; - std::cout << std::hex << (uintptr_t)&(arena.Store) << std::endl; - std::cout << "\tType: "; + if (this->arenaType == ARENA_UNINIT) { + return; + } - switch (arena.Type) { - case ARENA_UNINIT: - std::cout << "uninitialized"; - break; - case ARENA_STATIC: - std::cout << "static"; - break; - case ARENA_ALLOC: - std::cout << "allocated"; - break; + switch (this->arenaType) { + case ARENA_STATIC: + break; + case ARENA_ALLOC: + delete this->store; + break; + #if defined(__linux__) + case ARENA_MMAP: + if (munmap(this->store, this->size) == -1) { + return -1; + } + + if (close(this->fd) == -1) { + return -1; + } + + this->fd = 0; + break; + #endif + default: + #if defined(NDEBUG) + return -1; + #else + abort(); + #endif + + } + + this->arenaType = ARENA_UNINIT; + this->size = 0; + this->store = nullptr; + return; +} + +std::ostream & +operator<<(std::ostream &os, const Arena arena) +{ + auto cursor = arena.NewCursor(); + char cursorString[17] = {0}; + snprintf(cursorString, 16, "%0p", cursor); + + os << "Arena<"; + switch (arena.Type()) { + case ARENA_UNINIT: + os << "uninitialized"; + break; + case ARENA_STATIC: + os << "static"; + break; + case ARENA_ALLOC: + os << "allocated"; + break; #if defined(__linux__) - case ARENA_MMAP: - std::cout << "mmap/file"; - break; + case ARENA_MMAP: + os << "mmap/file"; + break; #endif - default: - std::cout << "unknown (this is a bug)"; + default: + os << "unknown (this is a bug)"; } - std::cout << std::endl; -} -#else -void -DisplayArena(const Arena &arena) -{ + os << ">@0x"; + os << std::hex << (uintptr_t) &arena; + os << std::dec; + os << ",store<" << arena.Size() << "B>@0x"; + os << std::hex << cursorString; + os << std::dec; + return os; } -#endif + int -WriteArena(const Arena &arena, const char *path) +Arena::Write(const char *path) { - FILE *arenaFile = NULL; - int retc = -1; + FILE *arenaFile = NULL; + int retc = -1; +#if defined(__posix__) arenaFile = fopen(path, "w"); if (arenaFile == NULL) { +#else + if (fopen_s(&arenaFile, path, "w") != 0) { +#endif return -1; } - if (fwrite(arena.Store, sizeof(*arena.Store), arena.Size, - arenaFile) == arena.Size) { + if (fwrite(this->store, sizeof(*this->store), this->size, + arenaFile) == this->size) { retc = 0; } @@ -258,3 +278,5 @@ WriteArena(const Arena &arena, const char *path) return retc; } + +} // namespace klib \ No newline at end of file diff --git a/Arena.h b/Arena.h index 10fbae0..69f0735 100644 --- a/Arena.h +++ b/Arena.h @@ -2,43 +2,82 @@ #define KIMODEM_ARENA_H +#include #include #include #include -typedef struct { - uint8_t *Store; - size_t Size; - int fd; - uint8_t Type; -} Arena; +namespace klib { -/* - * InitializeArena is intended for use only with systems that - * do not initialize new variables to zero. It should be called - * exactly once, at the start of the program. Any other time the - * arena needs to be reset, it should be called with clear_arena - * or destroy_arena. - */ -void InitializeArena(Arena &arena); -int NewStaticArena(Arena &, uint8_t *, size_t); -int AllocNewArena(Arena &, size_t); +enum ArenaType : uint8_t { + ARENA_UNINIT, + ARENA_STATIC, + ARENA_ALLOC, + ARENA_MMAP, +}; + +class Arena { +public: + Arena(); + + ~Arena(); + + /* + * InitializeArena is intended for use only with systems that + * do not initialize new variables to zero. It should be called + * exactly once, at the start of the program. Any other time the + * arena needs to be reset, it should be called with Clear or + * Destroy. + */ + void Initialize(); + + int SetStatic(uint8_t *, size_t); + + int SetAlloc(size_t allocSize); + #if defined(__linux__) -int MMapArena(Arena &, int); /* arena will own fd */ -int CreateArena(Arena &arena, const char *path, size_t size, mode_t mode); -int OpenArena(Arena &, const char *, size_t); + int MemoryMap(int fd); // Arena will own fd. + int Create(const char *path, size_t size, mode_t mode); + int Open(const char *, size_t); #endif -void ClearArena(Arena &); -int DestroyArena(Arena &); /* dispose of any memory used by arena */ + uint8_t *NewCursor() const { return this->store; } + uint8_t *End() { return this->store + this->size; } + bool CursorInArena(uint8_t *cursor); -/* DANGER: if arena is file backed (mmap or open), DO NOT WRITE TO THE - * BACKING FILE! */ -int WriteArena(const Arena &arena, const char *path); + size_t Size() const + { return this->size; } -void DisplayArena(const Arena &arena); + uint8_t Type() const + { return this->arenaType; } + + bool Ready() const { return this->Type() != ARENA_UNINIT; }; + void Clear(); + void Destroy(); /* dispose of any memory used by arena */ + + /* + * DANGER: if arena is file backed (mmap or open), DO NOT WRITE TO THE + * BACKING FILE! + */ + int Write(const char *path); + + uint8_t &operator[](size_t index) + { return this->store[index]; } + +private: + uint8_t *store; + size_t size; + int fd; + ArenaType arenaType; +}; + + +std::ostream &operator<<(std::ostream& os, Arena arena); + + +} // namespace klib #endif diff --git a/Buffer.cc b/Buffer.cc index 84b6980..478872e 100644 --- a/Buffer.cc +++ b/Buffer.cc @@ -19,7 +19,7 @@ nearestPower(size_t x) { if (x == 0) { return 0; - }; + } std::cout << "x -> "; @@ -80,6 +80,7 @@ Buffer::Append(uint8_t *data, size_t datalen) resized = true; } + assert(this->contents != nullptr); memcpy(this->contents + this->length, data, datalen); this->length += datalen; return resized; @@ -142,17 +143,14 @@ Buffer::Resize(size_t newCapacity) return; } - uint8_t *newContents = new uint8_t[newCapacity]; + auto newContents = new uint8_t[newCapacity]; memset(newContents, 0, newCapacity); if (this->length > 0) { memcpy(newContents, this->contents, this->length); } - if (this->contents != nullptr) { - delete this->contents; - } - + delete this->contents; this->contents = newContents; this->capacity = newCapacity; } @@ -198,7 +196,7 @@ Buffer::Reclaim() } size_t -Buffer::mustGrow(size_t delta) +Buffer::mustGrow(size_t delta) const { if ((delta + this->length) < this->capacity) { return 0; diff --git a/Buffer.h b/Buffer.h index 05c5f2f..b059e89 100644 --- a/Buffer.h +++ b/Buffer.h @@ -41,7 +41,7 @@ public: uint8_t &operator[](size_t index); private: - size_t mustGrow(size_t delta); + size_t mustGrow(size_t delta) const; bool shiftRight(size_t offset, size_t delta); bool shiftLeft(size_t offset, size_t delta); diff --git a/Dictionary.cc b/Dictionary.cc index 8deadad..cf8504d 100644 --- a/Dictionary.cc +++ b/Dictionary.cc @@ -105,8 +105,8 @@ Dictionary::spaceAvailable(uint8_t klen, uint8_t vlen) required += klen + 2; required += vlen + 2; - remaining = (uintptr_t)cursor - (uintptr_t)arena.Store; - remaining = arena.Size - remaining; + remaining = (uintptr_t)cursor - (uintptr_t)arena.NewCursor(); + remaining = arena.Size() - remaining; return ((size_t)remaining >= required); } @@ -115,7 +115,7 @@ Dictionary::spaceAvailable(uint8_t klen, uint8_t vlen) void Dictionary::DumpKVPairs() { - uint8_t *cursor = (this->arena).Store; + uint8_t *cursor = (this->arena).NewCursor(); TLV::Record rec; TLV::ReadFromMemory(rec, cursor); @@ -147,5 +147,5 @@ Dictionary::DumpKVPairs() void Dictionary::DumpToFile(const char *path) { - WriteArena(this->arena, path); + this->arena.Write(path); } diff --git a/TLV.cc b/TLV.cc index 4a2b49c..4a156c3 100644 --- a/TLV.cc +++ b/TLV.cc @@ -1,33 +1,32 @@ #include #include "TLV.h" +using namespace klib; + #define REC_SIZE(x) ((std::size_t)x.Len + 2) +namespace klib { namespace TLV { static bool spaceAvailable(Arena &arena, uint8_t *cursor, uint8_t len) { - uintptr_t remaining = 0; - if (cursor == NULL) { return false; } - remaining = (uintptr_t)cursor - (uintptr_t)arena.Store; - remaining = arena.Size - remaining; - return ((size_t)remaining >= ((size_t)len+2)); + return arena.CursorInArena(cursor + len); } static inline void clearUnused(Record &rec) { - uint8_t trail = TLV_MAX_LEN-rec.Len; + uint8_t trail = TLV_MAX_LEN - rec.Len; - memset(rec.Val+rec.Len, 0, trail); + memset(rec.Val + rec.Len, 0, trail); } @@ -58,7 +57,7 @@ WriteToMemory(Arena &arena, uint8_t *cursor, Record &rec) } -void +void SetRecord(Record &rec, uint8_t tag, uint8_t len, const char *val) { rec.Tag = tag; @@ -73,7 +72,7 @@ ReadFromMemory(Record &rec, uint8_t *cursor) { rec.Tag = cursor[0]; rec.Len = cursor[1]; - memcpy(rec.Val, cursor+2, rec.Len); + memcpy(rec.Val, cursor + 2, rec.Len); clearUnused(rec); } @@ -97,10 +96,10 @@ FindTag(Arena &arena, uint8_t *cursor, Record &rec) uint8_t * LocateTag(Arena &arena, uint8_t *cursor, Record &rec) { - uint8_t tag, len; + uint8_t tag, len; if (cursor == NULL) { - cursor = arena.Store; + cursor = arena.NewCursor(); } while ((tag = cursor[0]) != rec.Tag) { @@ -124,19 +123,19 @@ LocateTag(Arena &arena, uint8_t *cursor, Record &rec) uint8_t * -FindEmpty(Arena &arena, uint8_t *cursor) { - Record rec; - +FindEmpty(Arena &arena, uint8_t *cursor) +{ + Record rec; + rec.Tag = TAG_EMPTY; return FindTag(arena, cursor, rec); } - -uint8_t * +uint8_t * SkipRecord(Record &rec, uint8_t *cursor) { - return (uint8_t *)((uintptr_t)cursor + rec.Len + 2); + return (uint8_t *) ((uintptr_t) cursor + rec.Len + 2); } @@ -147,8 +146,8 @@ DeleteRecord(Arena &arena, uint8_t *cursor) return; } - uint8_t len = cursor[1] + 2; - uint8_t *stop = arena.Store + arena.Size; + uint8_t len = cursor[1] + 2; + uint8_t *stop = arena.NewCursor() + arena.Size(); stop -= len; @@ -166,3 +165,4 @@ DeleteRecord(Arena &arena, uint8_t *cursor) } // namespace TLV +} // namespace klib \ No newline at end of file diff --git a/TLV.h b/TLV.h index e1c7e84..bbd112a 100644 --- a/TLV.h +++ b/TLV.h @@ -5,6 +5,8 @@ #include "Arena.h" +using namespace klib; + #ifndef TLV_MAX_LEN #define TLV_MAX_LEN 253 @@ -14,6 +16,7 @@ #define TAG_EMPTY 0 +namespace klib { namespace TLV { @@ -23,18 +26,17 @@ struct Record { char Val[TLV_MAX_LEN]; }; - uint8_t *WriteToMemory(Arena &, uint8_t *, Record &); void ReadFromMemory(Record &, uint8_t *); void SetRecord(Record &, uint8_t, uint8_t, const char *); void DeleteRecord(Arena &, uint8_t *); /* - * returns a pointer to memory where the record was found, - * e.g. LocateTag(...)[0] is the tag of the found record. - * FindTag will call LocateTag and then SkipRecord if the - * tag was found. - */ +* returns a pointer to memory where the record was found, +* e.g. LocateTag(...)[0] is the tag of the found record. +* FindTag will call LocateTag and then SkipRecord if the +* tag was found. +*/ uint8_t *FindTag(Arena &, uint8_t *, Record &); uint8_t *LocateTag(Arena &, uint8_t *, Record &); @@ -43,6 +45,7 @@ uint8_t *SkipRecord(Record &, uint8_t *); } // namespace TLV +} // namespace klib #endif diff --git a/tlvTest.cc b/tlvTest.cc index 42d6e01..a646c67 100644 --- a/tlvTest.cc +++ b/tlvTest.cc @@ -11,63 +11,65 @@ static uint8_t arenaBuffer[ARENA_SIZE]; -bool +void tlvTestSuite(Arena &backend) { TLV::Record rec1, rec2, rec3, rec4; - uint8_t *cursor = NULL; + uint8_t *cursor = nullptr; + std::cout << "\tSetting first three records." << std::endl; TLV::SetRecord(rec1, 1, TEST_STRLEN1, TEST_STR1); TLV::SetRecord(rec2, 2, TEST_STRLEN2, TEST_STR2); TLV::SetRecord(rec3, 1, TEST_STRLEN4, TEST_STR4); rec4.Tag = 1; - assert(TLV::WriteToMemory(backend, cursor, rec1) != NULL); - assert((cursor = TLV::WriteToMemory(backend, cursor, rec2)) != NULL); - assert(TLV::WriteToMemory(backend, cursor, rec3) != NULL); - cursor = NULL; + std::cout << "\twriting new rec1" << std::endl; + assert(TLV::WriteToMemory(backend, cursor, rec1) != nullptr); + std::cout << "\twriting new rec2" << std::endl; + assert((cursor = TLV::WriteToMemory(backend, cursor, rec2)) != nullptr); + std::cout << "\twriting new rec3" << std::endl; + assert(TLV::WriteToMemory(backend, cursor, rec3) != nullptr); + cursor = nullptr; // the cursor should point at the next record, // and rec4 should contain the same data as rec1. + std::cout << "\tFindTag 1" << std::endl; cursor = TLV::FindTag(backend, cursor, rec4); - assert(cursor != NULL); - assert(cursor != backend.Store); + assert(cursor != nullptr); + assert(cursor != backend.NewCursor()); assert(cmpRecord(rec1, rec4)); + std::cout << "\tFindTag 2" << std::endl; cursor = TLV::FindTag(backend, cursor, rec4); - assert(cursor != NULL); + assert(cursor != nullptr); assert(cmpRecord(rec3, rec4)); TLV::SetRecord(rec4, 3, TEST_STRLEN3, TEST_STR3); - assert(TLV::WriteToMemory(backend, NULL, rec4)); + assert(TLV::WriteToMemory(backend, nullptr, rec4)); rec4.Tag = 2; - cursor = TLV::FindTag(backend, NULL, rec4); - assert(cursor != NULL); + cursor = TLV::FindTag(backend, nullptr, rec4); + assert(cursor != nullptr); TLV::DeleteRecord(backend, cursor); assert(cursor[0] == 3); - - return true; } bool runSuite(Arena &backend, const char *label) { - DisplayArena(backend); - + std::cout << backend << std::endl; std::cout << "running test suite " << label << ": "; - if (!tlvTestSuite(backend)) { + try { + tlvTestSuite(backend); + } catch (_exception){ std::cout << "FAILED" << std::endl; return false; } std::cout << "OK" << std::endl; std::cout << "\tdestroying arena: "; - if (DestroyArena(backend) != 0) { - std::cout << "FAILED" << std::endl; - return false; - } + backend.Destroy(); std::cout << "OK" << std::endl; return true; @@ -83,15 +85,13 @@ main(int argc, const char *argv[]) Arena arenaMem; std::cout << "TESTPROG: " << argv[0] << std::endl; - InitializeArena(arenaStatic); - InitializeArena(arenaMem); - if (-1 == NewStaticArena(arenaStatic, arenaBuffer, ARENA_SIZE)) { + if (-1 == arenaStatic.SetStatic(arenaBuffer, ARENA_SIZE)) { abort(); } else if (!runSuite(arenaStatic, "arenaStatic")) { abort(); } - ClearArena(arenaStatic); + arenaStatic.Clear(); #if defined(__linux__) Arena arenaFile; @@ -104,11 +104,12 @@ main(int argc, const char *argv[]) } #endif - if (-1 == AllocNewArena(arenaMem, ARENA_SIZE)) { + if (-1 == arenaMem.SetAlloc(ARENA_SIZE)) { abort(); } else if (!runSuite(arenaMem, "arenaMem")) { abort(); } + arenaMem.Clear(); std::cout << "OK" << std::endl; return 0;