Check the free lists on the cold walks instead of faulting on a corrupted link - #23
Open
robobun wants to merge 1 commit into
Open
Conversation
…pted link A free block holds its next link in its own first word. When something writes into a block after it was freed, or a stale free links a live object, the link points anywhere. Release builds do not encode links, so the walks that read a whole list followed it and faulted: the idle sweep (mi_page_purge_holes_walk), the forced collect of local_free in mi_page_free_collect_ex, and mi_page_thread_collect_to_local, which bounded the length of the thread-free list but not where its links point. This is the crash family behind oven-sh/bun BUN-40BH, BUN-40CP, BUN-40SQ and BUN-41H5. The three walks now check every link: it has to be NULL or the start of a formed block of the page. The sweep also tracks which blocks it has seen, so a block listed twice is found instead of counted twice (two counts for one block could make an OS page with one live block look entirely free and get discarded) and a cyclic list ends. A corrupted list is reported through _mi_error_message (EFAULT, as the existing thread-free message does) and cut in front of the block holding the bad link; the blocks cut off are counted as used from then on, so they are never handed out and the page is never returned to the arena while one of them may still be live. The sweep knows how many blocks remain listed and sets used exactly; the collects bump it by one. The thread-free collect keeps its existing recovery and drops the list it took. The scavenger thread blocked every signal, including the ones a fault on the thread itself raises. A blocked SIGSEGV or SIGBUS is not queued: the kernel resets it to the default action and kills the process, so a fault during a sweep the scavenger did for a parked thread ended the process with no report from the host's crash handler (verified on Linux). Leave the thread-directed fault signals unblocked; the process-directed ones stay blocked as before. test-freelist-corruption scribbles a freed block the way the crashes look (a link 8 bytes into the block, the word 0xA0D behind it) on each of the three lists, plus a bad head and a self-linked block, and checks the report, the cut, the block accounting, that a second pass is quiet, that the block is not handed out again and that the live blocks are intact. Without the src changes four of the five tests fault and the self-link test loops.
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
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.
Problem
mi_page_purge_holes_walk(src/page.c:704, oven-sh/bun BUN-40BH, BUN-40CP),mi_page_thread_collect_to_local(:268, BUN-40SQ, BUN-3ZSP), the forced collect inmi_page_free_collect_ex(:1209, BUN-41H5). The fault address is the garbage in the link, so something writes into blocks after they are freed.Fix
_mi_error_message(EFAULT, ...)and cut in front of the block that holds the bad link. The blocks cut off count as used from then on: they are never handed out, and the page stays out of the arena while one of them may be live. The thread-free collect keeps its existing recovery.test-freelist-corruption(new, 5 cases) faults in 4 and loops in 1 without thesrc/changes and passes with them, in Release andMI_DEBUG_FULL. The rest of the suite is unchanged.Background
nextlink in its own first word. Debug and secure builds encode it, andmi_block_nextrejects a link that leaves the page. Release builds store it plainly._mi_error_messagedoes not abort. It prints whenshow_errorsis set and calls themi_register_errorhandler, so an embedder can count or report these.used + free listed + discarded == capacity. A cut moves the dropped blocks intoused. The sweep knows how many blocks remain and setsusedexactly; the collects add one.Notes
_mi_malloc_generic, and it already chases every link it collects._mi_page_mallocpoppingpage->freeis not touched. The sweep zeroescapacity / 8bytes per walked page and keeps an 8 KiB array on the stack, as the report walker already does.0xA0D, single bits, pointers with their low dword replaced,-1.usedbecause they do not know how long the dropped tail was. Such a page only looks emptier than it is; the block holding the bad link is pinned either way. The test for that path corrupts the last block of the list, so its accounting check is exact too._mi_thread_idle_work, the same garbage in the allocation path (BUN-4616 pops0xA0Dinmi_page_malloc_zero), and the same shape on 1.3.x builds before the sweep existed (BUN-316K, BUN-3WB5). The writers are in bun, not in the allocator: io(windows): keep the buffer of a file read started during chunk delivery bun#39897 (a Windows file read that completes into a freed buffer) and the usockets poll double free from usockets(windows): keep a closed poll alive until the outer tick and libuv are done with it bun#39643. This change does not fix those. It only makes the walks report the damage and keep going, and fixes the two allocator-side items above (the signal mask, the double count). Whether the link checks belong in the allocator at all is for the maintainers to decide; the patch splits cleanly if only the two allocator-side items are wanted.6a14aee2in this environment, unrelated to this change:test-purge-holesfailsunformed-tailinMI_DEBUG_FULL;test-heap-mtsegfaults inheap-free-during-delete-overlapabout one run in three under load (mi_stat_freereadspage->heap->subprocof a heap being deleted,src/free.c:718, reported separately);src/prof.cneeds-D_GNU_SOURCEto build with cmake here.ctestin Release 20/20. InMI_DEBUG_FULLall pass except the two pre-existing failures above, the same two as without this change.