Implementation of sc-memory based on disk space - #495
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to fc6f7f5 in 3 minutes and 18 seconds. Click for details.
- Reviewed
4739lines of code in12files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sc-memory/sc-core/src/sc-store/sc-fs-memory/sc_file_system.c:92
- Draft comment:
BUG: The allocated 'command' buffer is not initialized before using strcat. This may lead to undefined behavior. Use strcpy or zero‐initialize the buffer. - Reason this comment was not posted:
Comment was on unchanged code.
2. sc-memory/sc-core/src/sc-store/sc-fs-memory/sc_file_system.c:205
- Draft comment:
BUG: 'char_result' is used without allocation. sc_str_cpy is called on an uninitialized pointer. - Reason this comment was not posted:
Comment was on unchanged code.
3. sc-memory/sc-core/src/sc-store/sc_segment.h:35
- Draft comment:
Typo in the parameter comment: "Number of created instance in sc-memory" should be "Number of created instances in sc-memory". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. sc-memory/sc-core/src/sc-store/sc_storage.c:660
- Draft comment:
Typographical note: The identifier 'null_ptr' in this new line may be a typo. If the intention is to use a null pointer literal, consider using 'nullptr' (in C++) or 'NULL' (in C) depending on your code's conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_Q1jJLymj506I50iT
You can customize 
| if (!cache->tail) | ||
| cache->tail = node; | ||
| cache->size++; | ||
| sc_hash_table_insert(cache->segments, &seg_num, node); |
There was a problem hiding this comment.
BUG: In _sc_l1_cache_add, using &seg_num as key for hash table insertion is unsafe. Use a stable key (e.g. GINT_TO_POINTER) instead.
| sc_hash_table_insert(cache->segments, &seg_num, node); | |
| sc_hash_table_insert(cache->segments, GUINT_TO_POINTER(seg_num), node); |
| io_destroy(ctx); | ||
| close(fd); | ||
|
|
||
| sc_hash_table_insert(cache->segments, &seg_num, (void *)(intptr_t)entry.offset); |
There was a problem hiding this comment.
BUG: In _sc_l2_cache_add, the key is inserted using &seg_num, a pointer to a local variable. Use a stable representation (e.g. GINT_TO_POINTER(seg_num)).
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| if (!cache->tail) | ||
| cache->tail = node; | ||
| cache->size++; | ||
| sc_hash_table_insert(cache->segments, &seg_num, node); |
There was a problem hiding this comment.
🚨 Bug: LRU cache inserts dangling pointer to stack key into hash table
_sc_l1_cache_add calls sc_hash_table_insert(cache->segments, &seg_num, node) and _sc_l2_cache_add calls sc_hash_table_insert(cache->segments, &seg_num, ...), passing the address of the seg_num function parameter. The GLib int64 hash table stores this key pointer, but it becomes dangling as soon as the function returns. Subsequent _sc_l1_cache_get/_sc_l2_cache_get lookups invoke g_int64_equal, which dereferences the stored dangling pointer — reading freed stack memory (undefined behavior), leading to unreliable cache hits/misses and potential crashes. Insert the address of a stably-stored key instead (e.g. &node->segment_num for L1, or a heap-allocated key for L2).
node->segment_num was already set from seg_num and outlives the entry:
// L1: key must live as long as the entry
sc_hash_table_insert(cache->segments, &node->segment_num, node);
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| storage->segments = malloc(storage->segments_count * sizeof(sc_segment*)); | ||
| if (!storage->segments) { | ||
| sc_fs_memory_error("Failed to allocate memory for segments array"); | ||
| close(fd); | ||
| return SC_FS_MEMORY_READ_ERROR; | ||
| } | ||
| for (sc_addr_seg i = 0; i < storage->segments_count; ++i) | ||
| { | ||
| sc_addr_seg const num = i; | ||
| sc_segment * seg = sc_segment_new(i + 1); | ||
| storage->segments[i] = seg; | ||
|
|
||
| for (sc_addr_seg j = 0; j < SC_SEGMENT_ELEMENTS_COUNT; ++j) | ||
| { | ||
| if (sc_io_channel_read_chars(segments_channel, (sc_char *)&seg->elements[j], element_size, &read_bytes, null_ptr) | ||
| != SC_FS_IO_STATUS_NORMAL | ||
| || read_bytes != element_size) | ||
| { | ||
| storage->segments_count = num; | ||
| sc_fs_memory_error("Error while sc-element %d in sc-segment %d reading", j, i); | ||
| goto error; | ||
| storage->segments[i] = null_ptr; |
There was a problem hiding this comment.
🚨 Bug: Segment load overwrites/undersizes storage->segments array
_sc_fs_memory_load_sc_memory_segments does storage->segments = malloc(storage->segments_count * sizeof(sc_segment*)), overwriting the array already allocated in sc_storage_initialize as sc_mem_new(sc_segment*, params->max_loaded_segments) (memory leak), and sizing it to segments_count rather than max_segments_count. After load, code such as _sc_storage_get_new_segment writes storage->segments[storage->segments_count] up to max_segments_count, causing a heap-buffer overflow past the smaller array. Keep the pre-allocated max_loaded_segments-sized array and only fill entries, rather than reallocating to segments_count.
Fix:
// Do not reallocate; reuse the array allocated in sc_storage_initialize
// (sized to max_loaded_segments). Just reset the used entries:
for (sc_addr_seg i = 0; i < storage->segments_count; ++i)
storage->segments[i] = null_ptr;
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| cache->capacity = capacity; | ||
| sc_monitor_init(&cache->monitor); | ||
|
|
||
| int fd = open(cache->cache_path, O_RDWR | O_CREAT | O_DIRECT, 0666); |
There was a problem hiding this comment.
⚠️ Bug: O_DIRECT I/O uses unaligned length/offset, will fail with EINVAL
The L2 cache opens its file with O_DIRECT and then writes/reads with unaligned parameters: the 16-byte metadata entry is written with a plain write(), and segment payloads are placed at entry.offset = offset + sizeof(entry) (16-byte granularity). O_DIRECT requires the buffer address, file offset, and transfer length to all be aligned to the device block size (typically 512/4096). Unaligned transfers return EINVAL, so write/io_submit/pread on these fds will consistently fail and the L2 cache will not function. Either drop O_DIRECT or align every offset and length to PAGE_SIZE.
Was this helpful? React with 👍 / 👎
| while (lseek(cache_fd, 0, SEEK_CUR) < cache_size) { | ||
| struct { sc_addr_seg num; off_t offset; } entry; | ||
| if (read(cache_fd, &entry, sizeof(entry)) != sizeof(entry)) { | ||
| sc_fs_memory_warning("Failed to read L2 cache entry"); | ||
| break; | ||
| } | ||
|
|
||
| // needed for sc-template search | ||
| if (!is_no_deprecated_segments) | ||
| { | ||
| seg->elements[j].incoming_arcs_count = 1; | ||
| seg->elements[j].outgoing_arcs_count = 1; | ||
| sc_segment* seg = aligned_alloc(PAGE_SIZE, sizeof(sc_segment)); | ||
| if (!seg) { | ||
| sc_fs_memory_warning("Failed to allocate segment for L2 cache"); | ||
| continue; | ||
| } | ||
| if (pread(cache_fd, seg, sizeof(sc_segment), entry.offset) != sizeof(sc_segment)) { | ||
| sc_fs_memory_warning("Failed to read segment from L2 cache"); | ||
| free(seg); | ||
| continue; |
There was a problem hiding this comment.
⚠️ Bug: L2 cache file read loop does not skip over segment payloads
On disk the L2 cache is laid out as [entry(16B)][segment][entry(16B)][segment]... because _sc_l2_cache_add writes the 16-byte entry sequentially and then writes the segment at entry.offset = pos+16. The load loop in _sc_fs_memory_load_sc_memory_segments reads a 16-byte entry, then preads the segment at entry.offset, but only advances the file position by 16 bytes. The next read therefore interprets the segment payload as the next entry, corrupting all subsequent cache reads. After reading an entry, seek past the segment payload (lseek(cache_fd, sizeof(sc_segment), SEEK_CUR)) before reading the next entry.
Was this helpful? React with 👍 / 👎
| return SC_FS_MEMORY_READ_ERROR; | ||
| } | ||
|
|
||
| storage->segments = malloc(storage->segments_count * sizeof(sc_segment*)); |
There was a problem hiding this comment.
⚠️ Bug: Allocator mismatch: aligned_alloc/malloc segments freed via sc_mem_free/sc_segment_free
Segments and the segments array loaded here are created with malloc/aligned_alloc, but elsewhere they are released with the project allocator: sc_storage_shutdown calls sc_segment_free(segment) on each and sc_mem_free(storage->segments). If sc_mem_new/sc_mem_free do not map directly to libc malloc/free (e.g. GLib slice or an instrumented allocator), freeing an aligned_alloc/malloc pointer through them is undefined behavior and can corrupt the heap. Use one consistent allocator for segments and the segments array across creation and destruction.
Was this helpful? React with 👍 / 👎
| if (!segment && op->type != SC_OP_ERASE_ELEMENT) | ||
| { | ||
| segment = sc_segment_new(op->addr.seg); | ||
| sc_monitor_acquire_write(&storage->segments_monitor); | ||
| storage->segments[op->addr.seg - 1] = segment; | ||
| if (op->addr.seg > storage->segments_count) | ||
| storage->segments_count = op->addr.seg; | ||
| sc_monitor_release_write(&storage->segments_monitor); | ||
| _sc_l1_cache_add(storage->l1_cache, op->addr.seg, segment); | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: _sc_storage_flush_buffer lacks upper-bound check on op->addr.seg
In the buffer-flush path, when no segment is found and the op is not an erase, it does storage->segments[op->addr.seg - 1] = segment without validating op->addr.seg against storage->max_segments_count (the earlier lookup only guards op->addr.seg <= storage->segments_count). A seg value of 0 yields segments[-1], and a value beyond max_segments_count writes past the array — both out-of-bounds writes. Add a bounds check (op->addr.seg >= 1 && op->addr.seg <= storage->max_segments_count) before indexing.
Was this helpful? React with 👍 / 👎
| @@ -1,8 +1,8 @@ | |||
| /* | |||
There was a problem hiding this comment.
💡 Quality: License headers and indentation mangled across files
Large portions of sc_file_system.c, sc_fs_memory.c and the private headers were reindented (2-space to 1-space) and the MIT license comment blocks lost their leading * alignment, producing ~124/-124 no-op churn that obscures the real changes and will conflict with the project style. Several files also lost their trailing newline. Revert the whitespace/header reformatting so the diff reflects only functional changes.
Was this helpful? React with 👍 / 👎
Code Review 🚫 Blocked 1 resolved / 8 findingsImplements disk-based sc-memory with L1/L2 caching and asynchronous I/O, but is blocked by critical memory safety issues, including dangling pointers in LRU cache inserts and heap corruption via allocator mismatches. Additionally, O_DIRECT alignment violations and improper segment loading logic prevent correct functional operation. 🚨 Bug: LRU cache inserts dangling pointer to stack key into hash table📄 sc-memory/sc-core/src/sc-store/sc_storage.c:1767 📄 sc-memory/sc-core/src/sc-store/sc_storage.c:1891
node->segment_num was already set from seg_num and outlives the entry🚨 Bug: Segment load overwrites/undersizes storage->segments array📄 sc-memory/sc-core/src/sc-store/sc-fs-memory/sc_fs_memory.c:290-297
Fix
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
1 participant
Important
Implements disk-based sc-memory with L1/L2 caching, asynchronous I/O, and buffered operations for enhanced efficiency and scalability.
sc_storage.cfor efficient memory management using DRAM and SSD/NVMe._sc_l1_cache_init(),_sc_l1_cache_destroy(),_sc_l1_cache_add(),_sc_l1_cache_get(),_sc_l2_cache_init(),_sc_l2_cache_destroy(),_sc_l2_cache_add(),_sc_l2_cache_get()for cache operations.sc_segment_bufferfor buffering operations insc_storage.c.write_segment_async()andwrite_segments_parallel()insc_fs_memory.cfor asynchronous and parallel segment writing.libaioandpthreadfor I/O operations._sc_storage_add_operation()and_sc_storage_flush_buffer()insc_storage.cto handle buffered operations.SC_OP_NODE_NEW,SC_OP_ARC_NEW,SC_OP_LINK_NEW,SC_OP_SET_LINK_CONTENT,SC_OP_ERASE_ELEMENT.sc_fs_memory.cto handle file-based storage, including reading and writing segments to disk.sc_file_system.candsc_io.hfor file operations.sc_storage_initialize()andsc_storage_shutdown()insc_storage.cto integrate new memory management features.sc_storage_private.hfor internal storage management.This description was created by
for fc6f7f5. You can customize this summary. It will automatically update as commits are pushed.