Tracking down a memory error.

The store memory address is being overrun in the call to `<<`.

Arena.cc

@@ -1,6 +1,7 @@
-#include <stdio.h>
+#include <cassert>
-#include <stdlib.h>
+#include <cstdio>
-#include <string.h>
+#include <cstdlib>
+#include <cstring>
 
 #if defined(__linux__)
 #include <sys/mman.h>
@@ -10,59 +11,61 @@
 #include <unistd.h>
 #endif
 
-#if defined(DESKTOP_BUILD)
-#include <iostream>
 #include <ios>
-#endif
 
 #include "Arena.h"
 
 
-#define ARENA_UNINIT	0
+namespace klib {
-#define ARENA_STATIC	1
+
-#define ARENA_ALLOC	2
+
-#if defined(__linux__)
+Arena::Arena()
-#define ARENA_MMAP	3
+    : store(nullptr), size(0), fd(0), arenaType(ARENA_UNINIT)
-#define PROT_RW		PROT_READ|PROT_WRITE
+{}
-#endif
+
+
+Arena::~Arena()
+{
+	this->Destroy();
+}
 
 
 void
-InitializeArena(Arena &arena)
+Arena::Initialize()
 {
-	arena.Store = NULL;
+	assert(this->arenaType != ARENA_UNINIT);
-	arena.Size = 0;
+	this->store = nullptr;
-	arena.Type = ARENA_UNINIT;
+	this->size = 0;
-	arena.fd = 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;
+	this->store = mem;
-	arena.Size = size;
+	this->size = allocSize;
-	arena.Type = ARENA_STATIC;
+	this->arenaType = ARENA_STATIC;
 	return 0;
 }
 
 
 int
-AllocNewArena(Arena & arena, size_t size)
+Arena::SetAlloc(size_t allocSize)
 {
-	if (arena.Size > 0) {
+	if (this->size > 0) {
-		if (DestroyArena(arena) != 0) {
+		this->Destroy();
-			return -1;
-		}
 	}
 
-	arena.Type = ARENA_ALLOC;
+	this->arenaType = ARENA_ALLOC;
-	arena.Size = size;
+	this->size = allocSize;
-	arena.Store = (uint8_t *)calloc(sizeof(uint8_t), size);
+	this->store = new uint8_t[allocSize];
-	if (arena.Store == NULL) {
+	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;
+	this->arenaType = ARENA_MMAP;
-	arena.Size = size;
+	this->size = size;
-	arena.Store = (uint8_t *)mmap(NULL, size, PROT_RW, MAP_SHARED, fd, 0);
+	this->store = (uint8_t *)mmap(NULL, size, PROT_RW, MAP_SHARED, fd, 0);
-	if ((void *)arena.Store == MAP_FAILED) {
+	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); 
+	this->fd = open(path, O_RDWR); 
-	if (arena.fd == -1) {
+	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,46 +142,61 @@ 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
+void
-DestroyArena(Arena &arena)
+Arena::Destroy()
 {
-	if (arena.Type == ARENA_UNINIT) {
+	if (this->arenaType == ARENA_UNINIT) {
-		return 0;
+		return;
 	}
 
-	switch (arena.Type) {
+	switch (this->arenaType) {
 		case ARENA_STATIC:
 			break;
 		case ARENA_ALLOC:
-		free(arena.Store);
+			delete this->store;
 			break;
 			#if defined(__linux__)
 			case ARENA_MMAP:
-		if (munmap(arena.Store, arena.Size) == -1) {
+				if (munmap(this->store, this->size) == -1) {
 					return -1;
 				}
 
-		if (close(arena.fd) == -1) {
+				if (close(this->fd) == -1) {
 					return -1;
 				}
 
-		arena.fd = 0;
+				this->fd = 0;
 				break;
 			#endif
 		default:
@@ -190,64 +208,66 @@ DestroyArena(Arena &arena)
 
 	}
 
-	arena.Type = ARENA_UNINIT;
+	this->arenaType = ARENA_UNINIT;
-	arena.Size = 0;
+	this->size = 0;
-	arena.Store = NULL;
+	this->store = nullptr;
-	return 0;
+	return;
 }
 
-#if defined(DESKTOP_BUILD)
+std::ostream &
-void
+operator<<(std::ostream &os, const Arena arena)
-DisplayArena(const Arena &arena)
 {
-	std::cout << "Arena @ 0x";
+	auto cursor = arena.NewCursor();
-	std::cout << std::hex << (uintptr_t)&arena << std::endl;
+	char cursorString[17] = {0};
-	std::cout << std::dec;
+	snprintf(cursorString, 16, "%0p", cursor);
-	std::cout << "\tStore is " << arena.Size << " bytes at address 0x";
-	std::cout << std::hex << (uintptr_t)&(arena.Store) << std::endl;
-	std::cout << "\tType: ";
 
-	switch (arena.Type) {
+	os << "Arena<";
+	switch (arena.Type()) {
 		case ARENA_UNINIT:
-		std::cout << "uninitialized";
+			os << "uninitialized";
 			break;
 		case ARENA_STATIC:
-		std::cout << "static";
+			os << "static";
 			break;
 		case ARENA_ALLOC:
-		std::cout << "allocated";
+			os << "allocated";
 			break;
 #if defined(__linux__)
 			case ARENA_MMAP:
-		std::cout << "mmap/file";
+				os << "mmap/file";
 				break;
 #endif
 		default:
-		std::cout << "unknown (this is a bug)";
+			os << "unknown (this is a bug)";
 	}
-	std::cout << std::endl;
+	os << ">@0x";
-}
+	os << std::hex << (uintptr_t) &arena;
-#else
+	os << std::dec;
-void
+	os << ",store<" << arena.Size() << "B>@0x";
-DisplayArena(const Arena &arena)
+	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;
 
+#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,
+	if (fwrite(this->store, sizeof(*this->store), this->size,
-		   arenaFile) == arena.Size) {
+		   arenaFile) == this->size) {
 		retc = 0;
 	}
 
@@ -258,3 +278,5 @@ WriteArena(const Arena &arena, const char *path)
 	return retc;
 }
 
+
+} // namespace klib

Arena.h

@@ -2,43 +2,82 @@
 #define KIMODEM_ARENA_H
 
 
+#include <iostream>
 #include <sys/stat.h>
 #include <cstddef>
 #include <cstdint>
 
 
-typedef struct {
+namespace klib {
-	uint8_t	*Store;
-	size_t	 Size;
-	int	 fd;
-	uint8_t	 Type;
-} Arena;
 
 
-/*
+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_arena
+	 * arena needs to be reset, it should be called with Clear or
- * or destroy_arena.
+	 * Destroy.
 	 */
-void	InitializeArena(Arena &arena);
+	void Initialize();
-int 	NewStaticArena(Arena &, uint8_t *, size_t);
+
-int 	AllocNewArena(Arena &, size_t);
+	int SetStatic(uint8_t *, size_t);
+
+	int SetAlloc(size_t allocSize);
+
 #if defined(__linux__)
-int 	MMapArena(Arena &, int); /* arena will own fd */
+	int 	 MemoryMap(int fd); // Arena will own fd.
-int	CreateArena(Arena &arena, const char *path, size_t size, mode_t mode);
+	int	 Create(const char *path, size_t size, mode_t mode);
-int 	OpenArena(Arena &, const char *, size_t);
+	int 	 Open(const char *, size_t);
 #endif
 
-void	ClearArena(Arena &);
+	uint8_t *NewCursor() const { return this->store; }
-int	DestroyArena(Arena &); /* dispose of any memory used by arena */
+	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
+	size_t Size() const
- * BACKING FILE! */
+	{ return this->size; }
-int	WriteArena(const Arena &arena, const char *path);
 
-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

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;
-	}
-
 	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;

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);
 

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 = (uintptr_t)cursor - (uintptr_t)arena.NewCursor();
-	remaining = arena.Size - remaining;
+	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);
 }

TLV.cc

@@ -1,33 +1,32 @@
 #include <cstring>
 #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;
+	return arena.CursorInArena(cursor + len);
-	remaining = arena.Size - remaining;
-	return ((size_t)remaining >= ((size_t)len+2));
 }
 
 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);
 }
 
 
@@ -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);
 }
 
