pkmem: handle accessing uninitialized debug blocks

3 files changed, 113 insertions, 6 deletions

diff --git a/pkmem.h b/pkmem.h
index 50e251d..2339d22 100644
--- a/pkmem.h
+++ b/pkmem.h
@@ -251,6 +251,8 @@ pk_mem_bucket_create(const char* description, int64_t sz, enum PK_MEMBUCKET_FLAG
 	bkt->debug_head_r = 0;
 	bkt->debug_block_capacity = 128;
 	bkt->debug_blocks = (struct pk_memblock*)aligned_alloc(alignof(struct pk_memblock), sizeof(struct pk_memblock) * 128);
+	bkt->debug_blocks[0].ptr = NULL;
+	bkt->debug_blocks[0].size = 0;
 #endif
 
 	return bkt;
@@ -269,7 +271,7 @@ pk_mem_bucket_destroy(struct pk_membucket* bkt)
 void
 pk_mem_bucket_reset(struct pk_membucket* bkt)
 {
-	if (PK_HAS_FLAG(bkt->flags, PK_MEMBUCKET_FLAG_TRANSIENT) == true) {
+	if (PK_HAS_FLAG(bkt->flags, PK_MEMBUCKET_FLAG_TRANSIENT) == false) {
 		PK_LOG_ERR("WARNING: pk_bucket_reset called on non-transient pk_membucket\n");
 	}
 	bkt->head = 0;
@@ -288,6 +290,8 @@ pk_mem_bucket_reset(struct pk_membucket* bkt)
 #ifdef PK_MEMORY_DEBUGGER
 	bkt->debug_head_l = 0;
 	bkt->debug_head_r = 0;
+	bkt->debug_blocks[0].ptr = NULL;
+	bkt->debug_blocks[0].size = 0;
 #endif
 }
 
@@ -330,6 +334,7 @@ pk_bucket_insert_block(struct pk_membucket* bkt, const struct pk_memblock* block
 			break;
 		}
 	}
+	assert(old_block != NULL);
 	if (i == 0 && old_block != NULL) {
 		*old_block = *block;
 	} else {
@@ -414,20 +419,32 @@ pk_new_bkt(size_t sz, size_t alignment, struct pk_membucket* bkt)
 
 			if (bkt->debug_head_r == bkt->debug_block_capacity) {
 				struct pk_memblock *debug_blocks;
-				debug_blocks = (struct pk_memblock*)aligned_alloc(alignof(struct pk_memblock), sizeof(struct pk_memblock) * bkt->debug_block_capacity + 128);
+				debug_blocks = (struct pk_memblock*)aligned_alloc(alignof(struct pk_memblock), sizeof(struct pk_memblock) * (bkt->debug_block_capacity + 128));
 				assert(debug_blocks != NULL);
 				memcpy(debug_blocks, bkt->debug_blocks, sizeof(struct pk_memblock) * bkt->debug_block_capacity);
 				free(bkt->debug_blocks);
 				bkt->debug_blocks = debug_blocks;
+				bkt->debug_block_capacity += 128;
 			}
 
+			bkt->debug_blocks[bkt->debug_head_r].ptr = NULL;
+			bkt->debug_blocks[bkt->debug_head_r].size = 0;
+
 		} else {
+			// 2025-06-05 JCB
+			// This intentionally looks at debug_head_r, which could potentially
+			//  be uninitialized. I added some logic elsewhere to ensure that
+			//  whenever debug_head_r is incremented, we set the related block
+			//  to NULL/0 so that this will catch size==0.
+			// I was experiencing an issue where in testing it was initialized to
+			//  NULL/0, but then in a client application it was garbage data.
 			for (ii = bkt->debug_head_l+1; ii <= bkt->debug_head_r; ++ii) {
 				if (bkt->debug_blocks[ii].size == 0) {
 					bkt->debug_head_l = ii;
 					break;
 				}
 			}
+			assert(ii != bkt->debug_head_r+1);
 		}
 		assert(bkt->debug_head_l <= bkt->debug_head_r);
 		bkt->debug_blocks[i].data = (char*)data;
@@ -516,7 +533,10 @@ pk_delete_bkt(const void* ptr, size_t sz, struct pk_membucket* bkt)
 				if (i <= bkt->debug_head_l) {
 					bkt->debug_head_l = i-1;
 				}
-				if (i == bkt->debug_head_r) {
+				if (i == bkt->debug_head_r+1) {
+					if (bkt->debug_head_l == bkt->debug_head_r) {
+						bkt->debug_head_l--;
+					}
 					bkt->debug_head_r--;
 				}
 				assert(bkt->debug_head_l <= bkt->debug_head_r);
diff --git a/test/pkmem.c b/test/pkmem.c
index ad95899..36bf0eb 100644
--- a/test/pkmem.c
+++ b/test/pkmem.c
@@ -113,5 +113,72 @@ int main(int argc, char *argv[])
 		teardown();
 	}
 
+	// expand buckets
+	{
+		// spinup_w_instr();
+		spinup();
+
+		/*
+		 * [00-47] (48) ptr1
+		 * [48-XX] ( 1) HEAD
+		 */
+		void *ptr1 = pk_new_bkt(48, 1, mt.bkt1);
+		(void)ptr1;
+
+		if ((void*)&mt.bkt1->data[0] != ptr1) exit(1);
+		if (mt.bkt1->alloc_count != 1) exit(1);
+		if (mt.bkt1->head != 48) exit(1);
+		if (mt.bkt1->debug_head_l != 1) exit(1);
+		if (mt.bkt1->debug_head_r != 1) exit(1);
+		if (mt.bkt1->block_head_l != 0) exit(1);
+		if (mt.bkt1->block_head_r != 0) exit(1);
+
+		/*
+		 * [00-47] (48) ptr1
+		 * [   48] ( 1) ptr2
+		 * [49-XX] ( 1) HEAD
+		 */
+		void *ptr2 = pk_new_bkt(1, 1, mt.bkt1);
+
+		if ((void*)(&mt.bkt1->data[0]+48) != ptr2) exit(1);
+		if (mt.bkt1->alloc_count != 2) exit(1);
+		if (mt.bkt1->head != 49) exit(1);
+		if (mt.bkt1->debug_head_l != 2) exit(1);
+		if (mt.bkt1->debug_head_r != 2) exit(1);
+		if (mt.bkt1->block_head_l != 0) exit(1);
+		if (mt.bkt1->block_head_r != 0) exit(1);
+
+		/*
+		 * [00-47] (48) ptr1
+		 * [48-XX] ( 1) HEAD
+		 */
+		pk_delete_bkt(ptr2, 1, mt.bkt1);
+
+		if (mt.bkt1->alloc_count != 1) exit(1);
+		if (mt.bkt1->head != 48) exit(1);
+		if (mt.bkt1->debug_head_l != 1) exit(1);
+		if (mt.bkt1->debug_head_r != 2) exit(1);
+		if (mt.bkt1->block_head_l != 0) exit(1);
+		if (mt.bkt1->block_head_r != 0) exit(1);
+
+		/*
+		 * [00-47] (48) ptr1
+		 * [   48] ( 1) ptr2
+		 * [49-XX] ( 1) HEAD
+		 */
+		ptr2 = pk_new_bkt(1, 1, mt.bkt1);
+
+		if ((void*)(&mt.bkt1->data[0]+48) != ptr2) exit(1);
+		if (mt.bkt1->alloc_count != 2) exit(1);
+		if (mt.bkt1->head != 49) exit(1);
+		if (mt.bkt1->debug_head_l != 2) exit(1);
+		if (mt.bkt1->debug_head_r != 2) exit(1);
+		if (mt.bkt1->block_head_l != 0) exit(1);
+		if (mt.bkt1->block_head_r != 0) exit(1);
+
+		PK_LOGV_INF("%s: %s\n", __FILE__, "handles free last + new");
+		teardown();
+	}
+
 	return 0;
 }
