Repository navigation
fix: close the handles to an object pdf_annot_remove_object() removes (ADR-032) - #73
Merged
Merged
Conversation
pdf_annot_remove_object() destroys an object of an annotation's appearance stream, but the handles made to it earlier still pinned an open annotation, so they passed every check. Under valgrind, the removed object's handles read the freed object (4 invalid reads), a clip path read from it 1, and an object nested in a removed form 5. The handles pdf_annot_objects() returns are now registered under the annotation's document as a kind whose release only clears the handle, and cpp_annot_remove_object() releases those whose address is the removed object's once PDFium has removed it. The clip paths and nested objects read through them are then refused by the ADR-029 chain, with a message naming pdf_annot_remove_object(). A removal PDFium refuses leaves the handles open, and objects read through another handle to the same annotation are separate copies that stay valid. See ADR-032. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CLAUDE.md's memory-model section, dev/architecture.md and the architecture vignette now mention that the handles to an annotation's page-objects are registered under their document and that pdf_annot_remove_object() closes them (ADR-032). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR 71 (claude/handle-lifetime-leftovers, this branch's base) has merged, so the branch now targets main. The merge also brings in PR 70, which runs the pdf_image_extract() example only when png is installed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in the R-level handle finalizers (ADR-031) along with the verified downloads and rhub checks. The handle registry conflicted where both branches changed it, and the resolution keeps both designs: the annotation page-object kind, its close_nothing release and release_annot_object_handles() from this branch; the single R-level finalizer from main in place of this branch's table of C finalizers, and no memory-buffer kind, which ADR-031 removed. The registry comments, CLAUDE.md and dev/architecture.md combine both descriptions, and the ADR index lists 027 to 032 in order. Rcpp exports and roxygen output are unchanged by the merge. The full suite passes under MALLOC_CHECK_=3 (3681 passed); the weekly valgrind invocation reports 0 bytes lost and no invalid access; the removed- object reproducer is still refused with 0 invalid reads, and the unload scenarios still exit cleanly. Co-Authored-By: Claude Opus 5.5 <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
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
pdf_annot_remove_object()destroys an object of an annotation's appearance stream, but handles made to it earlier throughpdf_annot_objects()still pinned an open annotation. They passed every validity check and read the freed object. Now those handles, and any clip paths or nested form objects read through them, are refused with a clear error. The design is recorded as ADR-032, which extends ADR-025 and ADR-029.Changes
pdf_annot_objects()returns are registered in the per-document handle registry under a new kind whose release only clears the handle. PDFium owns the object.cpp_annot_remove_object()releases the handles whose address is the removed object's, but only after PDFium has actually removed it. PDFium refuses removal for subtypes other than ink and stamp, and clearing first would close handles to objects that still exist. A test pins the refused case.pdf_annot_remove_object().cpp_annot_get_object()andcpp_annot_remove_object()take the document handle.CLAUDE.mdmemory-model section,dev/architecture.md, the architecture vignette, and the Rd forpdf_annot_objects()andpdf_annot_remove_object().User-visible change
Objects read through a second handle to the same annotation are separate copies and stay valid.
Verification
Linux, R 4.6.1, valgrind 3.22, PDFium chromium/8066. Invalid reads under valgrind, before and after:
[ FAIL 0 | SKIP 1 | PASS 3611 ].MALLOC_CHECK_=3withlibc_malloc_debug: 3611 pass.handle_registry.cppandapi_completion.cpp.Merging with the R-level finalizers PR
This PR and #72 both change
src/handle_registry.{cpp,h}. Whichever lands second resolves the conflict:CLAUDE.mdanddev/architecture.md;🤖 Generated with Claude Code