@@ -100,7 +99,7 @@ LocateTag(Arena &arena, uint8_t *cursor, Record &rec)
 	uint8_t tag, len;
 
 	if (cursor == NULL) {
-		cursor = arena.Store;
+		cursor = arena.NewCursor();
 	}
 
 	while ((tag = cursor[0]) != rec.Tag) {
@@ -124,7 +123,8 @@ LocateTag(Arena &arena, uint8_t *cursor, Record &rec)
 
 
 uint8_t *
-FindEmpty(Arena &arena, uint8_t *cursor) {
+FindEmpty(Arena &arena, uint8_t *cursor)
+{
 	Record rec;
 
 	rec.Tag = TAG_EMPTY;
@@ -132,11 +132,10 @@ FindEmpty(Arena &arena, uint8_t *cursor) {
 }
 
 
-
 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);
 }
 
 
@@ -148,7 +147,7 @@ DeleteRecord(Arena &arena, uint8_t *cursor)
 	}
 
 	uint8_t len = cursor[1] + 2;
-	uint8_t		*stop = arena.Store + arena.Size;
+	uint8_t *stop = arena.NewCursor() + arena.Size();
 
 	stop -= len;
 
@@ -166,3 +165,4 @@ DeleteRecord(Arena &arena, uint8_t *cursor)
 
 
 } // namespace TLV
+} // namespace klib

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,
+* returns a pointer to memory where the record was found,
- * e.g. LocateTag(...)[0] is the tag of the found record.
+* e.g. LocateTag(...)[0] is the tag of the found record.
- * FindTag will call LocateTag and then SkipRecord if the
+* FindTag will call LocateTag and then SkipRecord if the
- * tag was found.
+* 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

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);
+	std::cout << "\twriting new rec1" << std::endl;
-	assert((cursor = TLV::WriteToMemory(backend, cursor, rec2)) != NULL);
+	assert(TLV::WriteToMemory(backend, cursor, rec1) != nullptr);
-	assert(TLV::WriteToMemory(backend, cursor, rec3) != NULL);
+	std::cout << "\twriting new rec2" << std::endl;
-	cursor = NULL;
+	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 != nullptr); 
-	assert(cursor != backend.Store);
+	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);
+	cursor = TLV::FindTag(backend, nullptr, rec4);
-	assert(cursor != NULL);
+	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) {
+	backend.Destroy();
-		std::cout << "FAILED" << std::endl;
-		return false;
-	}
 
 	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;