diff --git a/test/pkmem.cpp b/test/pkmem.cpp
index 3a2b425..52b393b 100644
--- a/test/pkmem.cpp
+++ b/test/pkmem.cpp
@@ -71,7 +71,7 @@ class FreeTest01 : public FreeTest {
 	 [19-31] (13) memblock
 	 [ 32  ] ( 1) ptr2
 	 [33-63] (31) memblock
-	 [ 64  ] ( 1) ptr2
+	 [ 64  ] ( 1) ptr1
 	 [ 65  ] ( 0) HEAD
 */
 class FreeTest02 : public FreeTest {
@@ -455,12 +455,16 @@ int main(int argc, char *argv[])
 		FreeTest02 ft{};
 		ft.ensureState();
 
+		pk_mem_assert(3 == ft.bkt->alloc_count);
+		pk_mem_assert(3 == ft.bkt->debug_head_l);
+		pk_mem_assert(3 == ft.bkt->debug_head_r);
+
 		/* fill everything, then allocate [65], moving HEAD
 			 [00-18] (19) ptr0
 			 [19-31] (13) ptr3
 			 [ 32  ] ( 1) ptr2
 			 [33-63] (31) ptr4
-			 [ 64  ] ( 1) ptr2
+			 [ 64  ] ( 1) ptr1
 			 [ 65  ] ( 1) ptr5
 			 [ 66  ] ( 0) HEAD
 		*/
@@ -468,6 +472,10 @@ int main(int argc, char *argv[])
 		void *ptr4 = pk_new(31, 1, ft.bkt);
 		void *ptr5 = pk_new( 1, 1, ft.bkt);
 
+		pk_mem_assert(6 == ft.bkt->alloc_count);
+		pk_mem_assert(6 == ft.bkt->debug_head_l);
+		pk_mem_assert(6 == ft.bkt->debug_head_r);
+
 		fprintf(stdout, "block_head_r: %u\n", ft.bkt->block_head_r);
 		pk_mem_assert(0 == ft.bkt->block_head_r);
 		fprintf(stdout, "head                : %li\n", ft.bkt->head);
@@ -491,13 +499,17 @@ int main(int argc, char *argv[])
 			 [19-31] (13) memblock
 			 [ 32  ] ( 1) ptr2
 			 [33-63] (31) ptr4
-			 [ 64  ] ( 1) ptr2
+			 [ 64  ] ( 1) ptr1
 			 [ 65  ] ( 0) ptr5
 			 [ 66  ] ( 0) HEAD
 		*/
 		pk_delete(ptr3, 13, ft.bkt);
 		ptr3 = nullptr;
 
+		pk_mem_assert(5 == ft.bkt->alloc_count);
+		pk_mem_assert(3 == ft.bkt->debug_head_l);
+		pk_mem_assert(6 == ft.bkt->debug_head_r);
+
 		fprintf(stdout, "block_head_r: %u\n", ft.bkt->block_head_r);
 		pk_mem_assert(1 == ft.bkt->block_head_r);
 		fprintf(stdout, "head                : %li\n", ft.bkt->head);
@@ -522,12 +534,20 @@ int main(int argc, char *argv[])
 		pk_mem_assert(13 == ft.bkt->blocks[15].size);
 
 		/* free [32] which gets absorbed into 19-32
+			 [00-18] (19) ptr0
 			 [19-32] (13) memblock
 			 [33-63] (31) ptr4
+			 [ 64  ] ( 1) ptr1
+			 [ 65  ] ( 0) ptr5
+			 [ 66  ] ( 0) HEAD
 		*/
 		pk_delete(ft.ptrs[2], ft.sz[2], ft.bkt);
 		ft.ptrs[2] = nullptr;
 
+		pk_mem_assert(4 == ft.bkt->alloc_count);
+		pk_mem_assert(2 == ft.bkt->debug_head_l);
+		pk_mem_assert(6 == ft.bkt->debug_head_r);
+
 		fprintf(stdout, "block_head_r: %u\n", ft.bkt->block_head_r);
 		pk_mem_assert(1 == ft.bkt->block_head_r);
 		fprintf(stdout, "head                : %li\n", ft.bkt->head);