Skip to content

Commit 9fc2edd

Browse files
1a1a11aclaude
andauthored
fix(pqueue): make a failed queue growth fatal instead of corrupting the heap (#334)
#333 changed pqueue to grow geometrically so Size and Belady could stop reserving 64MB apiece. It merged without this guard -- the review that found the problem landed while the merge was in flight -- so develop now has the growth policy and not the check. pqueue_insert returns 1 when the realloc fails, and the header documents "0 on success", but no caller in the tree checks it: Size.c:181, Belady.c:201, PG.c:344 and PG.c:399 all discard the return. They could not act on it if they did. Size_insert calls cache_insert_base first, so the object is already in the cache, then allocates the node with my_malloc -- plain malloc, not zeroed -- and assigns cached_obj->Size.pq_node = node whether or not the insert took. bubble_up never ran, so node->pos holds whatever was in that heap cell. Size_remove_obj sees a non-NULL pq_node and calls pqueue_remove: size_t posn = q->getpos(d); q->d[posn] = q->d[--q->size]; an unbounded write at that uninitialised index. Not a wrong miss ratio -- a heap out-of-bounds write. The path predates #333, but 8 million entries of slack stood in front of it. Growth is how the queue reaches its working size now, so it becomes reachable from about 65K objects onward, and specifically under the virtual-memory limits the smaller reservation was meant to fit inside. Terminate instead, which is how create_chained_hashtable_v2 already handles a failed table allocation, and which covers PG's two call sites by the same guard. Checking at the callers was the alternative and does not work: by the time the insert is attempted the object is in the cache, so there is no atomic recovery short of unwinding cache_insert_base. Verified by injecting a failed realloc on the third growth: the run aborts with "cannot grow priority queue to 136 entries (1088 bytes)" and rc=134 rather than continuing into the corrupting path. Suite passes 10/10 with 185 CLI checks, and neither memory nor miss ratios move. Claude-Session: https://claude.ai/code/session_01YUq1vM4g82TkaX2jvLmuQc Co-authored-by: Claude <noreply@anthropic.com>
1 parent 42b235b commit 9fc2edd

1 file changed

Lines changed: 20 additions & 1 deletion

File tree

libCacheSim/dataStructure/pqueue.c

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@
3333
#include <stdlib.h>
3434
#include <string.h>
3535

36+
#include "../include/libCacheSim/logging.h"
37+
3638
#ifdef __cplusplus
3739
extern "C" {
3840
#endif
@@ -148,7 +150,24 @@ int pqueue_insert(pqueue_t *q, void *d) {
148150
* q->step is left as the record of the initial capacity, which
149151
* pqueue_duplicate still copies. */
150152
newsize = q->avail * 2;
151-
if (!(tmp = realloc(q->d, sizeof(void *) * newsize))) return 1;
153+
tmp = realloc(q->d, sizeof(void *) * newsize);
154+
if (tmp == NULL) {
155+
/* Fatal, like the hash table's allocation failure, rather than the
156+
* documented "return non-zero". Every caller in the tree ignores the
157+
* return, and they cannot use it safely even if they checked: Size and
158+
* Belady have already put the object in the cache and gone on to attach
159+
* the node to it, so a queue that refused the insert leaves an object
160+
* whose pq_node->pos was never set by bubble_up. pqueue_remove then
161+
* writes to q->d at that uninitialised index. Dying here with a legible
162+
* message beats corrupting the heap and reporting a miss ratio.
163+
*
164+
* Growing is how the queue reaches its working size now, so this is a
165+
* real path under the virtual-memory limits this sizing is meant to fit
166+
* inside, not only at true exhaustion. */
167+
ERROR("cannot grow priority queue to %zu entries (%zu bytes)\n", newsize,
168+
sizeof(void *) * newsize);
169+
exit(1);
170+
}
152171
q->d = tmp;
153172
q->avail = newsize;
154173
}

0 commit comments

Comments
 (0)