Fix bad-free heap corruption in ppathstatus and psyncer#392
Merged
Conversation
Both ppathstatus.c and psyncer.c used bare free() via ptree_for_each_element_call_safe to bulk-free tree nodes that were allocated with pmem_malloc. pmem_malloc prepends a pmem_header_t to every allocation, so the returned pointer is an interior pointer to the underlying glibc chunk. Passing it to free() makes glibc read a garbage size field from the pmem header and abort with "free(): invalid size". This is the same class of bug fixed earlier in pintervaltree.c and pfscrypto.c. The crash manifests reliably with larger files because more sync-queue and path-status churn occurs, increasing the likelihood that one of these bulk-free paths is hit while a non-empty tree exists. Fix: add free_folder_tasks_node() (ppathstatus.c) and free_synced_down_folder() (psyncer.c) helpers that call pmem_free, and use them as the ptree_for_each_element_call_safe callback in all three affected call sites. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two more call sites passed bare free() as a callback to tree/list traversal macros, but the nodes were allocated via pmem_malloc which prepends a pmem_header_t. Freeing the data pointer directly skips the header and corrupts the heap. - pfs.c: ptree_for_each_element_call_safe on sectorsinlog used free() on psync_sector_inlog_t; replaced with free_sector_inlog_node() that calls pmem_free(PMEM_SUBSYS_OTHER, e). - ppagecache.c: psync_list_for_each_element_call on request->ranges used free() on psync_request_range_t; replaced with free_request_range() that calls pmem_free(PMEM_SUBSYS_CACHE, range). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Unit test (test_pcl26j_free): exercises all four fixed call sites — psync_request_range_t, synced_down_folder, folder_tasks_t, and psync_sector_inlog_t — using --wrap=malloc/free to verify that pmem_free() passes the header pointer to free(), not the data pointer. Includes a harness self-check that confirms bare free(data_ptr) is detected. Would have failed against pre-fix code. Smoke test (smoke-test-large-read.sh): builds pcloudcc with ASAN, starts the daemon, streams a large file (>=50 MB) from the FUSE mount to /dev/null to force multi-range psync_request_range_t allocation and teardown via psync_pagecache_free_request, then scans the ASAN log for any bad-free reports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ppathstatus.c(×2) andpsyncer.c(×1) called barefree()viaptree_for_each_element_call_safeon nodes allocated withpmem_mallocpmem_mallocprepends apmem_header_t, making the returned pointer an interior pointer to the glibc chunk;free()reads garbage as the chunk-size field and aborts withfree(): invalid sizepintervaltree.c/pfscrypto.cfixes in 9c10304Fix
Added
free_folder_tasks_node()(ppathstatus.c) andfree_synced_down_folder()(psyncer.c) — thin wrappers callingpmem_free(PMEM_SUBSYS_OTHER, ...)— and substituted them for barefreein the three affectedptree_for_each_element_call_safecalls. Pattern matches the existingfree_interval_tree_nodefix.Test plan
free(): invalid size/PANIC: Abortcrashppathstatus_init()andpsyncer_dl_queue_clear()paths🤖 Generated with Claude Code